Mac builds currently have no symbols because llvm-dsymutil is crashing on XUL with rust enabled. Building XUL with rust 1.12 beta seems to remove the issue. Since 1.12 stable is coming soon and we've just bumped our nightly version, update the mac builders to use the 1.12 beta rust toolchain to work around bug 1301751. We will update all builders to 1.12 stable once it's released.

[ { "algorithm": "sha512", "visibility": "public", "filename": "rustc-x86_64-apple-darwin-repack.tar.bz2", "unpack": true, "digest": "5002507a74ee87105ca38a2fb0c677669828a473a2241c8939b58e01abe012b3812befe4ea2e4fcc78fc5a30086bc68a7d8c3b6afd40e59e37635131140814dc", "size": 153093812 } ]

Comment on attachment 8793890 [details] Bug 1304815 - Update mac builders to rustc 1.12.0-beta.5. https://reviewboard.mozilla.org/r/80494/#review79186 Thanks!

Looks like this resolves the issue on the try push: 12:16:53 INFO - 46107: Worker finished processing dist/universal/firefox/Nightly.app/Contents/MacOS/XUL in 467.90s

Pushed by rgiles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2cbbd85a818 Update mac builders to rustc 1.12.0-beta.5. r=ted

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/11e6496ab698 Update mac builders to rustc 1.12.0-beta.5: CLOBBER. r=me

Aryx clobbered with CLOBBER, and it continued to fail, I clobbered with the clobberer, and it continued to fail, I'm gonna go with "along with not needing to clobber every single time you land something, the other thing dependencies are good for is to let you control races." Backed out in https://hg.mozilla.org/integration/autoland/rev/c2d98ee4132a for intermittently failing like https://treeherder.mozilla.org/logviewer.html#?job_id=3922451&repo=autoland no matter how hard it was clobbered.

This looks to be fallout from bug 1300208, FWIW.

Or bug 1295762, which added the nsstring crate.

Yes. In particular bug 1300208 added direct calls to rustc which generates objects outside cargo's dependency tracking.

Theory is we just need a clean target for the new rustc-produced targets, which should make clobber work. I can't reproduce the collision locally though, so it will be hard to test.

I was wrong, it's not clobber failing to remove cruft. This is a new conflict with rust 1.12. The link includes both the gtest and non-gtest builds of nsstring, and the compiler now errors instead of passing both the the linker and hoping for the best. Cargo doesn't have this problem because it uses explicit `--extern crate_name=library_path` lines and/or -C metadata flags to mangle symbols differently for different dependent crates. I'll try some options, but probably we should replace the raw rustc invocation with a generated crate to let cargo handle this.

(In reply to Ralph Giles (:rillian) needinfo me from comment #12) > I'll try some options, but probably we should replace the raw rustc > invocation with a generated crate to let cargo handle this. It should have been done that way in bug 1300208 in the first place.

(In reply to Mike Hommey [:glandium] from comment #13) > (In reply to Ralph Giles (:rillian) needinfo me from comment #12) > > I'll try some options, but probably we should replace the raw rustc > > invocation with a generated crate to let cargo handle this. > > It should have been done that way in bug 1300208 in the first place. It's not obvious to me that we could have done it that way. You can write out the Cargo.toml that specifies what other libraries to be linking in, but you can't say, "oh, by the way, take the already-compiled rlibs from here, here, and here." So you'd have to recompile everything from scratch...and we've already decided that we don't want to depend on generated-in-the-objdir Cargo.toml because they don't play well with --frozen support in Cargo. It's possible that we could mitigate some of that by having a single CARGO_TARGET_DIR, but I suspect we'd have to either serialize all of our Cargo invocations (because concurrent cargo processes seems like it's asking for a bad time), or we'd have to use something like workspaces.

We've also been having some discussion in bug 1295762; bug 1295762 comment 55 makes it sound like we could be seeing a bug in Rust and how it keeps track of things internally. I don't really know how to verify that, though.

When we were batting around ideas on IRC the other day, I floated a potential solution, which would involve: 1) Making gkrust-gtest import the gkrust crate the usual Cargo way (via a path dependency) 2) Making the build system *not* link gkrust when linking xul-gtest, but instead just link gkrust-gtest, which will include all of the code from gkrust. 3) Switch to using a Cargo workspace so the two crates share intermediate crate builds etc. (This requires the Cargo in Rust 1.12, which conveniently is being released this Thursday, September 29th). This should get us back to a well-supported Cargo workflow, where Cargo does all of the dependency resolution and spits out a single Rust staticlib per link target. I'm not sure precisely what we'd need to do to make #2 work, maybe just some build system silliness to only link Rust libraries into final link targets, and not intermediate static libs? xul-gtest works by linking the xul library as a static library (not really a static library under the hood) and then linking the gtest code with that. If we dropped gkrust from the xul static lib this plan should Just Work.

