Closed Bug 1490948 Opened 6 years ago Closed 6 years ago

Allow rust and bindgen to be used by Spidermonkey

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(8 files, 13 obsolete files)

1.46 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.12 KB, patch
sfink
: review+
Details | Diff | Splinter Review
10.74 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.93 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
2.76 MB, patch
Details | Diff | Splinter Review
15.78 KB, patch
chmanchester
: review+
Details | Diff | Splinter Review
1.93 KB, patch
chmanchester
: review+
Details | Diff | Splinter Review
1.81 KB, patch
chmanchester
: review+
Details | Diff | Splinter Review
I've tried doing my best in bug 1469027, but the last comment by glandium in bug 1469027 and discussions on irc had me realize that the approach wasn't correct. I've been spending time on this for 3 days and a half now, and I'm back to where I was on Monday. Could build people look at this problem, please?

I'll post a folded patch without the build bits, and will post another patch with where I was at, with comments.
Attached patch initial.patch (obsolete) — Splinter Review
Folded Cranelift patch, including dependencies, mach vendor rust etc, but nothing related to the build system.
Attached patch wip.patch (obsolete) — Splinter Review
My current wip patch, will comment appropriately the code.
Attachment #9008676 - Attachment is obsolete: true
Attachment #9008679 - Flags: review?(sphink)
Attachment #9008680 - Flags: review?(sphink)
Attached patch 3.bindgen-system-flags.patch (obsolete) — Splinter Review
This is partially working only. It was mostly for Android and Windows builds, but it seems the BINDGEN_SYSTEM_FLAGS environment variable is not set. It's defined in https://searchfox.org/mozilla-central/source/old-configure.in#4016 but I don't know how to propagate this value down to the shell.

The remaining part consists in setting it as an environment variable when building the JS shell; the build.rs file should then read it and things should go better. Of course, debugging what comes next will also be needed.
Attachment #9008681 - Flags: feedback?(core-build-config-reviews)
Attached patch 4.jsrust-static-lib.patch (obsolete) — Splinter Review
This adds a new Rust static library jsrust which goal is to get linked in as part of libjs_static.a. However, the build system doesn't seem to link it correctly at the moment. Help appreciated here.
Attachment #9008682 - Flags: feedback?(core-build-config-reviews)
Attached patch wip.patch (obsolete) — Splinter Review
The rest of the patch:

- in backend/common.py, a hack that was sufficient to link jsrust into libjs_static, but it failed for a browser build.
- in the TaskCluster's Spidermonkey Windows builds, provide clang access so llvm-config is available and usable by rust-bindgen. However, this wasn't enough to unbreak the build.
Remaining things that need to get fixed:

(This try build was using a different approach for the jsrust library that was linked into libxul, not into libjs_static, but I expect most issues will still happen: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4528a113bf038589a2eb6a2a5e0f1425f8d8ec07 )

- The libjsrust.a linkage into libjs_static.a should be done more properly, or an alternative approach should be found.
- The spidermonkey package needs to roll in all the dependencies to libjsrust, or it needs to not assume that they're in tree and instead it could download them from crates.io as usual.
- The tup build is unaware of some files produced by Cranelift, which are dependencies generated at build time via build.rs. I've been linked to https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/backend/cargo_build_defs.py but couldn't investigate more.
- The Windows spidermonkey builds are broken because of missing dependencies. Maybe this will get fixed by the BINDGEN_SYSTEM_FLAGS environment variable setting, maybe not.
- The Android builds are missing includes, which I think should be fixed by the BINDGEN_SYSTEM_FLAGS environment setting.
- The Spidermonkey Rust package (tier2) needs to be fixed; I haven't investigated it yet.

