Closed Bug 1302704 Opened 3 years ago Closed 3 years ago

Use a Cargo workspace for Gecko crates

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ted, Assigned: froydnj)

References

Details

Attachments

(6 files, 1 obsolete file)

Attached patch Use a cargo workspace (obsolete) — Splinter Review
I originally had this as part of the patch in bug 1298422, but it doesn't work right if people use a Cargo that doesn't support workspaces, so we can't do this until we require a new-enough cargo. The problem is that with workspaces you wind up with a single Cargo.lock file at the workspace root, but an older cargo will ignore that and write them out next to each Cargo.toml file.

I don't actually know what version of cargo we'd need to require for this.

Attaching the WIP patch I have, but it will probably have to be rewritten once we are ready to land it. It's not very complicated, it just creates a $topsrcdir/Cargo.toml with a [workspace] key listing each of our crates, and moves the combined Cargo.lock file next to it, removing the other existing Cargo.lock files.
I believe Cargo with workspaces support is coming with Rust 1.12, which will arrive on September 29, 2016.
Sounds like this is necessary to avoid building the style system twice, which is really annoying for edit-compile-test cycle times. So we should try to get it sooner rather than later.

In the mean time, stylo hackers can comment out this line: https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/toolkit/library/moz.build#107
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> In the mean time, stylo hackers can comment out this line:
> https://dxr.mozilla.org/mozilla-central/rev/
> 215f9686117673a2c914ed207bc7da9bb8d741ad/toolkit/library/moz.build#107

Note: this doesn't work work, because it causes compilation failures wherever we use |FINAL_LIBRARY = 'xul-gtest'|.

Instead, I removed this:

http://searchfox.org/mozilla-central/rev/3079f45ce59e105e2490b1c8b3a5a52cd47a5ac0/toolkit/library/gtest/moz.build#23

Which seems to do the trick.
Depends on: 1314709
Now we've been requiring Rust 1.13, which has workspace support. Can we have this landed?

Having a global workspace also makes it easier to workaround issues inside dependencies (via putting "[replace]" section in the top level Cargo.toml), so I'd like to see it sooner rather than later as well.
Flags: needinfo?(ted)
We should add a Cargo verson check in rust.configure, since it's possible for people to install rust and cargo separately (Debian packages them independently):
https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/rust.configure

Once we have that in place then yes, we can make this change. We should only have to add a Cargo.toml file in $topsrcdir with `[workspace] members = ...`, regenerate the consolidated Cargo.lock file next to it, remove the existing Cargo.lock files, and update the `mach vendor rust` command to match:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/vendor_rust.py

Also it'd be nice to mention adding new crates to the top-level Cargo.toml file in the build system documentation, probably in the "Linking Rust Crates into something else" section:
https://dxr.mozilla.org/mozilla-central/source/build/docs/rust.rst
Depends on: 1321696
No longer depends on: 1314709
Flags: needinfo?(ted)
I said I would do this for Q1.
Assignee: nobody → nfroyd
Blocks: 1314477
Blocks: 1290956
I looked into this some yesterday.  I ran into https://github.com/rust-lang/cargo/issues/3539 and https://github.com/rust-lang/cargo/issues/3192, which means that we would need to import test-assembler to satisfy the dev-dependencies for mp4parse, even though we don't currently use anything that would require dev-dependencies.

We can modify mp4parse's import procedure to remove its dev-dependencies, I suppose.  But for crates that might be developed inside mozilla-central and want to include the typical Rust tests (which we don't run, but maybe should?), we'll have to vendor their test dependencies (and possibly doc/example dependencies) unnecessarily.

There's also:

warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /home/froydnj/src/gecko-dev.git/media/libstagefright/binding/mp4parse_capi/Cargo.toml
workspace: /home/froydnj/src/gecko-dev.git/Cargo.toml
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /home/froydnj/src/gecko-dev.git/media/libstagefright/binding/mp4parse/Cargo.toml
workspace: /home/froydnj/src/gecko-dev.git/Cargo.toml
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /home/froydnj/src/gecko-dev.git/netwerk/base/rust-url-capi/Cargo.toml
workspace: /home/froydnj/src/gecko-dev.git/Cargo.toml

