1364428 - various prerequisites for building stylo by default

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Bobby Holley (:bholley)]

Splitting some stuff out of bug 1356991 that we can land before pulling the trigger.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add clang to more tooltool manifests. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Don't use LLVM_CONFIG in non-stylo builds. v1

By defining it in mozconfig.common, we're currently activating this
check on android, which is --disable-stylo and doesn't have llvm.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Pass LLVM_CONFIG everywhere. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Disable stylo for static analysis builds that use llvm38. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Disable packaging of stylo buidings for artifact builds. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Add a separate build-time option to enable stylo by default. v1

Eventually we should just use the same builds for our CI stylo jobs,
but that involves more TC wrangling than I want to do right now.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove the special logic to force mozjemalloc on stylo builds. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 8 - Pass bindir instead of libdir to llvm-config on windows. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 9 - Remove the special stylo #define in mozjemalloc.c. v1

Glandium may also do this when he finishes the jemalloc stuff, but
including it in the patch stack for completeness.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8867248 [details] [diff] [review]
Part 2 - Don't use LLVM_CONFIG in non-stylo builds. v1

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

With bug 1316956, you can use when='--enable-stylo' on check_prog directly.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8867256 [details] [diff] [review]
Part 9 - Remove the special stylo #define in mozjemalloc.c. v1

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

This is part of bu 1361258 and needs to land after the servo part of the bug.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8867253 [details] [diff] [review]
Part 7 - Remove the special logic to force mozjemalloc on stylo builds. v1

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

This is removed in bug 1363992

Comment 13

[Nathan Froyd [:froydnj]]

OK, so figured out the linux32 build problems with executing elfhack (bug 1356991 comment 13 and following).  With these patches, on linux32, we now have a situation where clang and gcc are simultaneously installed on a 32-bit system, which we don't have currently.  (We do not run static analysis builds on 32-bit, for some reason, possibly this one?)  That means we run into problems here:

http://dxr.mozilla.org/mozilla-central/source/build/unix/mozconfig.stdcxx#6

because we only insert the clang path into LD_LIBRARY_PATH, and not the gcc paths that we depended on before.  But our clang packages only contain a 64-bit libstdc++.  So during the build, we build with gcc, link against gcc's libstdc++, but when we go to run said binaries, LD_LIBRARY_PATH doesn't contain the libstdc++ that we linked against.  We check the libstdc++ from clang (64-bit only) and the system libstdc++ (wrong version), and fall over.

Options:

1) Don't build Stylo on Linux 32-bit.
2) Unconditionally add both clang and gcc libstdc++ paths to LD_LIBRARY_PATH, instead of preferring the clang one like we do today.  I think this results in problems if for whatever reason, our clang and gcc packages get out of sync, as is easy to do.  Out-of-sync-ness will be less of a problem once everything is fetched from taskcluster, I think....
3) Build clang packages with 32-bit and 64-bit libstdc++.so included in them, and change the above file to point to both.

glandium, what do you think is the right way forward here?

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8867248 [details] [diff] [review]
Part 2 - Don't use LLVM_CONFIG in non-stylo builds. v1

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

I think the better way forward here is to just not set LLVM_CONFIG in mozconfigs if it doesn't exist.  But I'm willing to hear arguments as to why we should use when='--enable-stylo' or similar.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8867249 [details] [diff] [review]
Part 3 - Pass LLVM_CONFIG everywhere. v1

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

This should check for llvm-config first, something like:

https://hg.mozilla.org/try/rev/ff66512dfa689a82183212ba8c907d50610bc7d9

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8867255 [details] [diff] [review]
Part 8 - Pass bindir instead of libdir to llvm-config on windows. v1

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

