Closed Bug 1381576 Opened 2 years ago Closed 2 years ago

Try using a cargo workspace for Gecko Rust crates again

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: ted, Assigned: mbrubeck)

References

Details

Attachments

(3 files)

froydnj tried this in bug 1302704 but was unsuccessful for a few reasons. We discussed this in SF and I believe we decided it was worth another try since some of those things had been fixed in the meantime.
I tried this today and ran into a couple of issues.  My root Cargo.toml looks like this:

[workspace]
members = [
  "toolkit/library/rust/",
  "toolkit/library/gtest/rust",
]

# It makes no difference to the analysis below whether you use glob paths or unglobbed paths.
exclude = [
  "third_party/rust/**",
  "servo/**",
  "servo/ports/**",
  "servo/components/**",
  "gfx/webrender/**",
  "gfx/webrender_api/**",
  "gfx/webrender_bindings/**",
  "media/libstagefright/binding/mp4parse/**",
]

Issues are, roughly in the order I am discovering them:

1. servo/servo/#17890, which will be merged soon.

2. Cargo complains that crates under servo/ live in the workspace defined in servo/Cargo.toml, and no amount of workspace.exclude magic is sufficient to convince it that we don't care.  One can work around this by forcibly removing servo/Cargo.toml, but this is not very nice.

3. The style crate seems to build slightly differently when I am building from topsrcdir vs. building from within the objdir: the `file!` macro returns relative vs. absolute paths.  I think this may be an artifact of the way I'm building, and it's possible it would be corrected if we invoked Cargo from the objdir as we do today.  I haven't chased down what causes this difference in `file!` invocations.

The above were discovered while building without a properly configured .cargo/config (i.e. dependencies were being downloaded from crates.io/github).  Once I set .cargo/config up correctly, new problems emerged:

4. Cargo complains that various dependencies of style and webrender, among others, aren't vendored.  This is because it wants to see all dependencies for all things in the workspace, even if the particular features aren't activated.  We ran into this problem last time as well (bug 1302704 comment 25).

5. Related to the last, Cargo also wants to see dev-dependencies as vendored crates as well.

I'm not sure how to work around #2 in a clean way right now.

My impression was that workspace.exclude was supposed to address problems like #4 and #5.  Alex, do I correctly understand how workspace.exclude works?
Flags: needinfo?(acrichton)
> and no amount of workspace.exclude magic is sufficient to convince it that we don't care

Hm I think that this may actually be a bug in Cargo where globs aren't respected (by accident) in the `exclude` key. If you remove the trailing `/**` from all those definitions does that work?

> The style crate seems to build slightly differently when I am building from topsrcdir vs. building from within the objdir

Surprising! Do you have an error message I could peek at to see here? I actually ran into this recently as well oddly enough but I just wanted to confirm it's the error I'd expect.

> Cargo complains that various dependencies of style and webrender, among others, aren't vendored.  This is because it wants to see all dependencies for all things in the workspace, even if the particular features aren't activated.

