Bug 1356991 (stylo-nightly-build)

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

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
3 months ago
19 hours ago

People

(Reporter: cpeterson, Assigned: froydnj)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [stylo])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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: 1291355
(Reporter)

Updated

3 months ago
No longer blocks: 1356988
Duplicate of this bug: 1356988
(Reporter)

Updated

3 months ago
Alias: stylo-define-MOZ_STYLO
Blocks: 1330412
Priority: -- → P1
(Reporter)

Updated

3 months ago
Alias: stylo-define-MOZ_STYLO → stylo-nightly-build
(Reporter)

Updated

3 months ago
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)
Blocks: 1345321
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)
(Reporter)

Updated

3 months ago
Depends on: 1310852
(Reporter)

Updated

3 months ago
Depends on: 1357889
(Reporter)

Updated

3 months ago
Depends on: 1314355
(Reporter)

Updated

3 months ago
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]
(Reporter)

Updated

3 months ago
Depends on: 1359508
Depends on: 1363375
(Reporter)

Updated

3 months ago
No longer depends on: 1341234
(Reporter)

Comment 3

3 months ago
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)
(Reporter)

Updated

3 months ago
Depends on: 1361258
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a7834e4fbd1f96a9fdc20ad431084a4caf62e91
Hm, need to pass llvm-config. Trying with https://pastebin.mozilla.org/9021495

https://treeherder.mozilla.org/#/jobs?repo=try&revision=069bf3d4590d3d57dd28575add44e25d06b62dd4
(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)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd1c1f49ee26c4be4d253ae7cc8a2cc2b4fff4e2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eba5f69cabe244bcc9d8029a95f260dc2e68fbe7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31cd852622457a38d6f1844a84f3ca027ee5d812
Depends on: 1364428
Created attachment 8867257 [details] [diff] [review]
Build stylo by default on nightly. v1
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)
(Assignee)

Comment 12

3 months ago
(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)
(Assignee)

Comment 13

3 months ago
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.
(Assignee)

Comment 14

3 months ago
(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.
(Assignee)

Comment 17

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

Comment 18

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

Comment 21

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

Comment 22

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

Comment 23

2 months ago
Created attachment 8869074 [details] [diff] [review]
Build stylo by default on nightly

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)
(Assignee)

Updated

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

Updated

2 months ago
Depends on: 1365937
(Reporter)

Updated

2 months ago
Depends on: 1365993
(Reporter)

Updated

2 months ago
Depends on: 1365254
(Reporter)

Updated

2 months ago
Depends on: 1366050
(Reporter)

Updated

2 months ago
Depends on: 1366048
(Reporter)

Updated

2 months ago
Depends on: 1366564
(Reporter)

Updated

2 months ago
Depends on: 1363655
No longer depends on: 1291355
(Reporter)

Comment 26

2 months ago
We aren't going to block the Stylo built-but-disabled milestone on win32 and linux32 support.
(Reporter)

Updated

2 months ago
No longer depends on: 1366048
(Reporter)

Updated

2 months ago
No longer depends on: 1366050
(Reporter)

Updated

2 months ago
No longer depends on: 1365254
(Reporter)

Updated

2 months ago
Depends on: 1366048
(Reporter)

Updated

2 months ago
Depends on: 1366050
Depends on: 1368083
(Reporter)

Updated

2 months ago
Depends on: 1368163

Updated

2 months ago
Depends on: 1353472

Updated

2 months ago
Depends on: 1368806
(Reporter)

Updated

2 months ago
Depends on: 1338651
(Reporter)

Updated

2 months ago
Depends on: 1361005

Updated

2 months ago
Depends on: 1365181
(Assignee)

Updated

2 months ago
Depends on: 1370571
(Reporter)

Updated

a month ago
Depends on: 1373317
(Reporter)

Updated

a month ago
No longer depends on: 1338651
(Assignee)

Updated

a month ago
Depends on: 1374432
(Reporter)

Updated

a month ago
No longer depends on: 1361005
(Reporter)

Updated

a month ago
No longer depends on: 1353472
(Reporter)

Updated

a month ago
Blocks: 1374748

Updated

a month ago
Depends on: 1374824
(Assignee)

Updated

a month ago
Depends on: 1375168
(Reporter)

Updated

a month ago
Depends on: 1375507

Updated

a month ago
Depends on: 1375692
(Reporter)

Updated

a month ago
Depends on: 1375699
(Reporter)

Updated

a month ago
Depends on: 1375774
(Reporter)

Updated

a month ago
No longer depends on: 1368806
(Reporter)

Updated

a month ago
No longer depends on: 1365993
(Reporter)

Updated

a month ago
No longer depends on: 1373317
(Reporter)

Updated

a month ago
No longer depends on: 1375507
(Reporter)

Updated

a month ago
No longer depends on: 1368163
(Reporter)

Updated

a month ago
No longer depends on: 1368083
(Reporter)

Updated

a month ago
No longer depends on: 1366564

Updated

a month ago
Depends on: 1375976
(Reporter)

Updated

a month ago
No longer depends on: 1370571
(Reporter)

Updated

a month ago
No longer depends on: 1366050
(Reporter)

Updated

a month ago
No longer blocks: 1345321
(Reporter)

Updated

a month ago
Depends on: 1366050
(Reporter)

Updated

13 days ago
Blocks: 1381147
(Reporter)

Updated

2 days ago
Depends on: 1384258
(Reporter)

Updated

2 days ago
No longer depends on: 1375774

Updated

19 hours ago
Depends on: 1384759
You need to log in before you can comment on or make changes to this bug.