::: toolkit/moz.configure
@@ +691,5 @@
>              return None
>  
>          check_minimum_llvm_config_version(llvm_config)
>          return namespace(
> +                libclang_path=invoke_llvm_config(llvm_config, '--bindir' if host.os == 'WINNT' else '--libdir'),

What is up with the indentation here?  It looks like this line got indented too far.

Please fix the documentation at:

http://searchfox.org/mozilla-central/source/python/mozboot/mozboot/windows.py#18
http://searchfox.org/mozilla-central/source/python/mozboot/mozboot/mozillabuild.py#20

as well.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8867252 [details] [diff] [review]
Part 6  - Add a separate build-time option to enable stylo by default. v1

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

This is just a temporary patch until --enable-stylo becomes the default, right?  Otherwise, I'm not sure what this is buying us.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8867252 [details] [diff] [review]
Part 6  - Add a separate build-time option to enable stylo by default. v1

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

::: toolkit/moz.configure
@@ +622,5 @@
> +# builds as regular linux64.
> +option(env='MOZ_STYLO_ENABLED_BY_DEFAULT',
> +       help='Enable the servo style system by default at runtime.')
> +set_define('MOZ_STYLO_ENABLED_BY_DEFAULT',
> +           depends_if('MOZ_STYLO_ENABLED_BY_DEFAULT')(lambda x: True))

Could we copy the `--enable-webrender` option instead, where we set the default to `--enable-webrender=build` on nightly, and allow `--enable-webrender` for "build and enable"?

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/toolkit/moz.configure#747

Comment 19

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8867248 [details] [diff] [review]
> Part 2 - Don't use LLVM_CONFIG in non-stylo builds. v1
> 
> Review of attachment 8867248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the better way forward here is to just not set LLVM_CONFIG in
> mozconfigs if it doesn't exist.  But I'm willing to hear arguments as to why
> we should use when='--enable-stylo' or similar.

Because we shouldn't run configure tests when they're not necessary.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> Comment on attachment 8867252 [details] [diff] [review]
> Part 6  - Add a separate build-time option to enable stylo by default. v1
> 
> Review of attachment 8867252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is just a temporary patch until --enable-stylo becomes the default,
> right?  Otherwise, I'm not sure what this is buying us.

This buys us the ability to have the existing linux64-stylo jobs continue to do what they've been doing (test stylo with the pref enabled), while also enabling MOZ_STYLO on all platforms. Currently MOZ_STYLO also controls the default value of the stylo pref, which we would have to change before enabling MOZ_STYLO everywhere. But there isn't another way (that I know of) to set a pref from a .mozconfig file, so we need to do this to make the linux64-stylo platform continue to work.

Eventually we should make them the same platform, share the builds, and just have separate test jobs, but that requires messing around with TaskCluster configs, and I'd rather defer that until later.

Comment 21

[Bobby Holley (:bholley)]

Nathan, given that you're working on this stuff, I assume you'll push the patches here with whatever fixups make sense. Let me know if there's anything you want me to do.

Comment 22

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Nathan Froyd [:froydnj] from comment #14)
> > I think the better way forward here is to just not set LLVM_CONFIG in
> > mozconfigs if it doesn't exist.  But I'm willing to hear arguments as to why
> > we should use when='--enable-stylo' or similar.
> 
> Because we shouldn't run configure tests when they're not necessary.

This sounds kind of cryptic...aren't we effectively not running the configure test by not setting LLVM_CONFIG?

Re-ni?'ing for comment 13 and for this.  My current approach is to just give priority to the gcc libstdc++, if it exists, by switching the ordering of the tests.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #22)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > (In reply to Nathan Froyd [:froydnj] from comment #14)
> > > I think the better way forward here is to just not set LLVM_CONFIG in
> > > mozconfigs if it doesn't exist.  But I'm willing to hear arguments as to why
> > > we should use when='--enable-stylo' or similar.
> > 
> > Because we shouldn't run configure tests when they're not necessary.
> 
> This sounds kind of cryptic...aren't we effectively not running the
> configure test by not setting LLVM_CONFIG?

One could have LLVM_CONFIG set for other reasons (Firefox is not the only thing using that environment variable). Or one could have it set for full builds, and be trying to do an artifact build, which would still do the llvm check just because the variable is set.

> Re-ni?'ing for comment 13 and for this.  My current approach is to just give
> priority to the gcc libstdc++, if it exists, by switching the ordering of
> the tests.

That's already done in bug 1365182

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 8867248 [details] [diff] [review]
Part 2 - Don't use LLVM_CONFIG in non-stylo builds. v1

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

This is OK, but I'm going to edit this to add a when='...' argument as suggested.

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8867249 [details] [diff] [review]
Part 3 - Pass LLVM_CONFIG everywhere. v1

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

Once we have the appropriate when= condition on check_prog, this setup will be fine.

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: make stylo enabling more flexible

We currently have an --enable-stylo option, which when passed builds
Stylo and enables Stylo at runtime.  With an upcoming move to building
Stylo everywhere by default, but only enabling it on specific platforms,
we need something more sophisticated than a binary yes/no.

The recent WebRender support offers a model worth emulating: we modify
things so there are four possibilities:

* nothing passed (the default);
* --disable-stylo (explicitly not building);
* --enable-stylo=build (build, but do not enable by default);
* --enable-stylo (build and enable)

The last option corresponds exactly to what we have today, so there's no
change in functionality.  This patch makes the default and
--disable-stylo the same; splitting the default and --disable-stylo into
separate cases enables us to change the default behavior at some point
in the future.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: Don't use LLVM_CONFIG in non-stylo builds

We'll want to define LLVM_CONFIG in mozconfig.common for building Stylo
by default.  But if we do that, builds where we don't have an
appropriate clang available will complain that they can't find
llvm-config.  So we should only check if we're building Stylo.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: Pass LLVM_CONFIG everywhere

Comment 29

[Nathan Froyd [:froydnj]]

Attachment: Add a separate define for enabling stylo

This completes the separation between "build stylo" and "enable stylo"
that we started in the build system a few commits ago.

Comment 30

[Nathan Froyd [:froydnj]]

Attachment: use normsep for windows bindgen paths

bindgen, for whatever reason, is much happier with C:/path/to/file
than "normal" Windows paths.

Comment 31

[Ralph Giles (:rillian)]

Comment on attachment 8869069 [details] [diff] [review]
make stylo enabling more flexible

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

::: toolkit/moz.configure
@@ +617,5 @@
> +
> +@depends('--enable-stylo')
> +def stylo_config(value):
> +    build_stylo = None
> +    enable_stylo = None

Should these default to `False` instead of `None`?

Comment 32

[Ralph Giles (:rillian)]

Comment on attachment 8869072 [details] [diff] [review]
Add a separate define for enabling stylo

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

::: toolkit/moz.configure
@@ +736,5 @@
>      )
>  
>  set_config('MOZ_STYLO', delayed_getattr(stylo, 'build'))
>  set_define('MOZ_STYLO', delayed_getattr(stylo, 'build'))
> +set_define('MOZ_STYLO_ENABLE', delayed_getattr(stylo_config, 'enable'))

