1341234 - bindgen doesn't pass CFLAGS for optional system dependencies

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jan Beich]

Downstream may want to use system libraries for easier patching across many Gecko versions/forks but Stylo needs to know at least where to find NSPR and Pixman if not under objdir/dist/include.

$ echo "export LLVM_CONFIG=$(which llvm-config39)" >>.mozconfig
$ echo "ac_add_options --enable-stylo" >>.mozconfig

$ echo "ac_add_options --enable-system-pixman" >>.mozconfig
$ echo "ac_add_options --with-system-nspr" >>.mozconfig
$ echo "ac_add_options --with-system-nss" >>.mozconfig # bug 1341222 workaround

$ ./mach build
[...]
error: failed to run custom build command for `style v0.0.1 (file://servo/components/style)`
process didn't exit successfully: `objdir/toolkit/library/gtest/rust/./release/build/style-2f09a98c1583a348/build-script-build` (exit code: 101)
--- stderr
objdir/dist/include/nsISupportsImpl.h:18:10: fatal error: 'prthread.h' file not found
objdir/dist/include/nsString.h:205:10: fatal error: 'plhash.h' file not found
thread '<unnamed>' panicked at 'Unable to generate bindings: ()', src/libcore/result.rs:837
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any', src/libcore/result.rs:837
[...]
error: failed to run custom build command for `style v0.0.1 (file://servo/components/style)`
process didn't exit successfully: `objdir/toolkit/library/gtest/rust/./release/build/style-2f09a98c1583a348/build-script-build` (exit code: 101)
--- stderr
objdir/dist/include/nsRegion.h:27:10: fatal error: 'pixman.h' file not found
thread '<unnamed>' panicked at 'Unable to generate bindings: ()', src/libcore/result.rs:837
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any', src/libcore/result.rs:837
[...]

$ cat objdir/config/autoconf.mk
MOZ_PIXMAN_CFLAGS = -I/usr/local/include/pixman-1 ...
NSPR_CFLAGS = -I/usr/local/include/nspr

Comment 1

[Chris Peterson [:cpeterson]]

Nick, is this an issue with rust-bindgen itself or with Firefox's build configuration?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is an issue with Firefox's build. Needs also a change to the style system's build script, but that's no big deal. In particular, we need a way from the build script to inject the flags into bindgen, an env var will do.

But we also need a way to get them, and I don't know which variables are used when. We should try to mimic the environment of whatever is passed when compiling ServoBindings.cpp.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Seems like Emilio is on top of this.

Comment 4

[Chris Peterson [:cpeterson]]

Passing optional CFLAGS to bindgen doesn't need to block building Stylo by default (bug 1356991).

Comment 5

[Landry Breuil (:gaston)]

Fwiw, this will soon hit users/distributors in production, as it's now impossible to build beta using --with-system-nspr (cf #1384659)  - please provide an ETA for a workaround/fix...

Comment 6

[Jan Beich]

(In reply to Chris Peterson [:cpeterson] from comment #4)
> Passing optional CFLAGS to bindgen doesn't need to block building Stylo by
> default (bug 1356991).

Should it block bug 1374034 then? Alpine, Arch, BSDs, Debian (unstable), Fedora, Gentoo default to system NSPR. Arch, Alpine and BSDs also use system Pixman. DragonFly (BSD) doesn't support bundled NSPR.

Comment 7

[Landry Breuil (:gaston)]

Another issue/workaround with that is that we're going to use --disable-stylo, which will reduce its coverage for 3rd-party... where it would be crucially needed.

Comment 8

[Chris Peterson [:cpeterson]]

Nathan, this is the bug about optional CFLAGS for bindgen needed by distro builds. Emilio said in comment 2 that this can be fixed in the Firefox build system and doesn't seem to need changes to rust-bindgen itself.

Comment 9

[Ralph Giles (:rillian)]

As a work-around, you should be able to patch the CFLAGS you need into layout/style/bindgen.toml.in. The options need to be toml-ized, i.e. double-quote each argument and add them to the `args` array with comma separators.

Comment 10

[Landry Breuil (:gaston)]

Yes as a workaround i can also stab myself with a rusted fork :) Seriously, the rust/stylo build system integration needs more consideration for often-used build options, like supporting building against external libs as most distributors with common sense do by default...... we shouldnt have to patch this in 56.

