various prerequisites for building stylo by default

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bholley, Assigned: froydnj)

Tracking

unspecified
mozilla55

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(10 attachments, 5 obsolete attachments)

4.01 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.80 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.09 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.96 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
5.80 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.40 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.62 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.16 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.46 KB, patch
rillian
: review+
Details | Diff | Splinter Review
884 bytes, patch
rillian
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Splitting some stuff out of bug 1356991 that we can land before pulling the trigger.
(Reporter)

Comment 1

2 years ago
Created attachment 8867247 [details] [diff] [review]
Part 1 - Add clang to more tooltool manifests. v1
Attachment #8867247 - Flags: review?(nfroyd)
(Reporter)

Comment 2

2 years ago
Created attachment 8867248 [details] [diff] [review]
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.
Attachment #8867248 - Flags: review?(nfroyd)
(Reporter)

Comment 3

2 years ago
Created attachment 8867249 [details] [diff] [review]
Part 3 - Pass LLVM_CONFIG everywhere. v1
Attachment #8867249 - Flags: review?(nfroyd)
(Reporter)

Comment 4

2 years ago
Created attachment 8867250 [details] [diff] [review]
Part 4 - Disable stylo for static analysis builds that use llvm38. v1
Attachment #8867250 - Flags: review?(nfroyd)
(Reporter)

Comment 5

2 years ago
Created attachment 8867251 [details] [diff] [review]
Part 5 - Disable packaging of stylo buidings for artifact builds. v1
Attachment #8867251 - Flags: review?(nfroyd)
(Reporter)

Comment 6

2 years ago
Created attachment 8867252 [details] [diff] [review]
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.
Attachment #8867252 - Flags: review?(nfroyd)
(Reporter)

Comment 7

2 years ago
Created attachment 8867253 [details] [diff] [review]
Part 7 - Remove the special logic to force mozjemalloc on stylo builds. v1
Attachment #8867253 - Flags: review?(nfroyd)
(Reporter)

Comment 8

2 years ago
Created attachment 8867255 [details] [diff] [review]
Part 8 - Pass bindir instead of libdir to llvm-config on windows. v1
Attachment #8867255 - Flags: review?(nfroyd)
(Reporter)

Comment 9

2 years ago
Created attachment 8867256 [details] [diff] [review]
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.
Attachment #8867256 - Flags: review?(nfroyd)
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 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.
Attachment #8867256 - Flags: review?(nfroyd)
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
Attachment #8867253 - Flags: review?(nfroyd)
(Reporter)

Updated

2 years ago
Assignee: bobbyholley → nfroyd
(Assignee)

Comment 13

2 years ago
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?
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

2 years ago
Attachment #8867247 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 14

2 years ago
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.
Attachment #8867248 - Flags: review?(nfroyd)
(Assignee)

Comment 15

2 years ago
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
Attachment #8867249 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Attachment #8867251 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 16

2 years ago
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.
Attachment #8867255 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 17

2 years ago
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.
Attachment #8867252 - Flags: review?(nfroyd)
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
(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.
Flags: needinfo?(mh+mozilla)
Attachment #8867253 - Attachment is obsolete: true
Attachment #8867256 - Attachment is obsolete: true
(Reporter)

Comment 20

2 years ago
(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.
(Reporter)

Comment 21

2 years ago
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.
(Assignee)

Comment 22

2 years ago
(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.
Flags: needinfo?(mh+mozilla)
(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
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 24

2 years ago
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.
Attachment #8867248 - Flags: review+
(Assignee)

Comment 25

2 years ago
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.
Attachment #8867249 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8867250 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 26

2 years ago
Created attachment 8869069 [details] [diff] [review]
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.
Attachment #8869069 - Flags: review?(giles)
(Assignee)

Updated

2 years ago
Attachment #8867252 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
Created attachment 8869070 [details] [diff] [review]
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.
Attachment #8869070 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8867248 - Attachment is obsolete: true
(Assignee)

Comment 28

2 years ago
Created attachment 8869071 [details] [diff] [review]
Pass LLVM_CONFIG everywhere
Attachment #8869071 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8867249 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8869072 [details] [diff] [review]
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.
Attachment #8869072 - Flags: review?(giles)
(Assignee)

Comment 30

2 years ago
Created attachment 8869073 [details] [diff] [review]
use normsep for windows bindgen paths

bindgen, for whatever reason, is much happier with C:/path/to/file
than "normal" Windows paths.
Attachment #8869073 - Flags: review?(giles)
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`?
Attachment #8869069 - Flags: review?(giles) → review+
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}.
Attachment #8869072 - Flags: review?(giles) → review+
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.
Attachment #8869073 - Flags: review?(giles) → review+
(Assignee)

Comment 34

2 years ago
Created attachment 8869178 [details] [diff] [review]
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.
Attachment #8869178 - Flags: review?(giles)
Comment on attachment 8869178 [details] [diff] [review]
disable stylo for valgrind builds

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

Seems reasonable.
Attachment #8869178 - Flags: review?(giles) → review+

Comment 36

2 years ago
Pushed by nfroyd@mozilla.com:
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

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.