MOZ_STYLO[_*] is a completing naming convention here to MOZ_BUILD_WEBRENDER, MOZ_ENABLE_WEBRENDER. I don't feel strongly but wanted to point it. MOZ_${VERB}_${NOUN} seems to me more common in this file than MOZ_${CATEGORY}_${DETAIL}.

Comment 33

[Ralph Giles (:rillian)]

Comment on attachment 8869073 [details] [diff] [review]
use normsep for windows bindgen paths

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

Please include an actual error this fixes in the commit message. Your 'why' section here doesn't really add anything other than describing what `normsep` does.

Comment 34

[Nathan Froyd [:froydnj]]

Attachment: disable stylo for valgrind builds

Valgrind builds a) somehow run Stylo code despite it not being enabled and b)
find valgrind errors in said Stylo code.  Preliminary discussion in bug 1365915
indicates that this might be tricky to fix, so let's just disable Stylo
entirely for such builds for now.

Comment 35

[Ralph Giles (:rillian)]

Comment on attachment 8869178 [details] [diff] [review]
disable stylo for valgrind builds

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

Seems reasonable.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dfe645a3845
Add clang to more tooltool manifests; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c078e357f
make stylo enabling more flexible; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdab12ab8c24
Don't use LLVM_CONFIG in non-stylo builds; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/18ab60546638
Pass LLVM_CONFIG everywhere; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/399852d9c94d
Disable stylo for static analysis builds that use llvm38; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/44134a6b2b6a
Disable packaging of stylo buidings for artifact builds; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c779485d546
Add a separate define for enabling stylo; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/40d03699e48f
Pass bindir instead of libdir to llvm-config on windows; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9f8eb96ef3
use normsep for windows bindgen paths; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/a11eda203014
disable stylo for valgrind builds; r=rillian

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2dfe645a3845
https://hg.mozilla.org/mozilla-central/rev/bc1c078e357f
https://hg.mozilla.org/mozilla-central/rev/cdab12ab8c24
https://hg.mozilla.org/mozilla-central/rev/18ab60546638
https://hg.mozilla.org/mozilla-central/rev/399852d9c94d
https://hg.mozilla.org/mozilla-central/rev/44134a6b2b6a
https://hg.mozilla.org/mozilla-central/rev/3c779485d546
https://hg.mozilla.org/mozilla-central/rev/40d03699e48f
https://hg.mozilla.org/mozilla-central/rev/ff9f8eb96ef3
https://hg.mozilla.org/mozilla-central/rev/a11eda203014