Comment 11

[Landry Breuil (:gaston)]

I'm trying as hard as i can to reenable stylo, but even with BINDGEN_CFLAGS in the env, stylo wont build w/ 56.0b1:

   Compiling gkrust v0.1.0 (file:///usr/obj/ports/firefox-56.0beta1/firefox-56.0b1/toolkit/library/rust)
     Running `/usr/local/bin/rustc --crate-name gkrust /usr/obj/ports/firefox-56.0beta1/firefox-56.0b1/toolkit/library/rust/lib.rs --crate-type staticlib --emit=dep-info,link -C opt-level=2 -C panic=abort -C lto --cfg 'feature="bindgen"' --cfg 'feature="gkrust-shared"' --cfg 'feature="no-static-ideograph-encoder-tables"' --cfg 'feature="servo"' -C metadata=9428cf8d04e0b67f -C extra-filename=-9428cf8d04e0b67f --out-dir /usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/x86_64-unknown-openbsd/release/deps --target x86_64-unknown-openbsd -C linker=/usr/obj/ports/firefox-56.0beta1/firefox-56.0b1/build/cargo-linker -L dependency=/usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/x86_64-unknown-openbsd/release/deps -L dependency=/usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/release/deps --extern gkrust_shared=/usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/x86_64-unknown-openbsd/release/deps/libgkrust_shared-76a5242837fd53fb.rlib -C opt-level=0 -C debug-assertions=no -C debuginfo=2`
error: Could not compile `gkrust`.

Caused by:
  process didn't exit successfully: `/usr/local/bin/rustc --crate-name gkrust /usr/obj/ports/firefox-56.0beta1/firefox-56.0b1/toolkit/library/rust/lib.rs --crate-type staticlib --emit=dep-info,link -C opt-level=2 -C panic=abort -C lto --cfg feature="bindgen" --cfg feature="gkrust-shared" --cfg feature="no-static-ideograph-encoder-tables" --cfg feature="servo" -C metadata=9428cf8d04e0b67f -C extra-filename=-9428cf8d04e0b67f --out-dir /usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/x86_64-unknown-openbsd/release/deps --target x86_64-unknown-openbsd -C linker=/usr/obj/ports/firefox-56.0beta1/firefox-56.0b1/build/cargo-linker -L dependency=/usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/x86_64-unknown-openbsd/release/deps -L dependency=/usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/release/deps --extern gkrust_shared=/usr/obj/ports/firefox-56.0beta1/build-amd64/toolkit/library/x86_64-unknown-openbsd/release/deps/libgkrust_shared-76a5242837fd53fb.rlib -C opt-level=0 -C debug-assertions=no -C debuginfo=2` (signal: 11, SIGSEGV: invalid memory reference)


and the buildbot on i386 also fails on it: http://buildbot.rhaalovely.net/builders/mozilla-beta-i386/builds/92/steps/build/logs/stdio. In fact beta started failing after http://buildbot.rhaalovely.net/builders/mozilla-beta-amd64/builds/84, which matches the last m-c merge when beta went from 55 to 56.

Comment 12

[Nathan Froyd [:froydnj]]

That does sound like bindgen completed successfully and you were able to build the style crate, though, so that's good news!  It's unfortunate that things fall over when building gkrust itself, though...

Comment 13

[Ralph Giles (:rillian)]

That definitely is progress. Note that we have problem with stylo in i686-linux too (bug 1366050). I don't see anything in the buildbot log about why the style crate compile failed. I did have trouble with libclang crashing (or worse, hanging) when called from build-script-build for the style crate. The 4.0.1 release seems to mostly work.

Comment 16

[Landry Breuil (:gaston)]

Can we hopefully move forward on this bug ? I'd like to dogfood stylo in the 57.0 cycle, and if this doesnt build, that doesnt help much.. what's needed ?

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Landry Breuil (:gaston) from comment #16)
> Can we hopefully move forward on this bug ? I'd like to dogfood stylo in the
> 57.0 cycle, and if this doesnt build, that doesnt help much.. what's needed ?

It sounds to me (comment 12) that you have patches to enable bindgen to run successfully (comment 11), but then things fall over later.  Perhaps we just need to get your patches into the tree, and then you can debug the Rust compilation problems further?

Comment 18

[Landry Breuil (:gaston)]

Sorry, but https://cgit.rhaalovely.net/mozilla-firefox/commit/?h=beta&id=0ec116a0052fb8550230444a22fc2a74a0291a3e is just a bandaid/local hack i dont want to see upstreamed. And even with this, stylo fails to build within 57.0b3 regardless of the arch (per bug 1401093 comment 14)

interestingly, plain/pristine mozilla-beta build succeeds.. so maybe related to building with external nss/nspr ? doubt so.

Comment 19

[Jan Beich]

FWIW, FreeBSD has no issues building Stylo with system NSPR and Pixman using BINDGEN_CFLAGS workaround in .mozconfig. Obviously, only on amd64 due to bug 1401093.

http://beefy6.nyi.freebsd.org/data/103amd64-default/450797/logs/firefox-56.0,1.log
http://beefy9.nyi.freebsd.org/data/110amd64-default/450797/logs/firefox-56.0,1.log
http://beefy12.nyi.freebsd.org/data/head-amd64-default/p450797_s324074/logs/firefox-56.0,1.log (ipv6-only)

Comment 20

[Landry Breuil (:gaston)]

Still, a workaround is a workaround. If --with-system-foo is provided as an option, it should be 'supported', and there it's been broken since 6+ months.

Comment 21

[Landry Breuil (:gaston)]

a bit orthogonal to this bug, but i just saw https://dxr.mozilla.org/mozilla-beta/source/servo/components/style/build_gecko.rs#121 - this is probably why build with bundled nspr 'just works' (tm), but it feels like a hack.

Jan, is there a particular reason for using -isystem in BINDGEN_CFLAGS when pointing at nspr and pixman headers ? why not -I ?

Comment 22

[Mike Hommey [:glandium]]

This should be using NSPR_CFLAGS in all cases (with and without system nspr)

Comment 23

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #21)
> Jan, is there a particular reason for using -isystem in BINDGEN_CFLAGS when
> pointing at nspr and pixman headers ? why not -I ?

Just a habit.

-isystem silences warnings in system headers. Useful for -Werror builds but Firefox already has ALLOW_COMPILER_WARNINGS for stuff maintained elsewhere. bindgen probably ignores warnings as well given the underspecified CPPFLAGS.

-isystem puts the path at the end of include path order. If both -I${LOCALBASE}/include/nspr -I$(DIST)/include/nspr are passed there can be an ambiguity which header to use. NSPR and Pixman have stable API/ABI and don't install headers under locations searched by default, so such an issue is rare. Overcrowded -I${LOCALBASE}/include is more popular on FreeBSD e.g., bug 1228208.

Comment 25

[Landry Breuil (:gaston)]

So in the end with a pile of hacks (that, and default_rustflags = -C opt-level=2 to avoid issues with libgkrust.a linking?), i managed to build & run stylo w/ 57.0b4 (i386 and amd64). 

 [build]
 args = [
+    "-I/usr/local/include/nspr",
+    "-I/usr/X11R6/include/pixman-1",
     @BINDGEN_CFLAGS@
 ]

is an ugly fix. I dunno when @BINDGEN_CFLAGS@ is substituted by the build system (https://dxr.mozilla.org/mozilla-beta/source/build/moz.configure/toolchain.configure#900 ?), but it should definitely have at least NSPR_CFLAGS, PIXMAN_CFLAGS, and probably NSS_CFLAGS, CAIRO_CFLAGS, etc.. if they have to be added to the value given to BINDGEN_CFLAGS or added separately in the args array is out of my league, but a proper fix has to be devised for 57 final, please.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this as a regression since bug 1393000 was flagged that way. 
Is this something you are aiming to fix for 57?  Is anyone working on this? If not, maybe better to aim for a fix in 58.

Comment 27

[Landry Breuil (:gaston)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Marking this as a regression since bug 1393000 was flagged that way. 
> Is this something you are aiming to fix for 57?  Is anyone working on this?
> If not, maybe better to aim for a fix in 58.

Afaict nobody cares about distributors using --with-system-foo, so nobody is working on this upstream, and everyone downstream is using patches/hacks.

Comment 28

[Wolfgang Rosenauer [:wolfiR]]

Since I'm running into the same. From above I cannot easily tell what is the actual workaround now. Is it possible that someone who has it compiling sums up what was changed?
Actually if Mozilla is not providing a fix or at least a general patch before 57.0 this version will be shipped with stylo disabled in some downstream distributions including openSUSE which probably is not what mozilla upstream wants to see?

Comment 29

[Emilio Cobos Álvarez (:emilio)]

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #28)
> Since I'm running into the same. From above I cannot easily tell what is the
> actual workaround now. Is it possible that someone who has it compiling sums
> up what was changed?
> Actually if Mozilla is not providing a fix or at least a general patch
> before 57.0 this version will be shipped with stylo disabled in some
> downstream distributions including openSUSE which probably is not what
> mozilla upstream wants to see?

The workaround is using BINDGEN_CFLAGS in .mozconfig pointing to the relevant include directories.

Nathan, how hard would this be to fix? Any chance anyone in the build team can take this?

Comment 30

[Nathan Froyd [:froydnj]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #29)
> The workaround is using BINDGEN_CFLAGS in .mozconfig pointing to the
> relevant include directories.
> 
> Nathan, how hard would this be to fix? Any chance anyone in the build team
> can take this?

There are two ways to fix it:

1. (The easier, hackish way.) Convert the set_config of BINDGEN_CFLAGS in build/moz.configure/toolchain.configure to add_old_configure_assignment('_INITIAL_BINDGEN_CFLAGS', ...).  Then, in {,js/src/}old-configure.in, you'll want to set BINDGEN_CFLAGS to something like "$_INITIAL_BINDGEN_CFLAGS $NSPR_CFLAGS" (and include whatever else has --with-system options, like PIXMAN_CFLAGS, I guess?), and AC_SUBST_LIST(BINDGEN_CFLAGS).

2. (The better, harder way.) Move --with-system-* options into moz.configure and feed them into bindgen_cflags_defaults in build/moz.configure/android-ndk.configure, probably moving that function to a more appropriate place.

Option #1 would take about 30 minutes to write, but is a bit of a step backward for the build system.  Option #2 might take...a week or so?...and carries a significant amount of risk from changing a lot of moving parts.  Neither one is particularly easy to test, since we don't have any automation for this.  Perhaps everybody who is reporting this bug would be happy to test preliminary patches, though?

Chris, I think you've been converting things to moz.configure; have moving any of the aforementioned options into moz.configure come up?  WDYT about adopting Option #1 above as a hacky workaround to let downstream distributors build more conveniently?

Comment 31

[Chris Manchester (limited bugmail, email directly)]

Similarly expedient might be to add a new (old-)configure variable with "system includes" in the name that combines the flags we care about as in http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/python/mozbuild/mozbuild/frontend/context.py#414 and add that to bindgen.toml.in (as well as context.py if we like).

Comment 32

[Nathan Froyd [:froydnj]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #29)
> The workaround is using BINDGEN_CFLAGS in .mozconfig pointing to the
> relevant include directories.

Discovered while trying to implement comment 31 that this is actually broken!  So great.

Comment 33

[Nathan Froyd [:froydnj]]

Attachment: experimental patch that seems to pass configure

Something like this is what's necessary.  Interested to hear whether this solves everybody's problems, or whether more hacks are needed.

Comment 34

[Landry Breuil (:gaston)]

wolfir: here's what im using right now for OpenBSD: https://cgit.rhaalovely.net/mozilla-firefox/tree/patches/patch-layout_style_bindgen_toml_in?h=beta

Which is clearly a ugly hack, but allows me to enable/ship stylo. Will try to test att 8926587 asap.

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 8926587 [details] [diff] [review]
experimental patch that seems to pass configure

This fixes bug 1394697.  Thank you!
I couldn't test nss because Firefox requires a version that does not exist.
I couldn't test nspr because bug 1341222.

% pkg-config --cflags pixman-1
-I/usr/include/pixman-1

Comment 36

[Jan Beich]

Comment on attachment 8926587 [details] [diff] [review]
experimental patch that seems to pass configure

With the patch applied both system Pixman and NSPR work fine even without BINDGEN_CFLAGS in .mozconfig.

$ fgrep -i bindgen objdir/config/autoconf.mk
BINDGEN_SYSTEM_FLAGS = '-I/usr/local/include/nspr', '-I/usr/local/include/nss', '-I/usr/local/include/nss/nss', '-I/usr/local/include/pixman-1', '-Iobjdir/dist/include/cairo'
MOZ_STYLO_BINDGEN = 1

build log: https://ptpb.pw/vm9g

Comment 37

[Nathan Froyd [:froydnj]]

Attachment: part 0 - fix typo in AC_SUBST_SET checking

The various AC_SUBST macros generate AC_SUBST_*FOO macros for holding the
values to substitute.  The macros also cross-check the AC_SUBST_* macros
generated by other variants to make sure that you don't try to do
something like AC_SUBST(FOO) and AC_SUBST_SET(FOO).  However, the check
in AC_SUBST_SET for AC_SUBST_LIST duplicate is missing an underscore:
the AC_SUBST_LIST macro generates another macro starting with
AC_SUBST_LIST_, but the AC_SUBST_SET macro checks for the prefix
AC_SUBST_LIST, which is missing the trailing underscore.

As we're going to be adding yet another AC_SUBST_* macro variant, and
therefore adding more checks to all existing macros, let's clean this up
before we start.

Comment 38

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add AC_SUBST_TOML_LIST macro

Stylo's bindgen is configured partially through a .toml.in file that
substitutes the value of a configure variable (BINDGEN_CFLAGS) into a
TOML list.  We can debate whether this is a good thing to do some other
time; the reality is that the current moz.configure code that provides
the set_config for BINDGEN_CFLAGS needs to perform all the quoting
itself.

We want, however, to define the substituted variable in old-configure.in
land (some of the values that will go into BINDGEN_CFLAGS are only
defined in old-configure.in, and are not trivially ported to
moz.configure), which means that we need to have quoting logic in
m4/Python when we generate config.status.  This patch adds an
appropriate macro for doing so.

Comment 39

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add an intermediate step for determining bindgen's CFLAGS

Add an intermediate step in old-configure.in for setting up
BINDGEN_CFLAGS (renamed to BINDGEN_SYSTEM_FLAGS), so we can add whatever
flags we like (e.g. for system libaries with their includes in
non-standard places) at a later point.

Comment 40

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add various system library CFLAGS to BINDGEN_SYSTEM_FLAGS

This change ensures that folks who configure --with-system-FOO for
various values of FOO can build Stylo, since bindgen will know where to
find the flags for said FOO packages.

The libraries added here might be a little excessive, or they might not be
excessive enough; I'm not really sure.  I've added the ones reported in this
bug and elsewhere, and thrown in a few others for good measure.

Comment 41

[Landry Breuil (:gaston)]

With the patch backported on 57.0rc1, it failed to pass configure, so i suppose something is missing for this in beta/release:

  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/configure.py", line 124, in <module>
    sys.exit(main(sys.argv))
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/configure.py", line 29, in main
    sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure'))
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/configure/__init__.py", line 427, in run
    func(*args)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/configure/__init__.py", line 473, in _value_for
    return self._value_for_depends(obj, need_help_dependency)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/util.py", line 925, in method_call
    cache[args] = self.func(instance, *args)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/configure/__init__.py", line 482, in _value_for_depends
    return obj.result(need_help_dependency)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/util.py", line 925, in method_call
    cache[args] = self.func(instance, *args)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/configure/__init__.py", line 122, in result
    return self._func(*resolved_args)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/python/mozbuild/mozbuild/configure/__init__.py", line 1001, in wrapped
    return new_func(*args, **kwargs)
  File "/usr/obj/ports/firefox-57.0rc1/firefox-57.0/build/moz.configure/old.configure", line 365, in old_configure
    exec code in raw_config
  File "config.data", line 444, in <module>

    (''' BINDGEN_SYSTEM_FLAGS ''', r''' %s ''' % str(', '.join("'%s'" % s for s in split(r'''  -I/usr/local/include/nspr -I/usr/local/include/nss -I/usr/X11R6/include/pixman-1 -I/usr/local/include/cairo -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include -I/usr/local/include -I/usr/X11R6/include/pixman-1 -I/usr/X11R6/include -I/usr/X11R6/include/freetype2 -I/usr/local/include/libpng16 -I/usr/local/include/cairo -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include -I/usr/local/include -I/usr/X11R6/include/pixman-1 -I/usr/X11R6/include -I/usr/X11R6/include/freetype2 -I/usr/local/include/libpng16 ''')))),
NameError: name 'split' is not defined

Comment 42

[Nathan Froyd [:froydnj]]

(In reply to Landry Breuil (:gaston) from comment #41)
> With the patch backported on 57.0rc1, it failed to pass configure, so i
> suppose something is missing for this in beta/release:

What version of Python are you using?

I don't understand how you can get errors with this patch and not get errors normally, given that the global name `split` is already used in config.status.m4...

Comment 43

[Wolfgang Rosenauer [:wolfiR]]

I have exactly the same issue with the patch fwiw. As a base I'm using FIREFOX_57_0b14_RELEASE from mozilla-beta tree.

Comment 44

[Landry Breuil (:gaston)]

(In reply to Nathan Froyd [:froydnj] from comment #42)
> (In reply to Landry Breuil (:gaston) from comment #41)
> > With the patch backported on 57.0rc1, it failed to pass configure, so i
> > suppose something is missing for this in beta/release:
> 
> What version of Python are you using?

2.7.14 in that case.

> I don't understand how you can get errors with this patch and not get errors
> normally, given that the global name `split` is already used in
> config.status.m4...

Yeah, im puzzled.. at first i thought this was a rust keyword in a rust version i dont have yet, but then it should be python.. or some wrapping, or unbalanced quoting ?

Note that 'for s in split' is the only occurence of this construct in old-configure, the other uses are always something.split()

ie

(''' MOZ_CAIRO_OSLIBS ''', list(r''' $MOZ_CAIRO_OSLIBS '''.split()))

vs

(''' BINDGEN_SYSTEM_FLAGS ''', r''' %s ''' % str(', '.join("'%s'" % s for s in split(r''' $BINDGEN_SYSTEM_FLAGS '''))))

Comment 45

[Nathan Froyd [:froydnj]]

Uh, ok, weird.  What Python version and what shells are you using for /bin/sh?

Comment 46

[Landry Breuil (:gaston)]

sh is openbsd's pdksh here. Maybe that's a weird m4 construct our m4 doesnt grok, and somehow gnu m4 isnt enforced ?

Comment 47

[Nathan Froyd [:froydnj]]

Oh, I see.  config.status.m4 got a little love in bug 1403346, and presumably we have `split` in the global namespace now.  So the patch would have to be a little different for beta vs. trunk. =/

Maybe the relevant line in config.status.m4 should be:

    (''' $1 ''', r''' %s ''' % str(', '.join("'%s'" % s for s in r''' [$]$1 '''.split())))

Can you try that instead?

Comment 48

[Landry Breuil (:gaston)]

That passes configure fine when applied on 57.0 build1 - will let it build overnight but that looks promising :)

config.status:    'BINDGEN_SYSTEM_FLAGS': '\'-I/usr/local/include/nspr\', \'-I/usr/local/include/nss\', \'-I/usr/X11R6/include/pixman-1\', \'-I/usr/local/include/cairo\', \'-I/usr/local/include/glib-2.0\', \'-I/usr/local/lib/glib-2.0/include\', \'-I/usr/local/include\', \'-I/usr/X11R6/include/pixman-1\', \'-I/usr/X11R6/include\', \'-I/usr/X11R6/include/freetype2\', \'-I/usr/local/include/libpng16\', \'-I/usr/local/include/cairo\', \'-I/usr/local/include/glib-2.0\', \'-I/usr/local/lib/glib-2.0/include\', \'-I/usr/local/include\', \'-I/usr/X11R6/include/pixman-1\', \'-I/usr/X11R6/include\', \'-I/usr/X11R6/include/freetype2\', \'-I/usr/local/include/libpng16\'',

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8926912 [details] [diff] [review]
part 2 - add an intermediate step for determining bindgen's CFLAGS

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

::: build/moz.configure/toolchain.configure
@@ +1074,3 @@
>  def bindgen_cflags(value):
>      if value and len(value):
>          # Reformat the env value for substitution into a toml list.

This comment is no longer valid.

@@ +1074,5 @@
>  def bindgen_cflags(value):
>      if value and len(value):
>          # Reformat the env value for substitution into a toml list.
>          flags = value[0].split()
> +        return flags

Skip the intermediate variable?

Comment 50

[Chris Peterson [:cpeterson]]

Unfortunately, release management says it is too late to uplift a bindgen fix to Firefox 57. Instead, we can provide downstream maintainers with a rollup patch consolidating all the build script fixes on this bug.

Comment 51

[Landry Breuil (:gaston)]

(In reply to Chris Peterson [:cpeterson] from comment #50)
> Unfortunately, release management says it is too late to uplift a bindgen
> fix to Firefox 57.

Well, that's a shame, but $reasons. As long as it gets uplifted to 58...

> Instead, we can provide downstream maintainers with a
> rollup patch consolidating all the build script fixes on this bug.

I'll keep my hackish patch for 57 i think :)

Fwiw, the patch itself (with the fix from comment 41) works on 57, just need to whack the autoconf warning between configure and build.

Comment 52

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Chris Peterson [:cpeterson] from comment #50)
> Unfortunately, release management says it is too late to uplift a bindgen
> fix to Firefox 57. Instead, we can provide downstream maintainers with a
> rollup patch consolidating all the build script fixes on this bug.

This is perfectly fine for me here. I also was able to build with the last change about the split.

But there is something weird. While the build logs show that stylo is enabled and also some stylo build logs the about:support page still tells me:
false (deaktiviert durch Build) (disabled by or via build)

So any hints how I can check if stylo is finally used and if not why?

The build logfile is here: https://build.opensuse.org/build/mozilla:beta/openSUSE_Leap_42.3/x86_64/firefox/_log

Comment 53

[Jan Beich]

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #52)
> While the build logs show that stylo is enabled...

Does -DMOZ_STYLO=1 show up in build logs? My feedback+ here only covers FF58, I didn't check FF57 in case it silently disables Stylo for some reason.

> https://build.opensuse.org/build/mozilla:beta/openSUSE_Leap_42.3/x86_64/firefox/_log

anonymous_user(Anonymous user is not allowed here - please login):

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1721ccb0d0d6
part 0 - fix typo in AC_SUBST_SET checking; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5adaaf6156
part 1 - add AC_SUBST_TOML_LIST macro; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/16278cfafaa9
part 2 - add an intermediate step for determining bindgen's CFLAGS; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/5471d0611032
part 3 - add various system library CFLAGS to BINDGEN_SYSTEM_FLAGS; r=gps

Comment 55

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Jan Beich from comment #53)
> Does -DMOZ_STYLO=1 show up in build logs? My feedback+ here only covers
> FF58, I didn't check FF57 in case it silently disables Stylo for some reason.

-DMOZ_STYLO=1 -DMOZ_STYLO_ENABLE=1 is part of all the compile commandlines
 
> > https://build.opensuse.org/build/mozilla:beta/openSUSE_Leap_42.3/x86_64/firefox/_log
> 
> anonymous_user(Anonymous user is not allowed here - please login):

sorry

Comment 56

[Chris Peterson [:cpeterson]]

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #52)
> But there is something weird. While the build logs show that stylo is
> enabled and also some stylo build logs the about:support page still tells me:
> false (deaktiviert durch Build) (disabled by or via build)
...
> -DMOZ_STYLO=1 -DMOZ_STYLO_ENABLE=1 is part of all the compile commandlines

Maybe you need to define MOZ_STYLO_BINDGEN too?


> So any hints how I can check if stylo is finally used and if not why?

The following website can detect if your Firefox is using Stylo (by detecting some features that are supported in Stylo but not Gecko's old style system). Note that this website only produces meaningful results in Firefox. :)

https://arewestyloyet.rs/

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1721ccb0d0d6
https://hg.mozilla.org/mozilla-central/rev/9b5adaaf6156
https://hg.mozilla.org/mozilla-central/rev/16278cfafaa9
https://hg.mozilla.org/mozilla-central/rev/5471d0611032

Comment 58

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Chris Peterson [:cpeterson] from comment #56)
> > So any hints how I can check if stylo is finally used and if not why?
> 
> The following website can detect if your Firefox is using Stylo (by
> detecting some features that are supported in Stylo but not Gecko's old
> style system). Note that this website only produces meaningful results in
> Firefox. :)
> 
> https://arewestyloyet.rs/

This page is telling me that I'm using stylo. So something is fishy for me about the about:support page which still tells me "false" but this seems to be OT for here then.

Comment 59

[tt_1]

Is there any other page around to test for stylo? arewestyloyet.rs is pretty much down, it seems.