If the dev-dependencies and other feature-related deps of these crates shouldn't be included, could the crates be excluded from the workspace? Or is `cargo build` run directly inside these repos?
Flags: needinfo?(acrichton)
(In reply to Alex Crichton [:acrichto] from comment #2)
> > and no amount of workspace.exclude magic is sufficient to convince it that we don't care
> 
> Hm I think that this may actually be a bug in Cargo where globs aren't
> respected (by accident) in the `exclude` key. If you remove the trailing
> `/**` from all those definitions does that work?

It does not.

> > The style crate seems to build slightly differently when I am building from topsrcdir vs. building from within the objdir
> 
> Surprising! Do you have an error message I could peek at to see here? I
> actually ran into this recently as well oddly enough but I just wanted to
> confirm it's the error I'd expect.

I don't have an error message, per se, but I have symptoms!  This code:

https://hg.mozilla.org/mozilla-central/file/tip/servo/components/style/build.rs#l72

is trying to locate a file relative to the currently built Rust file.  It does this by using the `file!` macro and then manipulating the result.

When that code gets built in Gecko, we're running `cargo build` from /path/to/objdir/toolkit/rust/, and `file!` returns an absolute path to the build.rs file in my srcdir.  Path manipulation works fine and opening the file works fine, because it's an absolute path.  (AFAICT, this is also the behavior when building servo...)

When that code gets built in my experiment, we're running `cargo build` from the source directory of a mozilla-central checkout, and `file!` returns a *relative* path to the build.rs file from the toplevel (i.e. "servo/components/style/build.rs").  Path manipulation works fine, but opening the file does not, because the build "script" isn't running from the toplevel mozilla-central directory.

This is surprising behavior to me!  I tried to look at the rustc source to see how `file!` was implemented; it appears to be tied into general source code location facilities, so the difference in return values is sort of understandable, but somewhat undesirable.

> > Cargo complains that various dependencies of style and webrender, among others, aren't vendored.  This is because it wants to see all dependencies for all things in the workspace, even if the particular features aren't activated.
> 
> If the dev-dependencies and other feature-related deps of these crates
> shouldn't be included, could the crates be excluded from the workspace? Or
> is `cargo build` run directly inside these repos?

I am attempting to exclude the relevant crates from the workspace, and it doesn't seem to be having an effect.  `cargo build` is being run from the toplevel of a mozilla-central checkout, not inside any of the want-to-be-excluded directories.
Flags: needinfo?(acrichton)
> When that code gets built in my experiment, we're running `cargo build` from the source directory of a mozilla-central checkout, and `file!` returns a *relative* path to the build.rs file from the toplevel (i.e. "servo/components/style/build.rs").  

Ah ok, I think see what's happening here. AFAIK the `file!()` macro is directly related to how you invoked rustc itself. So if you run `rustc /path/to/file.rs` then `file!()` will have an absolute path, but if you run `rustc file.rs` it'll have a relative path.

Cargo attempts to compile with "small" paths to have "pretty" verbose output. Basically Cargo will try to strip the cwd of workspace, and compile a relative path or failing that it'll just use the absolute path. This means that crates.io/git deps, for example, will all use absolute paths b/c they're rarely below the current workspace. For path dependenices *not* in a workspace you'll use absolute paths b/c they're not hierarchically below the final package in the filesystem, but once everything's in the same workspace then all members are beneath the workspace root which allows uage of short paths.

I think this is boiling down to a bug in the build script itself. Could it be changed from `file!()` to usage of `env::current_dir()` at runtime? The cwd of a build script is constant across a compile regardless of workspace settings and such.

> I am attempting to exclude the relevant crates from the workspace, and it doesn't seem to be having an effect.  `cargo build` is being run from the toplevel of a mozilla-central checkout, not inside any of the want-to-be-excluded directories.

Hm this may just be a symptom of "exclude isn't working" above. If they're excluded from the workspace the dev-deps won't show up, but if Cargo's thinking they're in the workspace (for whatever reason) the dev-deps will show up.
Flags: needinfo?(acrichton)
> I think this is boiling down to a bug in the build script itself. Could it be changed from `file!()` to usage of `env::current_dir()` at runtime?

This was fixed by https://github.com/servo/servo/pull/18363.
Simplified the root Cargo.toml slightly.
Comment on attachment 8955351 [details]
Bug 1381576 - Use a Cargo workspace for rust crates.

https://reviewboard.mozilla.org/r/224526/#review230470

::: Cargo.toml:2
(Diff revision 2)
> +[workspace]
> +members = [

This `members` list includes all of the "root" crates whose dependencies we currently vendor.

::: Cargo.toml:19
(Diff revision 2)
> +
> +  "gfx/webrender",
> +  "gfx/webrender_api",
> +  "gfx/webrender_bindings",
> +
> +  "dom/webauthn/u2f-hid-rs",

`u2f-hid-rs` is excluded because it has dev-dependencies that are not necessary for the Firefox build, so we don't need or want to vendor them.

::: Cargo.toml:30
(Diff revision 2)
> +  "media/cubeb-rs",
> +  "media/cubeb-rs/cubeb-backend",
> +  "media/cubeb-rs/cubeb-core",
> +  "media/mp4parse-rust/mp4parse",
> +  "media/mp4parse-rust/mp4parse_capi",
> +  "media/mp4parse-rust/mp4parse_fallible",

These `media` crates are excluded because they have their own workspaces set up.  I think it would be harmless to remove those workspaces and include these in the top-level workspace instead, but there's no pressing reason to do this now.
Removed a mozbuild test that was failing because we no longer check for panic="abort" in library Cargo.toml files (because we can now set it just once in the root Cargo.toml).  New try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92c7a1bb64d0808d29f19970b4ff8502e5db5347
Comment on attachment 8955351 [details]
Bug 1381576 - Use a Cargo workspace for rust crates.

https://reviewboard.mozilla.org/r/224526/#review230628

I have a few questions here and I think you should update the docs a bit, but overall this looks like a nice improvement. Thanks for doing the work here!

::: Cargo.toml:1
(Diff revision 4)
> +[workspace]

It might be good to have a short comment with a guideline as to why crates are in one list or the other, for future developers who are adding new top-level crates.

Similarly, we should probably mention this in the "Including Rust Code in Firefox" doc: https://dxr.mozilla.org/mozilla-central/source/build/docs/rust.rst

::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 4)
>          if crate_type != 'staticlib':
>              raise SandboxValidationError(
>                  'crate-type %s is not permitted for %s' % (crate_type, libname),
>                  context)
>  
> -        # Check that the [profile.{dev,release}.panic] field is "abort"

Is there potential for any issues with this sneaking into a crate that's in the `exclude` list?

::: python/mozbuild/mozbuild/vendor_rust.py:272
(Diff revision 4)
> -        lockfiles = []
> -        for (lib, crate_root) in crates_and_roots:
> -            path = mozpath.join(self.topsrcdir, crate_root)
> -            # We use check_call instead of mozprocess to ensure errors are displayed.
> +        # We use check_call instead of mozprocess to ensure errors are displayed.
> -            # We do an |update -p| here to regenerate the Cargo.lock file with minimal changes. See bug 1324462
> +        # We do an |update -p| here to regenerate the Cargo.lock file with minimal changes. See bug 1324462
> -            subprocess.check_call([cargo, 'update', '--manifest-path', mozpath.join(path, 'Cargo.toml'), '-p', lib], cwd=self.topsrcdir)
> +        subprocess.check_call([cargo, 'update', '-p', 'gkrust'], cwd=self.topsrcdir)

Is this going to do the right thing when updating other top-level crates like geckodriver? I don't know how `cargo update -p` interacts with workspaces like that.
Attachment #8955351 - Flags: review?(ted) → review+
Comment on attachment 8955350 [details]
Bug 1381576 - Remove reference to obsolete encoding_rs/parallel-utf8 feature.

https://reviewboard.mozilla.org/r/224524/#review230634
Attachment #8955350 - Flags: review?(ted) → review+
Comment on attachment 8955352 [details]
Bug 1381576 - Re-vendor Rust dependencies.

https://reviewboard.mozilla.org/r/224528/#review230636

You are almost certainly going to have to rebase and regenerate this patch, but rs=me.
Attachment #8955352 - Flags: review?(ted) → review+
Comment on attachment 8955351 [details]
Bug 1381576 - Use a Cargo workspace for rust crates.

https://reviewboard.mozilla.org/r/224526/#review230708

::: python/mozbuild/mozbuild/frontend/emitter.py
(Diff revision 4)
>          if crate_type != 'staticlib':
>              raise SandboxValidationError(
>                  'crate-type %s is not permitted for %s' % (crate_type, libname),
>                  context)
>  
> -        # Check that the [profile.{dev,release}.panic] field is "abort"

No added risk, since the old code only checks this for "root" crates.  In fact, the only existing crates that have these profiles are gkrust and gkrust-test.

The `[profile.*]` section in the workspace manifest will be the only one used by the Firefox build; profile sections from dependencies have no effect.

::: python/mozbuild/mozbuild/vendor_rust.py:272
(Diff revision 4)
> -        lockfiles = []
> -        for (lib, crate_root) in crates_and_roots:
> -            path = mozpath.join(self.topsrcdir, crate_root)
> -            # We use check_call instead of mozprocess to ensure errors are displayed.
> +        # We use check_call instead of mozprocess to ensure errors are displayed.
> -            # We do an |update -p| here to regenerate the Cargo.lock file with minimal changes. See bug 1324462
> +        # We do an |update -p| here to regenerate the Cargo.lock file with minimal changes. See bug 1324462
> -            subprocess.check_call([cargo, 'update', '--manifest-path', mozpath.join(path, 'Cargo.toml'), '-p', lib], cwd=self.topsrcdir)
> +        subprocess.check_call([cargo, 'update', '-p', 'gkrust'], cwd=self.topsrcdir)

Yes, it'll regenerate the whole `Cargo.`lock file, adjusting any dependencies that have been added/removed/changed in any `Cargo.toml` files.  Which package is passed to `-p` doesn't really matter as long as it's a local one (so that it won't pull a new version of it from crates.io).

