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.

Folded Cranelift patch, including dependencies, mach vendor rust etc, but nothing related to the build system.

My current wip patch, will comment appropriately the code.

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.

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.

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)

(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?

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.

(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...

I don't have anything to add to what has already been said.

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.

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).

(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?

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.)

(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.

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)

Forgot to say: this rolled up patch applies cleanly on top of mozilla-inbound 1dd9fc971b3c.

https://hg.mozilla.org/mozilla-central/rev/9d57ac1bef3a https://hg.mozilla.org/mozilla-central/rev/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.

See changeset message.

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)

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.

(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.

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.).

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!

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?

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.

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.

(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 .

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).

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.

Ok, I'm already looking into the tup bustage.

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.

needinfo for comment 52

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?

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...

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.

(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.

(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

(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!

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.

(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

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.

Comment on attachment 9012541 [details] [diff] [review] 7.source-package.patch Review of attachment 9012541 [details] [diff] [review]: ----------------------------------------------------------------- r+ with Nathan's suggestion added.

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.

Comment on attachment 9012571 [details] [diff] [review] 6.mozconfigure.patch Review of attachment 9012571 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me.

Thanks for all the reviews! I'll add the comment before landing.

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

Last try build was all green (except for tier2 Btup, to be fixed in bug 1494139): https://treeherder.mozilla.org/#/jobs?repo=try&revision=282c9c7435616db59cd4ad672ea876c883ea3333

https://hg.mozilla.org/mozilla-central/rev/537457dc2f47 https://hg.mozilla.org/mozilla-central/rev/2225cbe1f042 https://hg.mozilla.org/mozilla-central/rev/e58d578f13d9 https://hg.mozilla.org/mozilla-central/rev/8c10309414ae https://hg.mozilla.org/mozilla-central/rev/4d9500ca5761