This is why I think it makes more sense to have a CRATES variable in moz.build and generate the top-level crate directly. It's nice to be able to edit the build settings in various top-level Cargo.toml files, but we're caught between cargo being the only thing which knows how to build rust code and mozbuild being the only thing which knows what needs to be linked into each final target. I tried to write a minimal testcast for the nsstring SVH mismatch, without success. https://github.com/rillian/cratefoo

Generating crates doesn't seem like a good idea. That's not a thing that normal Rust projects do, so we'll be back in the land of weird build config bugs. I think my proposal in comment 16 is pretty workable: if we only link one crate per final linkable, we should be able to stay on cargo's good side. Using a workspace should ensure that we don't have to compile all the crates we use multiple times.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16) > When we were batting around ideas on IRC the other day, I floated a > potential solution, which would involve: > 1) Making gkrust-gtest import the gkrust crate the usual Cargo way (via a > path dependency) > 2) Making the build system *not* link gkrust when linking xul-gtest, but > instead just link gkrust-gtest, which will include all of the code from > gkrust. > 3) Switch to using a Cargo workspace so the two crates share intermediate > crate builds etc. (This requires the Cargo in Rust 1.12, which conveniently > is being released this Thursday, September 29th). FWIW this seems reasonable to me. You may not require workspaces to cache dependencies as well as simply using `CARGO_TARGET_DIR` may suffice (all pointing to the same location). If this is super painful, though, to do on the Gecko side then I'd be open to exploring alternate solutions in Cargo!

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16) > When we were batting around ideas on IRC the other day, I floated a > potential solution, which would involve: > 1) Making gkrust-gtest import the gkrust crate the usual Cargo way (via a > path dependency) > 2) Making the build system *not* link gkrust when linking xul-gtest, but > instead just link gkrust-gtest, which will include all of the code from > gkrust. > 3) Switch to using a Cargo workspace so the two crates share intermediate > crate builds etc. (This requires the Cargo in Rust 1.12, which conveniently > is being released this Thursday, September 29th). > > This should get us back to a well-supported Cargo workflow, where Cargo does > all of the dependency resolution and spits out a single Rust staticlib per > link target. I'm not sure precisely what we'd need to do to make #2 work, > maybe just some build system silliness to only link Rust libraries into > final link targets, and not intermediate static libs? xul-gtest works by > linking the xul library as a static library (not really a static library > under the hood) and then linking the gtest code with that. If we dropped > gkrust from the xul static lib this plan should Just Work. My suggestion in bug 1295762 comment 54 is pretty much a particular implementation of this. I think I've grown to like this solution a bit more if we make sure that mozbuild verifies that you do the right things. I really don't like the parallel dependency management, but we can use mozbuild to ensure that we do it correctly and avoid weird linker errors. Workspaces are also definitely something which we want, but I don't think they should be a necessary part of this solution. (In reply to Ralph Giles (:rillian) needinfo me from comment #17) > This is why I think it makes more sense to have a CRATES variable in > moz.build and generate the top-level crate directly. It's nice to be able to > edit the build settings in various top-level Cargo.toml files, but we're > caught between cargo being the only thing which knows how to build rust code > and mozbuild being the only thing which knows what needs to be linked into > each final target. I previously tried implementing something very similar to that in bug 1300208. It didn't work out very well. Cargo really likes to have complete control over its build environment, and generating Cargo.lock files is something which is near impossible (cargo byte-by-byte compares them to check if it needs to change them). Because of problems like that, I don't think that generating crates is a good strategy.

Dropping the 1.12.0-beta patch in favour of bug 1306438 now that 1.12.0 is in stable release.

I think this is basically what we discussed in comment 16 and in bug 1295762. It differs slightly (see the XXX comment in recursivemake.py) and it doesn't include the workspaces bit from comment 16. The slight difference is because the original strategy--at least as I understood it--doesn't work, and the lack of workspaces is because I would like to change one thing at a time. I can link xul and xul-gtest with and without debug information using Rust 1.12 on Linux x86-64 with this patch, whereas I could not link xul-gtest prior to this patch.