which is really inconvenient.  The rust-url-capi one can go away (you're developing with -O3 and lto?), but the mp4parse ones are there for a reason, as their Cargo.toml documents, and to lose those would be unfortunate.  I don't think we want to compile all the Rust code for libxul with debug assertions on, either. :(
It's also not clear to me whether the upcoming Servo import would need to vendor its dev-dependencies etc. under the workspace model.  I *think* we would need to do that, which...ugh.
We cannot naively strip dev-dependencies, as some of the dependencies are for the build script, which is also widely used. (e.g. libbindgen is a dev-dependency of style component for build script to do build time bindgen.)

But yeah, vendoring testing-only dependencies which we never run doesn't sound right... I guess it may be possible to have a script to scan actual dependencies of each crate, and remove unneeded dev-dependencies from their Cargo.toml? But that may mean we can not rely on cargo's verdor command... or probably cargo can provide a hook for us to process a crate before it further fetching?
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> We cannot naively strip dev-dependencies, as some of the dependencies are
> for the build script, which is also widely used. (e.g. libbindgen is a
> dev-dependency of style component for build script to do build time bindgen.)

Why is it a dev-dependency?  The docs say: "Dev-dependencies are not used when compiling a package for building, but are used for compiling tests, examples, and benchmarks."  Are the docs wrong, or do things just happen to work for style because of other reasons?

> But yeah, vendoring testing-only dependencies which we never run doesn't
> sound right... I guess it may be possible to have a script to scan actual
> dependencies of each crate, and remove unneeded dev-dependencies from their
> Cargo.toml? But that may mean we can not rely on cargo's verdor command...
> or probably cargo can provide a hook for us to process a crate before it
> further fetching?

We can do whatever post-processing we like.  But I don't think that will play well with VCS sync from servo/servo (and perhaps other things at a later date).
One option for the [profile] thing is to just specify the necessary flags (e.g. -Z force-overflow-checks=on) as RUSTFLAGS when we invoke Cargo.  We might have to live with the warnings from vendored crates if nothing else, but this would at least let us keep the flags we want.
Oh, never mind. I was wrong. Build script uses build-dependencies, not dev-dependencies. So we can probably just strip all dev-dependencies. Actually I suspect whether cargo really vendor dev-dependencies? It sounds like a bug of cargo...
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> Actually I suspect whether cargo really vendor dev-dependencies? It sounds
> like a bug of cargo...

There are bugs open on ways to change this behavior with Cargo.toml declarations, but it sounds like the core behavior will not be changed.

The issue is that we declare all of our dependencies with { path = "......" } and such dependencies are considered as a member of the workspace, not as merely a dependency.  And as you probably want |cargo test| on a workspace to execute tests of all member crates, you would need to vendor dev-dependencies.  This makes a certain amount of sense, but it's inconvenient for how we're using Cargo, and somewhat surprising IMHO.

The proposed solution is to manually exclude certain crates from being considered workspace members.
(In reply to Nathan Froyd [:froydnj] from comment #11)
> One option for the [profile] thing is to just specify the necessary flags
> (e.g. -Z force-overflow-checks=on) as RUSTFLAGS when we invoke Cargo.  We
> might have to live with the warnings from vendored crates if nothing else,
> but this would at least let us keep the flags we want.

Ralph, how important are the debug assertions in the mp4parse crate?  Does mp4parse just turn on debug-assertions to get at -Z force-overflow-checks=on, or are there other interesting things that mp4parse cares about?
Flags: needinfo?(giles)
(In reply to Nathan Froyd [:froydnj] from comment #7)
> I looked into this some yesterday.  I ran into
> https://github.com/rust-lang/cargo/issues/3539 and
> https://github.com/rust-lang/cargo/issues/3192, which means that we would
> need to import test-assembler to satisfy the dev-dependencies for mp4parse,
> even though we don't currently use anything that would require
> dev-dependencies.
> 
> We can modify mp4parse's import procedure to remove its dev-dependencies, I
> suppose.  But for crates that might be developed inside mozilla-central and
> want to include the typical Rust tests (which we don't run, but maybe
> should?), we'll have to vendor their test dependencies (and possibly
> doc/example dependencies) unnecessarily.

I think we should absolutely figure out how to run the Rust unit tests for crates that live in mozilla-central. Things that live in third_party/rust are upstream code and I don't think we need to run their tests.

I think the solution here is to just vendor the dev-depedencies of crates that are in the workspace. This would mean that we'd have to vendor immediate dev-dependencies of servo itself, but not those from its transitive dependencies.
Filed bug 1331022 on running Rust tests.
(In reply to Nathan Froyd [:froydnj] from comment #14)
> Ralph, how important are the debug assertions in the mp4parse crate?  Does
> mp4parse just turn on debug-assertions to get at -Z
> force-overflow-checks=on, or are there other interesting things that
> mp4parse cares about?

If mp4parse relies on overflow checking for safety it should probably be using the explicit methods like `overflowing_add`:
https://doc.rust-lang.org/std/primitive.u32.html#method.overflowing_add

Those were added in Rust 1.7, so it's possible they didn't exist when this code was first written.
If it would help we can also definitely add new features to Cargo to help out this situation. For example:

* We could add a way to explicitly exclude path dependencies from a workspace (excluding mp4parse from the gecko workspace)
* We could add top-level workspace configuration saying "don't include dev deps", meaning that you just wouldn't be able to `cargo test` and such in workspace members.

Or something else along those lines. If you want to be sure to be able to run mp4parse tests, though, then the dev deps will need to be vendored, yeah.

Also for the profile issue I'm definitely open to possible changes there. Cargo historically hasn't been the best at fine-tuned control over how all the dependencies are compiled (e.g. which flags and whatnot), but I'm open to ideas of how to control this!
(In reply to Nathan Froyd [:froydnj] from comment #14)

> Ralph, how important are the debug assertions in the mp4parse crate?  Does
> mp4parse just turn on debug-assertions to get at -Z
> force-overflow-checks=on, or are there other interesting things that
> mp4parse cares about?

It was just for the release-build overflow checks as far as I remember. Matthew, anything I'm forgetting?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)

> If mp4parse relies on overflow checking for safety it should probably be
> using the explicit methods like `overflowing_add`:
> https://doc.rust-lang.org/std/primitive.u32.html#method.overflowing_add

We don't consider those practical; every arithmetic operation turns into a 12-character method call, which makes things much harder to read. IIRC we asked about getting a way to request -Z options for specific code blocks, which would be fine, but I don't think that was ever implemented.
Flags: needinfo?(giles) → needinfo?(kinetik)
> IIRC we asked about getting a way to request -Z options for specific code blocks, which would be fine, but I don't think that was ever implemented.

There's no need for -Z options for specific code blocks here, you can just call .checked_add() where you need it.
(In reply to Ralph Giles (:rillian) | needinfo me from comment #19)
> It was just for the release-build overflow checks as far as I remember.
> Matthew, anything I'm forgetting?

That's right.
Flags: needinfo?(kinetik)
Is this still the key to solving the double-rebuild? Here's a log of an 8-minute opt rebuild after touching a file in the style crate: https://pastebin.mozilla.org/8971751
Flags: needinfo?(ted)
Yeah, I believe so. Nathan indicated he was going to fix this.
Flags: needinfo?(ted)
The addition of webrender has apparently complicated things since the last time I tried this.
OK, so I can compile a tree with workspaces.  Open issues:

1. The webrender imports have a |workspace = ".."| key in them, which is undesirable for our purposes here.  Any future imports would have to strip those keys out.

2. webrender dev-depends on angle, which we may not be wild about.

3. Cargo really wants to see optional, feature-controlled but not feature-activated dependencies in our vendor directory when workspaces are active.  This is noticeably different from how Cargo works with our current setup.  Kats filed https://github.com/rust-lang/cargo/issues/3675 on this, but I think this is just a consequence of how Cargo workspaces implicitly treat path dependencies as members of the workspace, and we'd really need something more like Alex's proposal(s) in comment 18 to address this, which means waiting for a future Cargo release.  I have started trying to see what's necessary on the Cargo side for this.

4. We still have the warnings from comment 7.  Dealing with the rust-url-capi one is easy; dealing with the mp4parse-related ones is somewhat less so.  Turning on -Z force-overflow-checks=on for the entirety of Rust code seems undesirable, if possibly justifiable in terms of safety.  I'm interested to hear people's thoughts on turning on -Z force-overflow-checks=on globally and/or interesting ways that mp4parse could get the wrapping behavior without compiler options.  (Or if we could somehow shoehorn options into non-root crates in workspaces.)

5. The build system still uses CARGO_TARGET_DIR=. when invoking cargo on local manifests.  This works OK, except that it completely negates any benefit of workspaces: the gkrust_shared crate, for instance, is compiled separately for gkrust and gkrust_gtest.  Which means we'd need to rewrite the build system support for Rust entirely to line up with how workspaces work.  In particular, we'd need to drive Rust compilation from a single Cargo process for all of our Rust code (possibly a good thing, depending on how you look at it), and rewrite how our build system expects to find binaries and staticlibs from the Cargo compilation process.  I'm not sure how complex this rewrite would be.
(In reply to Nathan Froyd [:froydnj] from comment #25)
> 3. Cargo really wants to see optional, feature-controlled but not
> feature-activated dependencies in our vendor directory when workspaces are
> active.

I should also say: this drags in things we simply are not going to need like servo-freetype-sys (another copy of Freetype) and ipc-channel.
(In reply to Nathan Froyd [:froydnj] from comment #25)
> 5. The build system still uses CARGO_TARGET_DIR=. when invoking cargo on
> local manifests.  This works OK, except that it completely negates any
> benefit of workspaces: the gkrust_shared crate, for instance, is compiled
> separately for gkrust and gkrust_gtest.  Which means we'd need to rewrite
> the build system support for Rust entirely to line up with how workspaces
> work.  In particular, we'd need to drive Rust compilation from a single
> Cargo process for all of our Rust code (possibly a good thing, depending on
> how you look at it), and rewrite how our build system expects to find
> binaries and staticlibs from the Cargo compilation process.  I'm not sure
> how complex this rewrite would be.

I don't think that it would be too complex. It's just changing a bit of code which looks up files.

Can we put the workspace inside of library/ such that only gkrust, gkrust_shared, and gkrust_gtest are within it? Are you allowed to have path dependencies which are outside of the workspace?
(In reply to Michael Layzell [:mystor] from comment #27)
> (In reply to Nathan Froyd [:froydnj] from comment #25)
> > 5. The build system still uses CARGO_TARGET_DIR=. when invoking cargo on
> > local manifests.  This works OK, except that it completely negates any
> > benefit of workspaces: the gkrust_shared crate, for instance, is compiled
> > separately for gkrust and gkrust_gtest.  Which means we'd need to rewrite
> > the build system support for Rust entirely to line up with how workspaces
> > work.  In particular, we'd need to drive Rust compilation from a single
> > Cargo process for all of our Rust code (possibly a good thing, depending on
> > how you look at it), and rewrite how our build system expects to find
> > binaries and staticlibs from the Cargo compilation process.  I'm not sure
> > how complex this rewrite would be.
> 
> I don't think that it would be too complex. It's just changing a bit of code
> which looks up files.

The files part might not be so bad, true.  I'm not sure how bad redoing the makefile rules would be.

> Can we put the workspace inside of library/ such that only gkrust,
> gkrust_shared, and gkrust_gtest are within it? Are you allowed to have path
> dependencies which are outside of the workspace?

You are, and those path dependencies are considered to be regular dependencies, not members of the workspace, which might help address some of the the problems we're having.

I think we would want to extend the workspace to the entirety of Gecko eventually, though, since presumably programs that we compile are not going to want to recompile things that gkrust might have already compiled and vice versa.  But maybe by the time we did that, Cargo would have gained support for excluding paths from being considered members of the workspaces, so everything would work out.
(In reply to Nathan Froyd [:froydnj] from comment #25)
> 1. The webrender imports have a |workspace = ".."| key in them, which is
> undesirable for our purposes here.  Any future imports would have to strip
> those keys out.

I made a PR to fix this, https://github.com/servo/webrender/pull/856. It'll get merged over to the m-c copy in due course.
(In reply to Nathan Froyd [:froydnj] from comment #25)
> 3. [..snip..] but I think this is just a consequence of how Cargo workspaces implicitly
> treat path dependencies as members of the workspace

If it's just the webrender and webrender_traits path dependencies that are causing this problem, another possible solution here might be to vendor in webrender and webrender_traits into third_party/rust, rather than putting them in gfx/webrender and gfx/webrender_traits. Right now we have a gfx/webrender_bindings crate which is what adds path dependencies to webrender and webrender_traits, but we could replace those with a crates.io versioned dependency.

The main disadvantage is that we'd have to push webrender updates to crates.io more frequently, in order to pull them into m-c. If we had a travis hook to auto-publish on version bumps [1] that would make it much easier. An added advantage is that it would make updating our in-tree webrender much simpler since `mach vendor rust` would do most of the work for us.

This is something I'd been planning on doing at some point in the future anyway, but right now we're still churning webrender enough that I thought it made sense to wait.

[1] last line of Simon's comment at https://groups.google.com/d/msg/mozilla.dev.servo/iHykieVC5SM/i5TLurvNBAAJ
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> (In reply to Nathan Froyd [:froydnj] from comment #25)
> > 3. [..snip..] but I think this is just a consequence of how Cargo workspaces implicitly
> > treat path dependencies as members of the workspace
> 
> If it's just the webrender and webrender_traits path dependencies that are
> causing this problem, another possible solution here might be to vendor in
> webrender and webrender_traits into third_party/rust, rather than putting
> them in gfx/webrender and gfx/webrender_traits. Right now we have a
> gfx/webrender_bindings crate which is what adds path dependencies to
> webrender and webrender_traits, but we could replace those with a crates.io
> versioned dependency.

I would like that, but I think we can implement a temporary solution that doesn't put the workspace at the toplevel.  This has the disadvantage that the workspace would only apply to libgkrust and libgkrust_gtest, but I think we'd need more modifications to cargo to make things work how we would like, and we don't have a lot of other independent Rust code (e.g. programs) in the tree at the moment, so I think we can eat the cost of requiring Rust programs and/or other libraries to recompile the crates they need (?).

One other problem is that we currently use different profile settings for libgkrust and libgkrust_gtest (different values of profile.release.lto), and Cargo workspaces would require us to use a single configuration...which would in turn make Cargo workspaces unusable, because requiring a single setting of profile.release.lto doesn't work for us: lto = false would make the binaries too big and lto = true makes things unlinkable.  I'm not sure if there's a way around this. :(
> One other problem is that we currently use different profile settings for libgkrust and libgkrust_gtest (different values of profile.release.lto), and Cargo workspaces would require us to use a single configuration

It requires a single configuration per "profile", but we could use multiple profiles. Does libgkrust_gtest need to use the "release" profile rather than "dev" or "test"? We can still choose to enable optimizations, for example:

http://doc.crates.io/manifest.html#the-profile-sections
(In reply to Nathan Froyd [:froydnj] from comment #31)
> I would like that, but I think we can implement a temporary solution that
> doesn't put the workspace at the toplevel.  This has the disadvantage that
> the workspace would only apply to libgkrust and libgkrust_gtest, but I think
> we'd need more modifications to cargo to make things work how we would like,
> and we don't have a lot of other independent Rust code (e.g. programs) in
> the tree at the moment, so I think we can eat the cost of requiring Rust
> programs and/or other libraries to recompile the crates they need (?).

From what I understand right now libgkrust and libgkrust_gtest are the only final products which we create during a build, as we don't want to link in multiple copies of the standard library. The only problem then would be small standalone rust programs which we also build with mach, which I imagine won't share too many crates with libgkrust proper, at least at first.

If this turns out to be a bigger problem in the future, then we can work through it then. I wouldn't spend too much effort trying to optimize for that case right now.

> One other problem is that we currently use different profile settings for
> libgkrust and libgkrust_gtest (different values of profile.release.lto), and
> Cargo workspaces would require us to use a single configuration...which
> would in turn make Cargo workspaces unusable, because requiring a single
> setting of profile.release.lto doesn't work for us: lto = false would make
> the binaries too big and lto = true makes things unlinkable.  I'm not sure
> if there's a way around this. :(

Why does lto = true on libgkrust_gtest make things unlinkable?
Just getting libkgrust and libgkrust_gtest into a workspace seems like a reasonable first step. If we add other crates we can always manually add them to the workspace by putting them in `members` and pointing their `workspace` to the right place.
(In reply to Simon Sapin (:SimonSapin) from comment #32)
> > One other problem is that we currently use different profile settings for libgkrust and libgkrust_gtest (different values of profile.release.lto), and Cargo workspaces would require us to use a single configuration
> 
> It requires a single configuration per "profile", but we could use multiple
> profiles. Does libgkrust_gtest need to use the "release" profile rather than
> "dev" or "test"? We can still choose to enable optimizations, for example:
> 
> http://doc.crates.io/manifest.html#the-profile-sections

I suppose we could try building libgkrust_gtest with the test profile, though that might cause weirdness if #[test] Rust code gets in?  (Not quite sure how that works, so maybe that's not a problem.)

(In reply to Michael Layzell [:mystor] from comment #33)
> Why does lto = true on libgkrust_gtest make things unlinkable?

Because libgkrust and libgkrust_gtest then both have copies of the Rust runtime libraries embedded inside them (maybe other things as well?) and the compiler complains about multiply-defined symbols.  I thought there was a "cstaticlib" build kind that didn't link in the Rust runtime library code, but I can't find anything about that right now.
(In reply to Nathan Froyd [:froydnj] from comment #35)
> (In reply to Simon Sapin (:SimonSapin) from comment #32)
> > > One other problem is that we currently use different profile settings for libgkrust and libgkrust_gtest (different values of profile.release.lto), and Cargo workspaces would require us to use a single configuration
> > 
> > It requires a single configuration per "profile", but we could use multiple
> > profiles. Does libgkrust_gtest need to use the "release" profile rather than
> > "dev" or "test"? We can still choose to enable optimizations, for example:
> > 
> > http://doc.crates.io/manifest.html#the-profile-sections
> 
> I suppose we could try building libgkrust_gtest with the test profile,
> though that might cause weirdness if #[test] Rust code gets in?  (Not quite
> sure how that works, so maybe that's not a problem.)

`cargo test` for building gkrust_gtest doesn't work (complains about a missing `main` function).  So unless there's some other secret here...
(In reply to Nathan Froyd [:froydnj] from comment #35)
> Because libgkrust and libgkrust_gtest then both have copies of the Rust
> runtime libraries embedded inside them (maybe other things as well?) and the
> compiler complains about multiply-defined symbols.  I thought there was a
> "cstaticlib" build kind that didn't link in the Rust runtime library code,
> but I can't find anything about that right now.

I thought we were going to some effort to make sure that when we were building gtest, we didn't include libgkrust as well, and only linked in libgkrust_gtest in its place. When I was originally working on the build system IIRC that was what the plan was.

I don't think we should be linking in both libgkrust_gtest and libgkrust into any binaries.
Oh, that's right!  Maybe this is not a problem at all, then!
Experimenting with a non-topsrcdir workspace runs into at least two issues:

1. servo/ports/geckolib/ belongs to the servo/Cargo.toml workspace, so Cargo complains.
2. Various path dependencies are considered members of the workspace, and are therefore not below the workspace root, so Cargo complains.

I'm beginning to wonder whether it wouldn't just be simpler to compile things into a single CARGO_TARGET_DIR and skip workspaces altogether.
CARGO_TARGET_DIR is currently hardcoded to the current directory, but
we'd like the ability to choose a value for Rust libraries.
Attachment #8838217 - Flags: review?(cmanchester)
Attachment #8791169 - Attachment is obsolete: true
We're going to be introducing a value for CARGO_TARGET_DIR in a later
patch, we we should rename this function to not conflict with that
concept.
Attachment #8838218 - Flags: review?(cmanchester)
Rust libraries can set RUST_LIBRARY_TARGET_DIR so that they can share
compilation artifacts with other libraries.  This setting needs to be
propagated to the backend so it can be communicated to Cargo.
Attachment #8838219 - Flags: review?(cmanchester)
Doing this gets us all of the benefits that we want from Cargo
workspaces (only compiling artifacts from gkrust-shared one time for
both libraries) without all the hassles that using Cargo workspaces
would require.  We don't have to worry about multiple Cargo processes
running simultaneously and racing to write files; Cargo is smart enough
to lock the build directory to serialize access to it.
Attachment #8838220 - Flags: review?(cmanchester)
This is a non-ideal solution, of course: workspaces would enable Cargo to have full visibility into the dependency graph and to launch rustc processes simultaneously when possible.  For example, we currently have to compile all of gkrust before gkrust-gtest starts compiling.  With workspaces, we could compile parts of gkrust-gtest while gkrust finishes its final link.  So workspaces would probably be slightly faster than the current solution, but they have enough sharp edges for what we want to do that I'm not sure we want to try to work around those sharp edges.
(In reply to Nathan Froyd [:froydnj] from comment #44)
> This is a non-ideal solution, of course: workspaces would enable Cargo to
> have full visibility into the dependency graph and to launch rustc processes
> simultaneously when possible.  For example, we currently have to compile all
> of gkrust before gkrust-gtest starts compiling.  With workspaces, we could
> compile parts of gkrust-gtest while gkrust finishes its final link.  So
> workspaces would probably be slightly faster than the current solution, but
> they have enough sharp edges for what we want to do that I'm not sure we
> want to try to work around those sharp edges.

Could we not ask it to build gkrust-gtest first (which is a strict superset in terms of things to build aiui), and then build gkrust? If we did that the gkrust build would theoretically be just a single link.
(In reply to Michael Layzell [:mystor] from comment #45)
> Could we not ask it to build gkrust-gtest first (which is a strict superset
> in terms of things to build aiui), and then build gkrust? If we did that the
> gkrust build would theoretically be just a single link.

That would be a one-line change and is probably worth trying.
Sorry I haven't been following super closely up to this point, but out of curiosity is there a tl;dr; for the problems that workspaces are causing? I've seen https://github.com/rust-lang/cargo/issues/3192 cause problems when vendoring source code of repos, and would that help out with problems encountered here?
(In reply to Nathan Froyd [:froydnj] from comment #46)
> (In reply to Michael Layzell [:mystor] from comment #45)
> > Could we not ask it to build gkrust-gtest first (which is a strict superset
> > in terms of things to build aiui), and then build gkrust? If we did that the
> > gkrust build would theoretically be just a single link.
> 
> That would be a one-line change and is probably worth trying.

This doesn't make that much of a difference (testing with a non-Stylo build), but it did remind me that we need to tweak gkrust-gtest's Cargo.toml so Cargo thinks the dependencies for it are exactly the same as the ones for gkrust.  (I did make this change to make the workspaces review easier, but then discarded it when we weren't going to do workspaces and forgot to put it back in.)  Path forthcoming.
(In reply to Alex Crichton [:acrichto] from comment #47)
> Sorry I haven't been following super closely up to this point, but out of
> curiosity is there a tl;dr; for the problems that workspaces are causing?
> I've seen https://github.com/rust-lang/cargo/issues/3192 cause problems when
> vendoring source code of repos, and would that help out with problems
> encountered here?

I think it would help! The other issues, gleaned from comment 25 and comment 39:

1. Vendored dependencies often have `workspace` keys in them or Cargo.tomls in parent directories that specify workspaces.  This makes using vendored dependencies with workspaces problematic.  Maybe workspace determination should take source replacement into account, somehow?

2. Some libraries may want fine-grained flags; our mp4parse library turns on debug-assertions unconditionally so it can get at -Z force-overflow-checks=on.  We can't do something like that with workspaces, and it's not obvious that turning debug-assertions on globally(or even just force-overflow-checks, if we did so via RUSTFLAGS or similar) is desirable.

3. Requiring dev-dependencies unconditionally is surprising when compared to our current setup.  This might go away if cargo/#3192 was implemented, but it might also require separate configuration, like the bit suggested in comment 18.

I can file Cargo issues for some of these if you like.
The comment here is a relic from bygone days when we tried sticking
gkrust and gkrust-gtest into the same library at link time.  Making
gkrust and gkrust-gtest's profile.release settings identical also means
that Cargo considers build artifacts from one suitable for the other,
which is extremely desirable with our new shared CARGO_TARGET_DIR setup.
Attachment #8838247 - Flags: review?(cmanchester)
(In reply to Nathan Froyd [:froydnj] from comment #49)
> 1. Vendored dependencies often have `workspace` keys in them or Cargo.tomls
> in parent directories that specify workspaces.

Makes sense! Am I correct in inferring that you just don't want these crates in the workspace at all? They should be like a git/crates.io dep (e.g. opaque and unchangeable in tree)?

> 2. Some libraries may want fine-grained flags

Definitely! This isn't so much a workspace problem as it is a general Cargo problem. We've never had a great ability to have fine-grained configuration over the crate graph to dictate what's compiled with what.
I think https://github.com/rust-lang/rust/issues/33134 would help a little here but we probably want the ability to flag a crate needs to always have certain flags or such.

In the meantime I'd probably recommend enabling this via `RUSTFLAGS` and then only disabling it if it turns out to be a problem in practice.

> 3. Requiring dev-dependencies unconditionally is surprising when compared to
> our current setup.

Yeah this would definitely go away if webrender were *not* a member of the workspace (which I think you want?). Workspaces just bring in all the dev-dependencies of all their members.
(In reply to Nathan Froyd [:froydnj] from comment #49)
> I can file Cargo issues for some of these if you like.

Oh and yes of course, this would be great!
Attachment #8838217 - Flags: review?(cmanchester) → review+
Attachment #8838218 - Flags: review?(cmanchester) → review+
Comment on attachment 8838219 [details] [diff] [review]
part 3 - propagate information about CARGO_TARGET_DIR from the frontend into the backend

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

::: python/mozbuild/mozbuild/frontend/context.py
@@ +965,5 @@
> +        to the current objdir; absolute paths should not be used.
> +
> +        This variable should not be used directly; you should be using the
> +        RustLibrary template instead.
> +        """),

Then let's update the `RustLibrary` template in this commit as well.
Attachment #8838219 - Flags: review?(cmanchester) → review+
Comment on attachment 8838220 [details] [diff] [review]
part 4 - provide a target directory for gkrust and gkrust-gtest

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

I'm inclined to say these patches should be in a separate bug, or this one should be left open once this lands. I understand we're solving our main pain point here, but the context here might be useful for possible future efforts to use cargo workspaces.
Attachment #8838220 - Flags: review?(cmanchester) → review+
Comment on attachment 8838247 [details] [diff] [review]
part 5 - harmonize gkrust_gtest's profile.release with gkrust's

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

So fiddling with one of these Cargo.tomls is going to be enough to backslide into lots of unnecessary re-building? Can we come up with some way to fail the build if this happens?
Attachment #8838247 - Flags: review?(cmanchester) → review+
This patch addresses review comments about trying to enforce identical profiles
for gkrust and gkrust-gtest, so well-meaning changes to one but not the other
don't wind up regressing build times.  It's not perfect--changing the name of
gkrust will completely bypass the check--but it's at least one line of defense.
Attachment #8839622 - Flags: review?(cmanchester)
Comment on attachment 8839622 [details] [diff] [review]
part 6 - enforce identical profiles for gkrust and gkrust-gtest

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

Thank you for this addition.
Attachment #8839622 - Flags: review?(cmanchester) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b63c26dd7c1
part 1 - define CARGO_TARGET_DIR in the backend; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b7f6a47936
part 2 - rename cargo_target_directory; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3ffbed3a4b
part 3 - propagate information about CARGO_TARGET_DIR from the frontend into the backend; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/95b0a6226992
part 4 - provide a target directory for gkrust and gkrust-gtest; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbbf867604c
part 5 - harmonize gkrust_gtest's profile.release with gkrust's; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/e71b6d1907a8
part 6 - enforce identical profiles for gkrust and gkrust-gtest; r=chmanchester
Blocks: 1381576
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.