I double-checked this by adding a dependency to the geckodriver manifest and re-running `mach vendor rust` to make sure it was added to `Cargo.lock`.
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ea18b70b170
Remove reference to obsolete encoding_rs/parallel-utf8 feature. r=ted
https://hg.mozilla.org/integration/autoland/rev/bbae7807c164
Use a Cargo workspace for rust crates. r=ted
https://hg.mozilla.org/integration/autoland/rev/3bc1743ad418
Re-vendor Rust dependencies. r=ted
I think we may also need to update the .gitignore file to match this?
Flags: needinfo?(mbrubeck)
Thanks for the answers!
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2d5ccd7274f
Remove reference to obsolete encoding_rs/parallel-utf8 feature. r=ted
https://hg.mozilla.org/integration/autoland/rev/b26983780cb2
Use a Cargo workspace for rust crates. r=ted
https://hg.mozilla.org/integration/autoland/rev/33134d27bbee
Re-vendor Rust dependencies. r=ted
> I think we may also need to update the .gitignore file to match this?

Done, and fixed the bugzilla lint errors.
Flags: needinfo?(mbrubeck)
Product: Core → Firefox Build System
Two questions:
1) Should the comment at https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/python/mozbuild/mozbuild/vendor_rust.py#267-268 be removed now?
2) Should all rust projects in m-c be listed in exactly one of {workspace.members, workspace.exclude}? What happens if a project is not listed in either? For example gfx/wrench is currently not in either list.
Flags: needinfo?(mbrubeck)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> 2) Should all rust projects in m-c be listed in exactly one of
> {workspace.members, workspace.exclude}? What happens if a project is not
> listed in either? For example gfx/wrench is currently not in either list.