Comment on attachment 8798163 [details] [diff] [review] rearrange Rust crate structure for newer Rust releases Review of attachment 8798163 [details] [diff] [review]: ----------------------------------------------------------------- I have some drive-by comments ::: config/expandlibs_gen.py @@ +22,5 @@ > raise Exception("File not found: %s" % arg) > elif os.path.splitext(arg)[1] == conf.LIB_SUFFIX: > # We want to skip static libraries with the name foo-rs-prelink > # as they are individually linked for every final library, and > # thus should not be included in the descriptor file Remove this comment too ::: toolkit/library/gtest/rust/lib.rs @@ +1,5 @@ > // This Source Code Form is subject to the terms of the Mozilla Public > // License, v. 2.0. If a copy of the MPL was not distributed with this > // file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +extern crate gkrust_shared; I wish we could have code which verifies that you actually list your dependencies in your crate as extern crate, because otherwise you just get linker errors :-/. Not the best developer experience. ::: toolkit/library/rust/Cargo.toml @@ +5,5 @@ > license = "MPL-2.0" > description = "Rust code for libxul" > > [dependencies] > +gkrust-shared = { path = "shared" } I'm not a big fan of having a seperate gkrust-shared. Why can't gkrust-gtest just depend on gkrust? Is it because of the crate-type annotation? Can we have `crate-type = ["staticlib", "rlib"]` and dodge that problem?

