1570064 - Allow app (e.g. Thunderbird) to extend the system-headers variable

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Kai Engert (:KaiE:)]

I couldn't figure out where inside Thunderbird's moz.build files I might extend the system-headers variable. I tried inside comm/mail/app.mozbuild, but got an error that the system-headers variable is undefined.

I suggest to add a new configuration variable, that an application can define, and that triggers inclusion of an app defined file.

I'll attach a patch.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1570064-v1.patch

Comment 2

[Kai Engert (:KaiE:)]

The contents of the app defined file can then be something like:

system_headers += [
'file1.h',
'file2.h',
]

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 9081691 [details] [diff] [review]
1570064-v1.patch

Review of attachment 9081691 [details] [diff] [review]:
-----------------------------------------------------------------

I am slightly nervous about extending the ability to `include()` random files from moz.build files.  I think somehow I'd want to make it a property of what app you're building, putting it at some fixed position under the `MOZ_BUILD_APP` directory instead.

::: old-configure.in
@@ +3238,5 @@
>  dnl Host JavaScript runtime, if any, to use during cross compiles.
>  AC_SUBST(JS_BINARY)
>  
>  AC_SUBST(NSS_EXTRA_SYMBOLS_FILE)
> +AC_SUBST(MOZ_APP_SYSTEM_HEADERS)

This should really be an `option('MOZ_APP_SYSTEM_HEADERS', help=...)` defined in `toolkit/moz.configure`.  And then the argument should be checked for existence, file-ness, etc.; see examples in other .configure files.  We'd probably want to enforce that it's an absolute path?  Not sure.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Nathan Froyd [:froydnj] from comment #3)

I am slightly nervous about extending the ability to include() random
files from moz.build files. I think somehow I'd want to make it a property
of what app you're building, putting it at some fixed position under the
MOZ_BUILD_APP directory instead.

That's fine. We probably don't want to include the "extra" file for browser/firefox. An application could set a variable as a flag, and if it's set, we load the predefined filename from the app directory, e.g. MOZ_BUILD_APP/app-system-headers.mozbuild

+AC_SUBST(MOZ_APP_SYSTEM_HEADERS)

This should really be an option('MOZ_APP_SYSTEM_HEADERS', help=...)
defined in toolkit/moz.configure.

I'd like to avoid that an option must be manually added to mozconfig for everyone trying to build Thunderbird.

And then the argument should be checked
for existence, file-ness, etc.; see examples in other .configure files.
We'd probably want to enforce that it's an absolute path? Not sure.

If an application explicitly set the variable to request inclusion, why is sanity checking necessary? FYI, we've done something similar for security/nss.symbols, see end of file:

#ifdef NSS_EXTRA_SYMBOLS_FILE
#include @NSS_EXTRA_SYMBOLS_FILE@
#endif

Comment 5

[Kai Engert (:KaiE:)]

Attachment: 1570064-v2.patch

Avoid arbitrary inclusion, use fixed filename and location.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: example.patch

An example application definition for v2

Comment 7

[Kai Engert (:KaiE:)]

I tried to duplicate the entry in toolkit/moz.configure for MOZ_RUST_SIMD, to this:

option('--enable-app-system-headers', env='MOZ_APP_SYSTEM_HEADERS',
       help='Use additional system headers defined in MOZ_BUILD_APP/app-system-headers.mozbuild')

@depends('--enable-app-system-headers')
def app_system_headers(value):
    if value:
        return True

set_config('MOZ_APP_SYSTEM_HEADERS', app_system_headers)
set_define('MOZ_APP_SYSTEM_HEADERS', app_system_headers)

This works if I use ac_add_options --enable-app-system-headers in mozconfig.

But I need it to work with an entry in MOZ_BUILD_APP/confvars.sh to avoid that everyone must update their mozconfig.

After adding AC_SUBST(MOZ_APP_SYSTEM_HEADERS) to old-configure.in, defining MOZ_APP_SYSTEM_HEADERS=1 in confvars.sh worked. However, with that change, the mozconfig parameter --enable-app-system-headers no longer works, I get error
mozbuild.configure.ConfigureError: Cannot add 'MOZ_APP_SYSTEM_HEADERS' to configuration: Key already exists

Could you please advise a mechanism that works with a definition in confvars.sh, and also satisfies your requirement to use option() in toolkit/moz.configure? (I don't need a configuration option, the attached patches work for me.)

Comment 8

[Kai Engert (:KaiE:)]

Attachment: 1570064-option.patch

experimental patch with option() that doesn't work with MOZ_APP_SYSTEM_HEADERS=1 in confvars.sh

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Kai Engert (:kaie:) from comment #7)

I tried to duplicate the entry in toolkit/moz.configure for MOZ_RUST_SIMD, to this:

option('--enable-app-system-headers', env='MOZ_APP_SYSTEM_HEADERS',
       help='Use additional system headers defined in MOZ_BUILD_APP/app-system-headers.mozbuild')

@depends('--enable-app-system-headers')
def app_system_headers(value):
    if value:
        return True

set_config('MOZ_APP_SYSTEM_HEADERS', app_system_headers)
set_define('MOZ_APP_SYSTEM_HEADERS', app_system_headers)

This works if I use ac_add_options --enable-app-system-headers in mozconfig.

But I need it to work with an entry in MOZ_BUILD_APP/confvars.sh to avoid that everyone must update their mozconfig.

After adding AC_SUBST(MOZ_APP_SYSTEM_HEADERS) to old-configure.in, defining MOZ_APP_SYSTEM_HEADERS=1 in confvars.sh worked. However, with that change, the mozconfig parameter --enable-app-system-headers no longer works, I get error
mozbuild.configure.ConfigureError: Cannot add 'MOZ_APP_SYSTEM_HEADERS' to configuration: Key already exists

Could you please advise a mechanism that works with a definition in confvars.sh, and also satisfies your requirement to use option() in toolkit/moz.configure? (I don't need a configuration option, the attached patches work for me.)

Did you try defining MOZ_APP_SYSTEM_HEADERS in confvars.sh, not specifying the --enable* command-line option, and not adding anything to old-configure.in? The set_config in moz.configure makes the AC_SUBST unnecessary.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to Nathan Froyd [:froydnj] from comment #9)

Did you try defining MOZ_APP_SYSTEM_HEADERS in confvars.sh, not specifying the --enable* command-line option, and not adding anything to old-configure.in? The set_config in moz.configure makes the AC_SUBST unnecessary.

Yes, I tried.

With that, the following code in system-headers.mozbuild reaches the else: branch.

if CONFIG['MOZ_APP_SYSTEM_HEADERS']:
    include("../" + CONFIG['MOZ_BUILD_APP'] + "/app-system-headers.mozbuild")
else:
    include("does not exist")

Note I tested this on a beta tree.

Comment 11

[Kai Engert (:KaiE:)]

Same results with mozilla-central

Comment 12

[Nathan Froyd [:froydnj]]

I guess imply_option('--enable-app-system-headers', True) in suite/moz.configure prior to including toolkit/moz.configure is worth a shot?

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Nathan Froyd [:froydnj] from comment #12)

I guess imply_option('--enable-app-system-headers', True)

... instead of defining a variable in confvars.sh ...

in suite/moz.configure prior

(suite/ is for SeaMonkey, but Thunderbird has the equivalent file in mail/)

to including toolkit/moz.configure is worth a shot?

That works, thanks a lot!

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Bug 1570064 - Allow app (e.g. Thunderbird) to extend the system-headers variable. r?froydnj

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c87bbf21eed
Allow app (e.g. Thunderbird) to extend the system-headers variable. r=froydnj

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7c87bbf21eed