Could build people chime in and take over this bug, please? (sorry I can't needinfo build-peer it seems)
Flags: needinfo?(nfroyd)
Flags: needinfo?(mh+mozilla)
(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> - The libjsrust.a linkage into libjs_static.a should be done more properly,
> or an alternative approach should be found.

What does "more properly" mean here?

> - The spidermonkey package needs to roll in all the dependencies to
> libjsrust, or it needs to not assume that they're in tree and instead it
> could download them from crates.io as usual.

Uh, what is the packaging task doing here?  Is it trying to pull in all Rust crates or something?  I don't think we should be downloading anything from crates.io--those packages might not match what we're building with--so we should figure out how to package up just the bits that are being used by spidermonkey.  (The quick hack is to just package up all of third_party/rust, maybe with some cargo config bits to point the build at those files.)

> - The Windows spidermonkey builds are broken because of missing
> dependencies. Maybe this will get fixed by the BINDGEN_SYSTEM_FLAGS
> environment variable setting, maybe not.
> - The Android builds are missing includes, which I think should be fixed by
> the BINDGEN_SYSTEM_FLAGS environment setting.

You can just `export` variables from Makefiles, see the examples in config/config.mk, for instance.  I don't know if that's the best way forward, as BINDGEN_SYSTEM_FLAGS was really intended to be substituted into a .toml file (layout/style/bindgen.toml), and therefore might not be suitable for using as an environment variable as-is.

> - The Spidermonkey Rust package (tier2) needs to be fixed; I haven't
> investigated it yet.

This looks like something is trying to use cranelift, and something else didn't build it in:

[task 2018-09-12T17:39:02.692Z]   = note: /builds/worker/workspace/build/src/target/debug/deps/libmozjs_sys-25cac83658dd7ce3.rlib(Unified_cpp_js_src43.o): In function `AutoCranelift::init()':
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/build/src/js/src/wasm/WasmCraneliftCompile.cpp:145: undefined reference to `cranelift_compiler_create'
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/build/src/target/debug/deps/libmozjs_sys-25cac83658dd7ce3.rlib(Unified_cpp_js_src43.o): In function `js::wasm::CraneliftCompileFunctions(js::wasm::ModuleEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*)':
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/build/src/js/src/wasm/WasmCraneliftCompile.cpp:296: undefined reference to `cranelift_compile_function'
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/build/src/target/debug/deps/libmozjs_sys-25cac83658dd7ce3.rlib(Unified_cpp_js_src43.o): In function `AutoCranelift::~AutoCranelift()':
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/build/src/js/src/wasm/WasmCraneliftCompile.cpp:150: undefined reference to `cranelift_compiler_destroy'
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/gcc/bin/../lib/gcc/x86_64-pc-linux-gnu/6.4.0/../../../../x86_64-pc-linux-gnu/bin/ld: /builds/worker/workspace/build/src/target/debug/deps/js-89742a41fe30b6dd: hidden symbol `cranelift_compiler_destroy' isn't defined
[task 2018-09-12T17:39:02.692Z]           /builds/worker/workspace/gcc/bin/../lib/gcc/x86_64-pc-linux-gnu/6.4.0/../../../../x86_64-pc-linux-gnu/bin/ld: final link failed: Bad value
[task 2018-09-12T17:39:02.692Z]           collect2: error: ld returned 1 exit status

So somewhere needs tweaking, but I'm not sure what.

> Could build people chime in and take over this bug, please? (sorry I can't
> needinfo build-peer it seems)

Do the above answers help any?
Flags: needinfo?(nfroyd)
Comment on attachment 9008682 [details] [diff] [review]
4.jsrust-static-lib.patch

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

This seems reasonable enough.

::: js/src/jsrust/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +RustLibrary('jsrust')
> +
> +FINAL_LIBRARY = "js"

gkrust uses USE_LIBS:

http://dxr.mozilla.org/mozilla-central/source/toolkit/library/moz.build#362

and it's quite possible that trying FINAL_LIBRARY here goes through a different (untested) code path.  There are also specific requirements on where a RustLibrary gets linked relative to C/C++ code.  So either:

1) The build system isn't emitting anything for jsrust; or
2) The build system is, but jsrust isn't getting put in the right place on the link line, so you get link errors.
Attachment #9008682 - Flags: feedback?(core-build-config-reviews) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #9)

Thank you for your answers!

> (In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> > - The libjsrust.a linkage into libjs_static.a should be done more properly,
> > or an alternative approach should be found.
> 
> What does "more properly" mean here?

See the wip.patch; to make Glandium's suggestion work, I had libjsrust be a static library, that gets linked into libjs_static.a; I've tweaked the make backend, but it's not correct. (Now that I think about it, I can't see how we could implement what glandium suggested, since the Rust standard library symbols would end up duplicated by libjsrust and gkshared-rust)

> > - The spidermonkey package needs to roll in all the dependencies to
> > libjsrust, or it needs to not assume that they're in tree and instead it
> > could download them from crates.io as usual.
> 
> Uh, what is the packaging task doing here?  Is it trying to pull in all Rust
> crates or something?  I don't think we should be downloading anything from
> crates.io--those packages might not match what we're building with--so we
> should figure out how to package up just the bits that are being used by
> spidermonkey.  (The quick hack is to just package up all of
> third_party/rust, maybe with some cargo config bits to point the build at
> those files.)

The packaging task consists in making a tarball containing Spidermonkey and the minimal amount of transitive dependencies needed to compile it. It's for our embedders. Package up all of third_party/rust would probably mean taking a lot of external code that's not useful?

> 
> > - The Windows spidermonkey builds are broken because of missing
> > dependencies. Maybe this will get fixed by the BINDGEN_SYSTEM_FLAGS
> > environment variable setting, maybe not.
> > - The Android builds are missing includes, which I think should be fixed by
> > the BINDGEN_SYSTEM_FLAGS environment setting.
> 
> You can just `export` variables from Makefiles, see the examples in
> config/config.mk, for instance.  I don't know if that's the best way
> forward, as BINDGEN_SYSTEM_FLAGS was really intended to be substituted into
> a .toml file (layout/style/bindgen.toml), and therefore might not be
> suitable for using as an environment variable as-is.

The .toml file descriptor used by the style system is much more complex: it can create a list of arguments to bindgen::Builder::clang_arg() that are matched according to the current OS, target, env, etc. I've extracted what I thought were the useful bits for Cranelift. But ideally, the build system should find what these values are, and provide them to the build.rs script itself; using an environment variable might be one way to do so, but that might break quotes etc.
Comment on attachment 9008681 [details] [diff] [review]
3.bindgen-system-flags.patch

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

I mean, this looks fine, but I'm not sure this is the best way to deal with BINDGEN_SYSTEM_FLAGS...
Attachment #9008681 - Flags: feedback?(core-build-config-reviews) → feedback+
Attachment #9008679 - Flags: review?(sphink) → review+
Attachment #9008680 - Flags: review?(sphink) → review+
I don't have anything to add to what has already been said.
Flags: needinfo?(mh+mozilla)
So after discussions on IRC, the plan is the following (names are proposals and should not be considered definitive)

- to have js/src/rust be a static Rust library (with a moz.build containing a RustLibrary), named "jsrust". It only references jsrust_shared as an extern crate in its lib.rs file.
- to have js/src/rust/shared be a rlib Rust library named jsrust_shared, reusable by Gecko (in gkrust-shared) and by jsrust. It would reference all the Rust crates used in Spidermonkey, so at the moment, only baldrdash (to be defined in wasm/cranelift).

Then, all the following build configurations can work:
- build Gecko: jsrust_shared is added as part of gkrust_shared
- build the JS lib as a static library: jsrust is compiled as a static library, and programs using libjs_static must also link with the libjsrust.a. We can't just link libjsrust.a with libjs_static.a, because the static library libjs_static.a is also used for Gecko, and this would result in us duplicating symbols (e.g. the Rust standard library symbols used in both libjs_static.a and gkrust_shared would be duplicated).
- build the JS lib as a shared library: jsrust is compiled as a static library, which is added as a link time dependency to libmozjs.so

I'm now thinking: could the symbol duplication issue happen also in the third case, if people build Gecko with JS as a shared library? If so, we can prevent this by specifically linking libmozjs with libjsrust if and only if we're building a dynamic lib + only the shell.

ni? to make sure :hsivonen sees this comment after asking in the other bug.
Flags: needinfo?(hsivonen)
I don't have the expertise to comment on symbol duplication in the scenario where Firefox is built with JS as a shared library.

For clarity, my use case has these constraints:
 * encoding_c depends on encoding_rs (both vendored crates)
 * encoding_glue (Gecko-only crate) depends on encoding_rs
 * nsstring (Gecko-only crate) depends on encoding_rs
 * jsrust_shared depends on encoding_c
 * gkrust_shared depends on jsrust_shared, nsstring and encoding_glue. (Currently depends also on encoding_c but maybe that won't be necessary if encoding_c is pulled in via jsrust_shared.)
 * C++ in SpiderMonkey can call encoding_c
 * C++ in non-SpiderMonkey parts of libxul can call encoding_c
 * Firefox release builds end up with one copy of encoding_rs (ideally with the parts of encoding_c and encoding_rs that neither SpiderMonkey nor the rest of Gecko calls discarded from the binary).
 * Standalone SpiderMonkey works (ideally with the parts of encoding_c and encoding_rs that it doesn't use discarded from the binary).
Flags: needinfo?(hsivonen)
(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
> So after discussions on IRC, the plan is the following (names are proposals
> and should not be considered definitive)
> 
> - to have js/src/rust be a static Rust library (with a moz.build containing
> a RustLibrary), named "jsrust". It only references jsrust_shared as an
> extern crate in its lib.rs file.
> - to have js/src/rust/shared be a rlib Rust library named jsrust_shared,
> reusable by Gecko (in gkrust-shared) and by jsrust. It would reference all
> the Rust crates used in Spidermonkey, so at the moment, only baldrdash (to
> be defined in wasm/cranelift).

You probably want a js_for_gecko static library that contains only the C++ parts of SM, and js_static would contains jsrust too, so that you don't need to add jsrust everywhere js_static is used (in all the programs that use it).

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> I don't have the expertise to comment on symbol duplication in the scenario
> where Firefox is built with JS as a shared library.
> 
> For clarity, my use case has these constraints:
>  * encoding_c depends on encoding_rs (both vendored crates)
>  * encoding_glue (Gecko-only crate) depends on encoding_rs
>  * nsstring (Gecko-only crate) depends on encoding_rs
>  * jsrust_shared depends on encoding_c
>  * gkrust_shared depends on jsrust_shared, nsstring and encoding_glue.
> (Currently depends also on encoding_c but maybe that won't be necessary if
> encoding_c is pulled in via jsrust_shared.)

This feels like this wouldn't work for --enable-shared-js.
(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
> - build the JS lib as a static library: jsrust is compiled as a static
> library, and programs using libjs_static must also link with the
> libjsrust.a. We can't just link libjsrust.a with libjs_static.a, because the
> static library libjs_static.a is also used for Gecko, and this would result
> in us duplicating symbols (e.g. the Rust standard library symbols used in
> both libjs_static.a and gkrust_shared would be duplicated).

Presumably we could do some fiddling here and create an intermediate library (I'm not even going to propose a color for the bikeshed) that contains libjs and libjsrust, which all the things that currently link libjs *except* libxul could link to. In standalone JS builds we should just include libjsrust directly into libjs to avoid this hassle.

> - build the JS lib as a shared library: jsrust is compiled as a static
> library, which is added as a link time dependency to libmozjs.so
> 
> I'm now thinking: could the symbol duplication issue happen also in the
> third case, if people build Gecko with JS as a shared library? If so, we can
> prevent this by specifically linking libmozjs with libjsrust if and only if
> we're building a dynamic lib + only the shell.

For --enable-shared-js builds (aside: do we actually need to support this as a build configuration for Firefox anymore?) we would presumably link libjsrust into libmozjs and *not* into gkrust, so we'd need to add a cargo feature to gkrust-shared to disable it for that configuration:
https://dxr.mozilla.org/mozilla-central/rev/e923330d5bd3aef1f687eddf96a06ad5ec3860ed/toolkit/library/rust/shared/Cargo.toml#37
https://dxr.mozilla.org/mozilla-central/rev/e923330d5bd3aef1f687eddf96a06ad5ec3860ed/toolkit/library/rust/gkrust-features.mozbuild
(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> - The spidermonkey package needs to roll in all the dependencies to
> libjsrust, or it needs to not assume that they're in tree and instead it
> could download them from crates.io as usual.

This is a decision best left to the Spidermonkey peers: should the Spidermonkey source package include all the Rust dependencies from crates.io necessary to build it, or is it OK for cargo to fetch them from crates.io?

For CI though, we'll need to make it use the existing set of vendored crates regardless. If SM peers decide not to include vendored crates in the source package then we could simply point cargo at the `third_party/rust` directory from the original repo in this task. If SM does want to vendor crates in the source package then we'll need to figure out a way to make that work. We could probably add this functionality to `cargo-vendor`, or we might be able to get away with something simpler. Either way I'm realizing that we'll probably want to generate a Cargo.lock for the Spidermonkey source package, since it won't have one in the repo being part of the workspace. Hopefully there's a sensible way to get cargo to generate that lockfile without re-resolving the dependencies, so that it can use the same set of crates from the top-level Cargo.lock!
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #18)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> > - The spidermonkey package needs to roll in all the dependencies to
> > libjsrust, or it needs to not assume that they're in tree and instead it
> > could download them from crates.io as usual.
> 
> This is a decision best left to the Spidermonkey peers: should the
> Spidermonkey source package include all the Rust dependencies from crates.io
> necessary to build it, or is it OK for cargo to fetch them from crates.io?

Considering the spidermonkey tarballs are similar to Firefox in that they should contain all the vendored things they need, no.
I wouldn't mind if it were easy to pull stuff down from crates.io, ignoring the vendored copy, but I agree with glandium that the primary scenario (and the only one we need to support for now) is to contain all the vendored stuff.
(In reply to Mike Hommey [:glandium] from comment #16)
> (In reply to Henri Sivonen (:hsivonen) from comment #15)
> >  * gkrust_shared depends on jsrust_shared, nsstring and encoding_glue.
> > (Currently depends also on encoding_c but maybe that won't be necessary if
> > encoding_c is pulled in via jsrust_shared.)
> 
> This feels like this wouldn't work for --enable-shared-js.

I expect that, going forward, it's likely that another Rust crate shows up whose FFI would make sense to call both from SpiderMonkey C++ and from non-SpiderMonkey Gecko C++. That is, I don't except the scenario I described to remain exceptional.

How important is --enable-shared-js relative to being able to use the same FFI crate from both SpiderMonkey and the rest of Gecko? Could we drop support for --enable-shared-js? Could we make both the js shared library and libxul-without-js shared library hide the FFI symbols from each other so that they wouldn't conflict when the same FFI symbols are used internally by two shared libraries in the same process?
Flags: needinfo?(mh+mozilla)
Attached patch wip.patch (obsolete) — Splinter Review
A more wip-y patch, which builds in more platforms.

Old try build with a stack of patches somewhat equivalent to this WIP patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b855a2bf4e871550b124a9cb14bd9bc9a9cb1a6e

Remaining tasks:

- static analysis build on win32 opt is busted, for no apparent reason. Ted noticed on IRC that there's a segfault happening (in lld?).

- Spidermonkey windows debug builds are failing: the libmozjs complains that it's using undefined symbols that live in Windows system libraries.

- the packaging task now complains that the Cargo.file needs to be updated, while it was called with --frozen, so it stops.

- the Rust bindings packaging process is a mess: `cargo build` calls a `build.rs` file, which calls into autospider.py, which then builds the shell with defaults. So on Nightly, it tries to use Cranelift; but since the Spidermonkey Rust libs are linked into libjsrust.a, then symbols are missing when linking the Rust bindings lib. In fact, to fix this, we'd need the ability to conditionally select whether we link against libjsrust.a or not, if the symbols are missing. (We could probably detect if the libjsrust.a library has been compiled in the build.rs file and choose to link or not)

- I haven't investigated the Tup build at all.

(I won't be able to work on this for the next few days, so if anybody wants to chime in and try other things, this would be greatly appreciated.)
Attachment #9008681 - Attachment is obsolete: true
Attachment #9008682 - Attachment is obsolete: true
Attachment #9008683 - Attachment is obsolete: true
(In reply to Henri Sivonen (:hsivonen) from comment #21)
> How important is --enable-shared-js relative to being able to use the same
> FFI crate from both SpiderMonkey and the rest of Gecko? Could we drop
> support for --enable-shared-js?

These are questions for dev-platform.
Flags: needinfo?(mh+mozilla)
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d57ac1bef3a
Don't set RUST_TARGET to 0 in autospider; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd9fc971b3c
Set the LLVM_CONFIG environment variable when building the Spidermonkey hazard builds; r=sfink
(landing patches that aren't risky, to make the work just a bit more incremental)
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Keywords: leave-open
Attached patch rolledup.patch (obsolete) — Splinter Review
Forgot to say: this rolled up patch applies cleanly on top of mozilla-inbound 1dd9fc971b3c.
(In reply to Benjamin Bouvier [:bbouvier] from comment #22)
> - the packaging task now complains that the Cargo.file needs to be updated,
> while it was called with --frozen, so it stops.

I looked into this a bit.  What this message really means is that there is no Cargo.lock file in the source package.  cargo is trying to update the Cargo.lock file (i.e. write it), but it can't, because --frozen.

We didn't include Cargo.toml or Cargo.lock in the JS source package.  Including Cargo.toml raises some questions, because it refers to a bunch of Gecko paths that aren't included in the JS source package, and I think cargo would complain about that.  Including Cargo.lock doesn't do anything on its own, I think: removing --frozen and letting cargo do its thing creates Cargo.lock in the "wrong" place, js/src/rust/Cargo.lock.  So we wouldn't even pick up the toplevel Cargo.toml lock.

It's possible that maybe we should just *not* use lockfiles for standalone JS builds, i.e. not use --frozen for such builds.  It works OK, I think, because we shouldn't have to hit the network for our automation builds: all of the sources should already be downloaded and packaged.
Some update about the Rust package issue: I can now confirm that the Rust bindings are using the abort panic hook instead of the unwind panic hook.

This probably stems from the fact that Spidermonkey builds libjsrust with a specific profile (inherited from the toplevel Cargo.toml) that sets panic=abort. Then the Rust exception handling personality is defined in libjsrust, which is linked against libjs_static. So when compiling the Rust bindings package (that links with libjs_static), the Rust compiler observes there's already an EH personality that's defined, and doesn't try to override it, according to this comment: https://github.com/rust-lang/rust/blob/6810f5286b6b91daab06fc3dccb27d8c46f14349/src/libpanic_abort/lib.rs#L73-L94

I suspect that there was no EH personality set before because no Cargo profile is set when just compiling js-sys (i.e. js/src/Cargo.toml) and the Rust bindings package (i.e. js/rust/Cargo.toml); now since the profile is defined during the build of jsrust, then we define an EH personality.

I'll try to see if overriding RUSTFLAGS="-C panic=unwind" is sufficient to take precedence over the Cargo profile during the build of libjsrust.
(In reply to Benjamin Bouvier [:bbouvier] from comment #30)
> I suspect that there was no EH personality set before because no Cargo
> profile is set when just compiling js-sys (i.e. js/src/Cargo.toml) and the
> Rust bindings package (i.e. js/rust/Cargo.toml); now since the profile is
> defined during the build of jsrust, then we define an EH personality.
> 
> I'll try to see if overriding RUSTFLAGS="-C panic=unwind" is sufficient to
> take precedence over the Cargo profile during the build of libjsrust.

Is there a reason that JS wouldn't want to build with panic=abort in all circumstances, just like Gecko does?
(In reply to Nathan Froyd [:froydnj] from comment #31)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #30)
> > I suspect that there was no EH personality set before because no Cargo
> > profile is set when just compiling js-sys (i.e. js/src/Cargo.toml) and the
> > Rust bindings package (i.e. js/rust/Cargo.toml); now since the profile is
> > defined during the build of jsrust, then we define an EH personality.
> > 
> > I'll try to see if overriding RUSTFLAGS="-C panic=unwind" is sufficient to
> > take precedence over the Cargo profile during the build of libjsrust.
> 
> Is there a reason that JS wouldn't want to build with panic=abort in all
> circumstances, just like Gecko does?

Yes: the Rust bindings want to be able to catch unwind at runtime; there's even a test for it that checks that a JS function calling into a builtin function defined in Rust that panic!s will be caught at runtime: https://searchfox.org/mozilla-central/source/js/rust/tests/panic.rs

That's where the feature is implemented in the Rust bindings:
https://searchfox.org/mozilla-central/source/js/rust/src/panic.rs#18

For what it's worth, setting RUSTFLAGS in the mozjs-sys lib build.rs script did the trick, so the Rust bindings package task now works and passes tests \o/. I do wonder if we'd like to allow people who build the mozjs-sys crate to define a preferred behavior here instead; I'll assume no, at the moment, but that would be easy to change in the future (e.g. with a feature, or another env variable).

To wit: this override only applies to the Rust bindings build; regular builds of the JS shell / libjs_static used in Gecko still inherit panic=abort.
(In reply to Nathan Froyd [:froydnj] from comment #29)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #22)
> > - the packaging task now complains that the Cargo.file needs to be updated,
> > while it was called with --frozen, so it stops.
> 
> I looked into this a bit.  What this message really means is that there is
> no Cargo.lock file in the source package.  cargo is trying to update the
> Cargo.lock file (i.e. write it), but it can't, because --frozen.
> 
> We didn't include Cargo.toml or Cargo.lock in the JS source package. 
> Including Cargo.toml raises some questions, because it refers to a bunch of
> Gecko paths that aren't included in the JS source package, and I think cargo
> would complain about that.  Including Cargo.lock doesn't do anything on its
> own, I think: removing --frozen and letting cargo do its thing creates
> Cargo.lock in the "wrong" place, js/src/rust/Cargo.lock.  So we wouldn't
> even pick up the toplevel Cargo.toml lock.
> 
> It's possible that maybe we should just *not* use lockfiles for standalone
> JS builds, i.e. not use --frozen for such builds.  It works OK, I think,
> because we shouldn't have to hit the network for our automation builds: all
> of the sources should already be downloaded and packaged.

So it appears that the package source didn't include the top level Cargo.toml file, thus Cargo tried to create a Cargo.lock file in js/src/wasm/cranelift. If we want to include the top level Cargo.toml file, which I think we should do (e.g. to keep the profile sections and have the package build system as close to the normal build system as possible), we'd need to tweak it to not include references to other directories that are not in the package's source; and then run ./mach vendor rust. That's probably worth a separate script to do so.

It makes things a bit more complicated but it's probably cleaner and worth it in the long run: it means only Rust dependencies that are actually used by the Spidermonkey package will be included in the tarball.
Attached patch 3.build-system-support.patch (obsolete) — Splinter Review
See changeset message.
Attachment #9008675 - Attachment is obsolete: true
Attachment #9009546 - Attachment is obsolete: true
Attachment #9010998 - Attachment is obsolete: true
Attachment #9011781 - Flags: review?(core-build-config-reviews)
Attachment #9011782 - Flags: review?(core-build-config-reviews)
Attached patch 5.link-libjsrust-mozjs-sys.patch (obsolete) — Splinter Review
Adds a similar hack to what was in the Rust bindings lib to detect if libjsrust has been compiled, and if so, add it to the list of link dependencies for the mozjs-sys library.

Note that the exception handling personality must be set to unwinding (instead of aborting, as is the case in Gecko), so as to be compatible with the Rust bindings package which wants to be able to properly unwind Rust code called by Spidermonkey. (There are comments in the patch)
Attachment #9011786 - Flags: review?(nfitzgerald)
Comment on attachment 9011786 [details] [diff] [review]
5.link-libjsrust-mozjs-sys.patch

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

Uhh, this is actually not enough to compile with the right EH personality.
Attachment #9011786 - Flags: review?(nfitzgerald)
(of course the Rust compile flag for the panic behavior should be set before compiling Spidermonkey and its Rust dependencies, not after, /me facepalms)

See previous comment and in-code comments for explanations.
Attachment #9011786 - Attachment is obsolete: true
Attachment #9011833 - Flags: review?(nfitzgerald)
Attached patch 6.disable-win32-st-an.patch (obsolete) — Splinter Review
This disables Cranelift on win32 static analysis builds, because of the OOM LLVM (32 bits) runs into when compiling gkrust. These builds are not to be published anyway, if I understand correctly, and we don't lose much coverage in this case (other toolchains on other platforms use the clang plugin, etc.).
Attachment #9011883 - Flags: review?(core-build-config-reviews)
Comment on attachment 9011883 [details] [diff] [review]
6.disable-win32-st-an.patch

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

IMO this would be easier to read if you just did a bare `return` for the case that you _don't_ want -- less of a `not` stack to keep in one's head.

Also, this should include a check for non-debug builds, right?
Comment on attachment 9011833 [details] [diff] [review]
5.link-libjsrust-mozjs-sys.patch

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

Thanks!
Attachment #9011833 - Flags: review?(nfitzgerald) → review+
Comment on attachment 9011781 [details] [diff] [review]
3.build-system-support.patch

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

::: python/mozbuild/mozbuild/backend/common.py
@@ -276,5 @@
>  
>          system_libs = not isinstance(input_bin, StaticLibrary)
>          for lib in input_bin.linked_libraries:
> -            if isinstance(lib, RustLibrary):
> -                continue

I'm not sure about the changes in this file... If this ends up being what we need to do we would also need to remove the RUST_STATIC_LIB bits from rules.mk, because as this stands it will put rust static libs on the command line twice.

If I apply this set of patches locally and try to build without the changes made to this file everything builds and links just fine, when does it seem to be needed?
Attachment #9011781 - Flags: review?(core-build-config-reviews)
Comment on attachment 9011782 [details] [diff] [review]
4.bindgen-extra-flags.patch

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

It looks like Nathan already took a look at this one.
Attachment #9011782 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 9011883 [details] [diff] [review]
6.disable-win32-st-an.patch

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

I concur with dmajor's comments here.
Attachment #9011883 - Flags: review?(core-build-config-reviews)
Attached patch previous.patchSplinter Review
(In reply to Chris Manchester (:chmanchester) from comment #42)
> Comment on attachment 9011781 [details] [diff] [review]
> 3.build-system-support.patch
> 
> Review of attachment 9011781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/common.py
> @@ -276,5 @@
> >  
> >          system_libs = not isinstance(input_bin, StaticLibrary)
> >          for lib in input_bin.linked_libraries:
> > -            if isinstance(lib, RustLibrary):
> > -                continue
> 
> I'm not sure about the changes in this file... If this ends up being what we
> need to do we would also need to remove the RUST_STATIC_LIB bits from
> rules.mk, because as this stands it will put rust static libs on the command
> line twice.
> 
> If I apply this set of patches locally and try to build without the changes
> made to this file everything builds and links just fine, when does it seem
> to be needed?

I think I needed this to have libjsrust link into libjs_static.

Also, to make it clearer and more up to date, these build patches are to be included in addition to all the patches from the dependent bug, which add the actual Rust dependency. They'll probably need to be all folded in into a single patch, to avoid build bustage during bisections; I split those to make reviews easier. Re-attaching a rollup patch containing all the relevant dependencies, applying cleanly on mozilla-inbound 0888032a7a08 .
Attached patch 6.disable-win32-st-an.patch (obsolete) — Splinter Review
Attachment #9011883 - Attachment is obsolete: true
Attachment #9012115 - Flags: review?(core-build-config-reviews)
Comment on attachment 9011781 [details] [diff] [review]
3.build-system-support.patch

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

(In reply to Benjamin Bouvier [:bbouvier] from comment #45)
> Created attachment 9012081 [details] [diff] [review]
> previous.patch
> 
> (In reply to Chris Manchester (:chmanchester) from comment #42)
> > Comment on attachment 9011781 [details] [diff] [review]
> > 3.build-system-support.patch
> > 
> > Review of attachment 9011781 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: python/mozbuild/mozbuild/backend/common.py
> > @@ -276,5 @@
> > >  
> > >          system_libs = not isinstance(input_bin, StaticLibrary)
> > >          for lib in input_bin.linked_libraries:
> > > -            if isinstance(lib, RustLibrary):
> > > -                continue
> > 
> > I'm not sure about the changes in this file... If this ends up being what we
> > need to do we would also need to remove the RUST_STATIC_LIB bits from
> > rules.mk, because as this stands it will put rust static libs on the command
> > line twice.
> > 
> > If I apply this set of patches locally and try to build without the changes
> > made to this file everything builds and links just fine, when does it seem
> > to be needed?
> 
> I think I needed this to have libjsrust link into libjs_static.

Confirmed: I need this to link libjsrust against JS programs depending on libjs_static (when we *don't* compile the dynamic lib), since it's not possible to add RUST_STATIC_LIB to the archive in the LIBRARY makefile rule (in config/rules.mk).
Attachment #9011781 - Flags: review?(core-build-config-reviews)
Attached patch 3.build-system-alternative.patch (obsolete) — Splinter Review
Alternatively, this patch could replace the patch 3.build-system-support.patch, if it passes try... https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b3f17f5a39c0209395eeba65ba8335e74e5d84e
And this one could replace the 6th if --enable-release and --enable-debug behave fine (not guaranteed). Again, the same try build will show if that works fine.

(Note the build system requires to return None to mean that the DEFINE value should not be defined, not False, which took me a few hours and a :ted to find out; also, this kind of issues manifests in such a way that the CONFIG might be not defined while the DEFINE will be, leading to intractable errors)
Fwiw, my plan is to leave the Tup build broken, because it's tier 2 and I heard I could do that if I gave a heads up to :chmanchester et al., so I'll do that instead.
Flags: needinfo?(cmanchester)
Ok, I'm already looking into the tup bustage.
Flags: needinfo?(cmanchester)
Comment on attachment 9011781 [details] [diff] [review]
3.build-system-support.patch

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

Can you please point to the job or local configuration I can set up that causes an issue without the changes to common.py? As far as I can see we're linking libjsrust.a and libjs_static.a for programs that need them just fine without these changes.
Attachment #9011781 - Flags: review?(core-build-config-reviews)
needinfo for comment 52
Flags: needinfo?(bbouvier)
Attachment #9012115 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 9011782 [details] [diff] [review]
4.bindgen-extra-flags.patch

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

::: old-configure.in
@@ +3946,5 @@
>  AC_SUBST(MOZ_TREE_PIXMAN)
>  
>  BINDGEN_SYSTEM_FLAGS="$_BINDGEN_CFLAGS $NSPR_CFLAGS $NSS_CFLAGS $MOZ_PIXMAN_CFLAGS $MOZ_CAIRO_CFLAGS"
> +AC_SUBST(BINDGEN_SYSTEM_FLAGS)
> +BINDGEN_SYSTEM_TOML_FLAGS="$BINDGEN_SYSTEM_FLAGS"

I assume this change is necessary because configure otherwise complains that you are using the same variable for two different AC_SUBST* invocations?
Attachment #9011782 - Flags: review?(nfroyd) → review+
I found them. The new patch looks much better to me, although the remaining one-line change to common.py doesn't seem to be necessary. I'm running that through try now...
Flags: needinfo?(bbouvier)
Comment on attachment 9012115 [details] [diff] [review]
6.disable-win32-st-an.patch

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

::: js/moz.configure
@@ +493,5 @@
> +         when='--enable-compile-environment')
> +def enable_cranelift(enable_cranelift, static_analysis, debug, target):
> +    # LLVM runs out of memory on win32 static analysis optimized builds when
> +    # linking gkrust, so do not compile Cranelift there.
> +    if static_analysis and target.kernel == 'WINNT' and target.cpu == 'x86' and not debug:

Apart from the --enable-debug thing that was discussed on irc, --enable-cranelift shouldn't silently do nothing in some cases if it's passed explicitly. You want to add a enable_cranelift.origin == 'default' condition.
Attachment #9012115 - Flags: review+
(In reply to Chris Manchester (:chmanchester) from comment #55)
> I found them. The new patch looks much better to me, although the remaining
> one-line change to common.py doesn't seem to be necessary. I'm running that
> through try now...

Without any change to common.py and the top patch from https://treeherder.mozilla.org/#/jobs?repo=try&revision=eecd68fe9de7a68ef2bf4e21f4efea0c2e20300f so we get system libs from rust libraries linked this all looks ok on try. I'll land those patches for the tup backend in a separate bug.
(In reply to Mike Hommey [:glandium] from comment #56)
> Comment on attachment 9012115 [details] [diff] [review]
> 6.disable-win32-st-an.patch
> 
> Review of attachment 9012115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/moz.configure
> @@ +493,5 @@
> > +         when='--enable-compile-environment')
> > +def enable_cranelift(enable_cranelift, static_analysis, debug, target):
> > +    # LLVM runs out of memory on win32 static analysis optimized builds when
> > +    # linking gkrust, so do not compile Cranelift there.
> > +    if static_analysis and target.kernel == 'WINNT' and target.cpu == 'x86' and not debug:
> 
> Apart from the --enable-debug thing that was discussed on irc,
> --enable-cranelift shouldn't silently do nothing in some cases if it's
> passed explicitly. You want to add a enable_cranelift.origin == 'default'
> condition.

Do you have any documentation about the .origin == 'default' condition you're talking about, please? Or an example I could cargo-cult? I can't find any similar code in the tree by looking up partial subset of this string on searchfox.
Flags: needinfo?(mh+mozilla)
(In reply to Chris Manchester (:chmanchester) from comment #57)
> (In reply to Chris Manchester (:chmanchester) from comment #55)
> > I found them. The new patch looks much better to me, although the remaining
> > one-line change to common.py doesn't seem to be necessary. I'm running that
> > through try now...
> 
> Without any change to common.py and the top patch from
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=eecd68fe9de7a68ef2bf4e21f4efea0c2e20300f so we get
> system libs from rust libraries linked this all looks ok on try. I'll land
> those patches for the tup backend in a separate bug.

Thanks a lot for the top patch! Is that alright if I fold it in into the build system patch? Happy to credit you as a patch co-author.
(In reply to Benjamin Bouvier [:bbouvier] from comment #58)
> (In reply to Mike Hommey [:glandium] from comment #56)
> > Comment on attachment 9012115 [details] [diff] [review]
> > 6.disable-win32-st-an.patch
> > 
> > Review of attachment 9012115 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/moz.configure
> > @@ +493,5 @@
> > > +         when='--enable-compile-environment')
> > > +def enable_cranelift(enable_cranelift, static_analysis, debug, target):
> > > +    # LLVM runs out of memory on win32 static analysis optimized builds when
> > > +    # linking gkrust, so do not compile Cranelift there.
> > > +    if static_analysis and target.kernel == 'WINNT' and target.cpu == 'x86' and not debug:
> > 
> > Apart from the --enable-debug thing that was discussed on irc,
> > --enable-cranelift shouldn't silently do nothing in some cases if it's
> > passed explicitly. You want to add a enable_cranelift.origin == 'default'
> > condition.
> 
> Do you have any documentation about the .origin == 'default' condition
> you're talking about, please? Or an example I could cargo-cult? I can't find
> any similar code in the tree by looking up partial subset of this string on
> searchfox.

There's nothing special to it, just copy/paste. https://dxr.mozilla.org/mozilla-central/search?q=%22origin+%3D%3D+%27default%27%22&redirect=false
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd [:froydnj] from comment #54)
> Comment on attachment 9011782 [details] [diff] [review]
> 4.bindgen-extra-flags.patch
> 
> Review of attachment 9011782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: old-configure.in
> @@ +3946,5 @@
> >  AC_SUBST(MOZ_TREE_PIXMAN)
> >  
> >  BINDGEN_SYSTEM_FLAGS="$_BINDGEN_CFLAGS $NSPR_CFLAGS $NSS_CFLAGS $MOZ_PIXMAN_CFLAGS $MOZ_CAIRO_CFLAGS"
> > +AC_SUBST(BINDGEN_SYSTEM_FLAGS)
> > +BINDGEN_SYSTEM_TOML_FLAGS="$BINDGEN_SYSTEM_FLAGS"
> 
> I assume this change is necessary because configure otherwise complains that
> you are using the same variable for two different AC_SUBST* invocations?

No, the BINDGEN_SYSTEM_TOML_FLAGS gets processed with AC_SUBST_TOML and not AC_SUBST, which suggested a different substitution format. But maybe it would have worked, or maybe I would have run into the error you're describing. Will check.
Much cleaner and simpler, thanks :chmanchester!
Attachment #9011781 - Attachment is obsolete: true
Attachment #9012224 - Attachment is obsolete: true
Attachment #9012540 - Flags: review?(core-build-config-reviews)
Adjustments for the source package, as discussed with Nathan we can just disable --frozen when compiling the JS shell standalone:

- if we do this from a package directory (i.e. decompressed from the package archive), this will create a Cargo.lock file and reuse the libs that have been copied out in the make-source-package script. (There are too many of them, we should probably add a cargo-vendor command to just vendor in dependencies for a subset of the workspace, but oh well)
- if we do this from within the tree, since js/rust/ belongs to the toplevel workspace, it will reuse the toplevel Cargo.lock file and since all the dependencies have been vendored in, it won't update it.
Attachment #9012541 - Flags: review?(core-build-config-reviews)
(In reply to Benjamin Bouvier [:bbouvier] from comment #61)
> (In reply to Nathan Froyd [:froydnj] from comment #54)
> > Comment on attachment 9011782 [details] [diff] [review]
> > 4.bindgen-extra-flags.patch
> > 
> > Review of attachment 9011782 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: old-configure.in
> > @@ +3946,5 @@
> > >  AC_SUBST(MOZ_TREE_PIXMAN)
> > >  
> > >  BINDGEN_SYSTEM_FLAGS="$_BINDGEN_CFLAGS $NSPR_CFLAGS $NSS_CFLAGS $MOZ_PIXMAN_CFLAGS $MOZ_CAIRO_CFLAGS"
> > > +AC_SUBST(BINDGEN_SYSTEM_FLAGS)
> > > +BINDGEN_SYSTEM_TOML_FLAGS="$BINDGEN_SYSTEM_FLAGS"
> > 
> > I assume this change is necessary because configure otherwise complains that
> > you are using the same variable for two different AC_SUBST* invocations?
> 
> No, the BINDGEN_SYSTEM_TOML_FLAGS gets processed with AC_SUBST_TOML and not
> AC_SUBST, which suggested a different substitution format. But maybe it
> would have worked, or maybe I would have run into the error you're
> describing. Will check.

Confirmed, you're 100% right:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11224d03b250c4c6c3cc95fc4c75de498afe4192&selectedJob=201906998

[task 2018-09-27T10:45:17.485Z] 10:45:17     INFO -  Cannot use AC_SUBST and AC_SUBST_TOML_LIST on the same variable (BINDGEN_SYSTEM_FLAGS)
Here we go! Thanks Glandium for helping figuring out the last bits.

With this last patch, all the platforms now correctly build and pass tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=282c9c7435616db59cd4ad672ea876c883ea3333
Attachment #9012115 - Attachment is obsolete: true
Attachment #9012225 - Attachment is obsolete: true
Attachment #9012571 - Flags: review?(core-build-config-reviews)
Comment on attachment 9012541 [details] [diff] [review]
7.source-package.patch

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

This LGTM, but I feel subversive reviewing it, since I was involved in the discussion; I'll let somebody else handle it.

::: config/rules.mk
@@ +846,2 @@
>  cargo_build_flags += --frozen
> +endif

This needs a comment explaining why we're doing this, and why it's reasonable to do so.
Attachment #9012540 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 9012541 [details] [diff] [review]
7.source-package.patch

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

r+ with Nathan's suggestion added.
Attachment #9012541 - Flags: review?(core-build-config-reviews) → review+
Attachment #9012571 - Flags: review?(core-build-config-reviews) → review?(mh+mozilla)
Comment on attachment 9012571 [details] [diff] [review]
6.mozconfigure.patch

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

Having participated heavily in how this patch is written, I can't review it myself.
Attachment #9012571 - Flags: review?(mh+mozilla) → review?(core-build-config-reviews)
Comment on attachment 9012571 [details] [diff] [review]
6.mozconfigure.patch

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

Looks fine to me.
Attachment #9012571 - Flags: review?(core-build-config-reviews) → review+
Thanks for all the reviews! I'll add the comment before landing.
Keywords: leave-open
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/537457dc2f47
Add build system support for a Rust library in Spidermonkey; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/2225cbe1f042
Add support for extra bindgen flags when compiling Cranelift; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/e58d578f13d9
Link libjsrust into mozjs_sys if it's been compiled; r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c10309414ae
Adjustments for the source package; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d9500ca5761
Compile Cranelift on Nightlies except for Win32 static analysis builds; r=chmanchester
See Also: → 1494907
You need to log in before you can comment on or make changes to this bug.