Closed Bug 1356991 (stylo-nightly-build) Opened 2 years ago Closed 2 years ago

Build Stylo in Nightly (always #define MOZ_STYLO but keep Stylo pref'd off by default)

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: cpeterson, Assigned: froydnj)

References

Details

(Whiteboard: [stylo])

Attachments

(1 file, 1 obsolete file)

We want to #define MOZ_STYLO for desktop platforms by default so Stylo code is built and shipped with Nightly, but pref'd off by default.
Note that we'll need to sort out our jemalloc story first. I promised glandium we wouldn't flip this #define until that was sorted out. :-)
Depends on: stylo-jemalloc
No longer blocks: 1356988
Duplicate of this bug: 1356988
Alias: stylo-define-MOZ_STYLO
Priority: -- → P1
Alias: stylo-define-MOZ_STYLO → stylo-nightly-build
Alias: stylo-nightly-build → stylo-nightly-pref
Summary: stylo: always #define MOZ_STYLO on desktop platforms (but keep Stylo pref'd off by default) → stylo: always #define MOZ_STYLO on Nightly desktop platforms (but keep Stylo pref'd off by default)
Summary: stylo: always #define MOZ_STYLO on Nightly desktop platforms (but keep Stylo pref'd off by default) → always #define MOZ_STYLO on Nightly desktop platforms (but keep Stylo pref'd off by default)
Depends on: 1310852
Depends on: 1357889
Depends on: 1314355
Alias: stylo-nightly-pref → stylo-nightly-build
Summary: always #define MOZ_STYLO on Nightly desktop platforms (but keep Stylo pref'd off by default) → Build Stylo in Nightly (always #define MOZ_STYLO but keep Stylo pref'd off by default)
Whiteboard: [stylo]
Depends on: 1359508
Depends on: 1363375
No longer depends on: 1341234
Bobby, do you want to prepare this patch to actually #define MOZ_STYLO by default? It would be good to have this patch in hand, so we can land it ASAP after the blocking bugs are resolved.
Flags: needinfo?(bobbyholley)
Depends on: 1361258
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Hm, need to pass llvm-config. Trying with
> https://pastebin.mozilla.org/9021495
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=069bf3d4590d3d57dd28575add44e25d06b62dd4

Hm, still doesn't work.
Flags: needinfo?(bobbyholley)
Depends on: 1364428
Attachment #8867257 - Flags: review?(nfroyd)
There's still more to do here, but I'm out of time and so Nathan said he'd take it over.

Outstanding issues:
* The "Don't use LLVM_CONFIG in non-stylo builds" doesn't quite seem to work, since we still bust on android.
* win32 bindgen busts because of an unknown calling convention. We don't actually need any bindgen-assisted C++ calls for stylo.
* linux32 bindgen triggers some static assertions in the JS engine. Presumably we're not passing the right defines.
* the "Pass bindir instead of libdir to llvm-config on windows" patch doesn't seem to work, win64 is still panicking with Unable to find libclang". See also bug 1363655.
* Some reftests seem to fail, probably because we have some code somewhere that depends on MOZ_STYLO instead of the pref. I can look at this, need more help on the build stuff.
* Possible some other as-yet-undiscovered stuff.

My most recent try push with these patches is https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a6e92ec22176c4a2421aa34e84dfc27c992dd2

Thanks Nathan!
Flags: needinfo?(nfroyd)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> * The "Don't use LLVM_CONFIG in non-stylo builds" doesn't quite seem to
> work, since we still bust on android.

The easiest way to deal with this is to test for the existence of llvm-config in the mozconfig and then set LLVM_CONFIG only if it exists. :)

> * linux32 bindgen triggers some static assertions in the JS engine.
> Presumably we're not passing the right defines.

This is servo/servo#16843.

> * win32 bindgen busts because of an unknown calling convention. We don't
> actually need any bindgen-assisted C++ calls for stylo.
> * the "Pass bindir instead of libdir to llvm-config on windows" patch
> doesn't seem to work, win64 is still panicking with Unable to find
> libclang". See also bug 1363655.