(In reply to Michael Layzell [:mystor] from comment #23) > ::: config/expandlibs_gen.py > @@ +22,5 @@ > > raise Exception("File not found: %s" % arg) > > elif os.path.splitext(arg)[1] == conf.LIB_SUFFIX: > > # We want to skip static libraries with the name foo-rs-prelink > > # as they are individually linked for every final library, and > > # thus should not be included in the descriptor file > > Remove this comment too Good call. > ::: toolkit/library/gtest/rust/lib.rs > @@ +1,5 @@ > > // This Source Code Form is subject to the terms of the Mozilla Public > > // License, v. 2.0. If a copy of the MPL was not distributed with this > > // file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > +extern crate gkrust_shared; > > I wish we could have code which verifies that you actually list your > dependencies in your crate as extern crate, because otherwise you just get > linker errors :-/. Not the best developer experience. I don't think that code would be too difficult to write. File a bug? > ::: toolkit/library/rust/Cargo.toml > @@ +5,5 @@ > > license = "MPL-2.0" > > description = "Rust code for libxul" > > > > [dependencies] > > +gkrust-shared = { path = "shared" } > > I'm not a big fan of having a seperate gkrust-shared. Why can't gkrust-gtest > just depend on gkrust? Is it because of the crate-type annotation? Can we > have `crate-type = ["staticlib", "rlib"]` and dodge that problem? Yes, this is because of the crate-type annotation--I think...Cargo/rustc gives inscrutable errors when you try and make gkrust-gtest depend on a "staticlib" gkrust. I didn't think about providing multiple types, though! I can try that and see if that works...

(In reply to Nathan Froyd [:froydnj] from comment #24) > (In reply to Michael Layzell [:mystor] from comment #23) > > I'm not a big fan of having a seperate gkrust-shared. Why can't gkrust-gtest > > just depend on gkrust? Is it because of the crate-type annotation? Can we > > have `crate-type = ["staticlib", "rlib"]` and dodge that problem? > > Yes, this is because of the crate-type annotation--I think...Cargo/rustc > gives inscrutable errors when you try and make gkrust-gtest depend on a > "staticlib" gkrust. I didn't think about providing multiple types, though! > I can try that and see if that works... crate-type = ["staticlib", "rlib"] does indeed work to solve the problem. I am +/- on doing that vs. using gkrust-shared. On the one hand, having both crate types means that it's slightly more straightforward to figure out where to add code. We can do dependency checking relatively straightforwardly. But we're going to be writing a libgkrust.a twice in this scheme--though that may disappear with workspaces? Not sure. Having gkrust-shared means that we can keep the invariant that our top-level Cargo.tomls only declare a single kind of build artifact; having a representation of a Rust library in the build system is also made simpler with a single crate-type because we don't have to add code for cases that the build system doesn't have to care about in practice. (We're not going to be producing rlibs directly from the build system; we've been down that path several times and it just doesn't work well.) It also seems like an epsilon-better factoring of the dependency graph, though reasonable people can differ about that. But the dependency checking is a much thornier problem, and it's a different setup from what we have for libxul itself, where the sharedness is taken care of in the build system, and people don't have to care about sharedness. I'll keep the patch the way it is for now on the strength of not having to add code to account for how to name rlibs and whatnot in the build system; Ted can weigh in with what he thinks in the review.

What would the changes look like if, say, spidermonkey started wanting to use rust code? Would the dependency system handle that case correctly right now, and what crates would have to be added/modified?

(In reply to Michael Layzell [:mystor] from comment #26) > What would the changes look like if, say, spidermonkey started wanting to > use rust code? Would the dependency system handle that case correctly right > now, and what crates would have to be added/modified? I *think* all that would be needed would be adding a dependency to gkrust-shared on whatever JS crates were necessary. We'd probably need to make some provision for embedders, which means that a JS-only build would need a separate "toplevel" crate to ensure that code got compiled into a staticlib, and moz.build code to link that into the shell, etc.

Just for confirmation, this patch does fix the issues with Rust 1.12: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dc5412f8174

Passing this to Nathan, who seems to have this in hand. Nathan, I'll be away next week.

(In reply to Ralph Giles (:rillian) PTO until Oct 17 | needinfo me from comment #29) > Passing this to Nathan, who seems to have this in hand. Nathan, I'll be away > next week. Thanks for the heads-up! If this gets r+ before you get back, I'll land the Rust updates after landing this patch.

Comment on attachment 8798163 [details] [diff] [review] rearrange Rust crate structure for newer Rust releases Review of attachment 8798163 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the ASCII dependency graph, it's actually really helpful! A few fiddly things but this looks great. Thanks for fixing it! ::: config/rules.mk @@ -964,5 @@ > -endif > - > -$(RUST_PRELINK): $(RUST_PRELINK_DEPS) $(RUST_PRELINK_SRC) > - $(REPORT_BUILD) > - $(RUSTC) -o $@ --crate-type staticlib --target $(RUST_TARGET) $(RUST_PRELINK_FLAGS) $(RUST_PRELINK_SRC) I'm glad we can get rid of this! ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +1260,5 @@ > + > + # For much the same reasons as we are doing the checks below, we can > + # only support a single Rust library directly linked into a library. > + if len(direct_linked) >= 2: > + raise Exception("Cannot link more than one Rust library into " + obj) Did you want to add a new unit test for this condition? @@ +1293,5 @@ > + # link it in when compiling gkrust-gtest. So we instead declare > + # a gkrust-shared and have both gkrust and gkrust-gtest depend > + # on that. But verifying that dependencies are satisfied in > + # that case is rather hairy. > + if False and rlibs: I'd rather not leave this code in and disabled. Remove it, file a new bug about trying to fix it, and just leave the bug number in the comment? @@ +1303,2 @@ > > + backend_file.write('RUST_STATIC_LIB_FOR_SHARED_LIB := %s/%s\n' % If nothing else, this makes the backend bits simpler for linking Rust code, which is good. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +466,5 @@ > + dependencies = config.get('dependencies', None) > + if not dependencies: > + dependencies = set() > + else: > + dependencies = set(dependencies.iterkeys()) You could write this as a one-liner if you were so inclined, like: dependencies = set(config.get('dependencies', {}).iterkeys()) ::: toolkit/library/rust/lib.rs @@ +3,5 @@ > // file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +// You should not be adding code to this crate; you will almost certainly > +// get link errors when linking libxul-gtest. Add any |extern crate| > +// declarations or similar to the gkrust-shared crate. Mention that you want to be putting code in `toolkit/library/rust/shared/lib.rs` in this comment? I think we're going to want to write some docs somewhere very soon about writing Rust code in Gecko.

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8090e9a4b93d rearrange Rust crate structure for newer Rust releases; r=ted.mielczarek

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #31) > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +1260,5 @@ > > + > > + # For much the same reasons as we are doing the checks below, we can > > + # only support a single Rust library directly linked into a library. > > + if len(direct_linked) >= 2: > > + raise Exception("Cannot link more than one Rust library into " + obj) > > Did you want to add a new unit test for this condition? I added a new unit test and also moved the check into the frontend, where it is easier to test and available for all backends. I also implemented the rest of your suggestions. Thanks for the review!