According to [1] `path` dependencies automatically become members of the workspace. So only crates that are "roots" of a dependency graph need to be listed.

[1] https://doc.rust-lang.org/cargo/reference/manifest.html#the-workspace-section
gfx/wrench is not in the dependency list of any of the other crates. It's a standalone thing, and it's something we want to keep standalone and not built by default in m-c. Right now trying to build it produces this error:

error: current package believes it's in a workspace when it's not:
current:   /Users/kats/zspace/mozilla/gfx/wrench/Cargo.toml
workspace: /Users/kats/zspace/mozilla/Cargo.toml

this may be fixable by adding `gfx/wrench` to the `workspace.members` array of the manifest located at: /Users/kats/zspace/mozilla/Cargo.toml

What we want is for it to be listed under the exclusions, but I guess my broader point is that we should have a lint to make sure that all Cargo.toml files in the tree are covered by exactly one of the {members, exclude}.
> gfx/wrench is not in the dependency list of any of the other crates. It's a standalone thing

Ok, so it should be explicitly listed in members.

> What we want is for it to be listed under the exclusions

Why? Members are not what’s compiled into Gecko (that’s the gkrust crate at toolkit/library/rust/Cargo.toml), they only share a build directory and a Cargo.lock file.

> all Cargo.toml files in the tree are covered by exactly one of the {members, exclude}

Not if they’re a path dependency of some other crate that is (recursively) a member.
(In reply to Simon Sapin (:SimonSapin) from comment #40)
> > gfx/wrench is not in the dependency list of any of the other crates. It's a standalone thing
> 
> Ok, so it should be explicitly listed in members.
> 
> > What we want is for it to be listed under the exclusions
> 
> Why? Members are not what’s compiled into Gecko (that’s the gkrust crate at
> toolkit/library/rust/Cargo.toml), they only share a build directory and a
> Cargo.lock file.

Doesn't that mean all the wrench dependencies and dev-dependencies will get listed in the Cargo.lock file? And will that not cause them to get vendored into third_party/rust?
I think we’d want to have it in the same Cargo.lock so that dependencies that are shared with Gecko stay in sync. But yeah, I hadn’t considered vendoring.

Alex, can we make cargo-vendor only consider at a set of "root" crates rather than the entire workspace?
Flags: needinfo?(acrichton)
Certainly! Such functionality isn't implemented today but it shouldn't be too hard to do so.
Flags: needinfo?(acrichton)
> 1) Should the comment at https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/python/mozbuild/mozbuild/vendor_rust.py#267-268 be removed now?

Yes.

> Doesn't that mean all the wrench dependencies and dev-dependencies will get listed in the Cargo.lock file? And will that not cause them to get vendored into third_party/rust?

Yes.  There are some crates that are currently excluded for just this reason (because otherwise they would add dev-dependencies that we don't want to vendor):

https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/Cargo.toml#30-31

If we could make cargo-vendor use a "whitelist" approach, we wouldn't need to exclude these from the workspace.
Flags: needinfo?(mbrubeck)
Filed bug 1443271 for the out-of-date comment in vendor_rust.py.
Target Milestone: --- → mozilla60
Depends on: 1445271
You need to log in before you can comment on or make changes to this bug.