I'm not entirely sure what's up with this.  Something about my patch to conditionally assign LLVM_CONFIG is causing the necessary variables to *not* get passed through to cargo, so I can't reproduce the original problem.  The original patches should have worked, though...
Flags: needinfo?(nfroyd)
Also, linux32 is not completely functional, even with the servo patch, because something causes elfhack to not run correctly.  I think this is because g++ is picking up the libstdc++ that comes with clang (?!), even though I have no idea how that can happen.
(In reply to Nathan Froyd [:froydnj] from comment #13)
> Also, linux32 is not completely functional, even with the servo patch,
> because something causes elfhack to not run correctly.  I think this is
> because g++ is picking up the libstdc++ that comes with clang (?!), even
> though I have no idea how that can happen.

Stranger and stranger:

* The g++ invocations for linking elfhack are identical for patched and unpatched.
* The actual linker invocations done by g++ for linking elfhack are identical for patched and unpatched.
* The patched version, however, links against the *system* libstdc++ (and can't be executed) whereas the unpatched version links against our g++'s libstdc++ and can be executed.
Assignee: nobody → nfroyd
Depends on: 1364838
Depends on: 1337667
Depends on: 1364863
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> * Some reftests seem to fail, probably because we have some code somewhere
> that depends on MOZ_STYLO instead of the pref. I can look at this, need more
> help on the build stuff.

This is bug 1364863. Got a patch.
(In reply to Nathan Froyd [:froydnj] from comment #14)
> (In reply to Nathan Froyd [:froydnj] from comment #13)
> > Also, linux32 is not completely functional, even with the servo patch,
> > because something causes elfhack to not run correctly.  I think this is
> > because g++ is picking up the libstdc++ that comes with clang (?!), even
> > though I have no idea how that can happen.
> 
> Stranger and stranger:
> 
> * The g++ invocations for linking elfhack are identical for patched and
> unpatched.
> * The actual linker invocations done by g++ for linking elfhack are
> identical for patched and unpatched.
> * The patched version, however, links against the *system* libstdc++ (and
> can't be executed) whereas the unpatched version links against our g++'s
> libstdc++ and can be executed.

Seems like this could be responsible for it:
https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/build/unix/mozconfig.stdcxx#6-7

And it should only be a matter of making the gcc check first (presumably, if we pull gcc, it's to build with it, so we should prefer that).

Filed bug 1365182.
(In reply to Nathan Froyd [:froydnj] from comment #12)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > * linux32 bindgen triggers some static assertions in the JS engine.
> > Presumably we're not passing the right defines.
> 
> This is servo/servo#16843.

...or it would be, if I actually understood cross-compilation.  What we really need to do is something like:

https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668

but that still doesn't fix things, because clang is picking up 64-bit headers for reasons I don't yet understand.  The 32-bit headers *are* there and we are, correctly, requesting 32-bit compilation.

> > * win32 bindgen busts because of an unknown calling convention. We don't
> > actually need any bindgen-assisted C++ calls for stylo.

We don't, but it would be good to at least have an accurate representation of calling conventions.  This is servo/rust-bindgen#541; see my most recent comment there, which means we have some work lower down in the stack to do before bindgen can understand this.

> > * the "Pass bindir instead of libdir to llvm-config on windows" patch
> > doesn't seem to work, win64 is still panicking with Unable to find
> > libclang". See also bug 1363655.
> 
> I'm not entirely sure what's up with this.  Something about my patch to
> conditionally assign LLVM_CONFIG is causing the necessary variables to *not*
> get passed through to cargo, so I can't reproduce the original problem.  The
> original patches should have worked, though...

This appears to be because the path returned from `llvm-config --bindir` has backslashes; replacing those with forward slashes does the right thing (!).  I don't really understand this; perhaps it has something to do with the environment we're running in.  Fixing the slashes problem just runs into the calling convention issues, though.
(In reply to Nathan Froyd [:froydnj] from comment #17)
> > > * win32 bindgen busts because of an unknown calling convention. We don't
> > > actually need any bindgen-assisted C++ calls for stylo.
> 
> We don't, but it would be good to at least have an accurate representation
> of calling conventions.  This is servo/rust-bindgen#541; see my most recent
> comment there, which means we have some work lower down in the stack to do
> before bindgen can understand this.

Filed rust-lang/rust#42044 for adding support for the thiscall calling convention to rustc, which AFAICT is a necessary prerequisite to using it in bindgen.
(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > > * linux32 bindgen triggers some static assertions in the JS engine.
> > > Presumably we're not passing the right defines.
> > 
> > This is servo/servo#16843.
> 
> ...or it would be, if I actually understood cross-compilation.  What we
> really need to do is something like:
> 
> https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668

The simpler thing to do, which should be done unconditionally for every platform, is to pass --target=$RUST_TARGET to clang. I think all rust targets are understood by clang.
(In reply to Nathan Froyd [:froydnj] from comment #17)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > > * linux32 bindgen triggers some static assertions in the JS engine.
> > > Presumably we're not passing the right defines.
> > 
> > This is servo/servo#16843.
> 
> ...or it would be, if I actually understood cross-compilation.  What we
> really need to do is something like:
> 
> https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668

Maybe we need a similar trick on win32? (see bug 1365254)
(In reply to Mike Hommey [:glandium] from comment #19)
> (In reply to Nathan Froyd [:froydnj] from comment #17)
> > (In reply to Nathan Froyd [:froydnj] from comment #12)
> > ...or it would be, if I actually understood cross-compilation.  What we
> > really need to do is something like:
> > 
> > https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668
> 
> The simpler thing to do, which should be done unconditionally for every
> platform, is to pass --target=$RUST_TARGET to clang. I think all rust
> targets are understood by clang.

This doesn't work for the linux32 case, though we do pass --target for the 32/64-bit MSVC case already.  clang picks up the 64-bit headers, rather than the 32-bit ones, and things do not end well from there:

https://public-artifacts.taskcluster.net/VF27iz1MR5C0EAItXU47MA/0/public/logs/live_backing.log

It looks like it's not even considering the 32-bit libstdc++ headers for some reason.  I *think* this is because it can't find an i686-pc-linux-gnu directory in the libstdc++ include directory(ies).  I don't know whether copying the x86_64-unknown-linux-gnu/32 directory to the appropriate i686-pc-linux-gnu/ in our clang packages would work or not, might try that tomorrow.

Using just -m32 instead of --target at least finds the libstdc++ headers, but fails to consider that it might want to look at the 32-bit multilib headers there:

https://public-artifacts.taskcluster.net/d4L3bVJCQsWSJP42eH05dw/0/public/logs/live_backing.log
(In reply to Masatoshi Kimura [:emk] from comment #20)
> (In reply to Nathan Froyd [:froydnj] from comment #17)
> > (In reply to Nathan Froyd [:froydnj] from comment #12)
> > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > > > * linux32 bindgen triggers some static assertions in the JS engine.
> > > > Presumably we're not passing the right defines.
> > > 
> > > This is servo/servo#16843.
> > 
> > ...or it would be, if I actually understood cross-compilation.  What we
> > really need to do is something like:
> > 
> > https://hg.mozilla.org/try/rev/88ba8160362c5330306df24e2f21cfd97ab2a668
> 
> Maybe we need a similar trick on win32? (see bug 1365254)

Windows probably does require a similar trick, but there's also some fixups to be made for passing environment variables into bindgen.  I will post patches tomorrow.  And even once those fixes are applied, bindgen won't work for win32 because of comment 18.
This is based on top of the patches in bug 1364428, and is somewhat more
straightforward than the previous version.

That reminds me, I need to file bugs about enabling Stylo on the platforms
mentioned in the patch.
Attachment #8869074 - Flags: review?(giles)
Attachment #8867257 - Attachment is obsolete: true
Attachment #8867257 - Flags: review?(nfroyd)
Comment on attachment 8869074 [details] [diff] [review]
Build stylo by default on nightly

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

\o/

Should probably remind dev-platform that they can install llvm with `./mach boostrap` once this in the landing queue.
Attachment #8869074 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #24)
> Should probably remind dev-platform that they can install llvm with `./mach
> boostrap` once this in the landing queue.

Please fix bug 1364406 and 1364458 before landing. Otherwise the |./mach boostrap| instruction will just confuse and frustrate people.
Depends on: 1364406, 1364458
Depends on: 1365937
Depends on: 1365993
Depends on: 1365254
Depends on: 1366050
Depends on: 1366048
Depends on: 1366564
Depends on: 1363655
No longer depends on: stylo-jemalloc
We aren't going to block the Stylo built-but-disabled milestone on win32 and linux32 support.
No longer depends on: 1366048
No longer depends on: 1366050
No longer depends on: 1365254
Depends on: 1366048
Depends on: 1366050
Depends on: 1368163
Depends on: 1353472
Depends on: 1368806
Depends on: 1338651
Depends on: 1361005
Depends on: 1365181
Depends on: 1370571
Depends on: 1373317
No longer depends on: 1338651
Depends on: 1374432
No longer depends on: 1361005
No longer depends on: 1353472
Blocks: 1374748
Depends on: 1374824
Depends on: 1375168
Depends on: 1375507
Depends on: 1375692
Depends on: 1375699
Depends on: 1375774
No longer depends on: 1368806
No longer depends on: 1365993
No longer depends on: 1373317
No longer depends on: 1375507
No longer depends on: 1368163
No longer depends on: 1368083
No longer depends on: 1366564
Depends on: 1375976
No longer depends on: 1370571
No longer depends on: 1366050
No longer blocks: stylo-tooling
Depends on: 1366050
Depends on: 1384258
No longer depends on: 1375774
Depends on: 1384759
Depends on: 1385241
Now that linux32 bug 1366050 has been fixed, we are building Stylo support (pref'd off) for all desktop platforms: linux32, linux64, macOS, win32, and win64! Stylo support for Android will ship in a later release, probably Firefox 58 or 59.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.