Closed
Bug 1381576
Opened 7 years ago
Closed 6 years ago
Try using a cargo workspace for Gecko Rust crates again
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
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.
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
> 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)
Comment 3•7 years ago
|
||
(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)
Comment 4•7 years ago
|
||
> 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)
Assignee | ||
Comment 5•6 years ago
|
||
> 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.
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1e8a259f724775b8a6a0966901ef0c56732c5df
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•6 years ago
|
||
Oops, wrong trychooser syntax. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc6ff4fe0c488fc377923ee7f1490abed3001792
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•6 years ago
|
||
Simplified the root Cargo.toml slightly.
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
Oops, missed another python test. https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc2908a633fe8eb01666b19e992676fa88fafac
Reporter | ||
Comment 22•6 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 23•6 years ago
|
||
mozreview-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+
Reporter | ||
Comment 24•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-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`.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•6 years ago
|
||
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)
Reporter | ||
Comment 31•6 years ago
|
||
Thanks for the answers!
Comment 32•6 years ago
|
||
Backed out 3 changesets (bug 1381576) for Bugzilla linting failure on a CLOSED TREE https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3bc1743ad418e8bfebfe4a2323d2e3519ba82891&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running https://hg.mozilla.org/integration/autoland/rev/90ea9e2402f67e48c928c7c5463a17a0c12bc4ef
Comment 33•6 years ago
|
||
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
Assignee | ||
Comment 34•6 years ago
|
||
> I think we may also need to update the .gitignore file to match this?
Done, and fixed the bugzilla lint errors.
Flags: needinfo?(mbrubeck)
Comment 35•6 years ago
|
||
Pushed by mbrubeck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d25c533d1f12 Fix .gitignore syntax. r=jryans
Updated•6 years ago
|
Product: Core → Firefox Build System
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2d5ccd7274f https://hg.mozilla.org/mozilla-central/rev/b26983780cb2 https://hg.mozilla.org/mozilla-central/rev/33134d27bbee https://hg.mozilla.org/mozilla-central/rev/d25c533d1f12
Comment 37•6 years ago
|
||
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)
Comment 38•6 years ago
|
||
(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
Comment 39•6 years ago
|
||
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}.
Comment 40•6 years ago
|
||
> 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.
Comment 41•6 years ago
|
||
(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?
Comment 42•6 years ago
|
||
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)
Comment 43•6 years ago
|
||
Certainly! Such functionality isn't implemented today but it shouldn't be too hard to do so.
Flags: needinfo?(acrichton)
Assignee | ||
Comment 44•6 years ago
|
||
> 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)
Assignee | ||
Comment 45•6 years ago
|
||
Filed bug 1443271 for the out-of-date comment in vendor_rust.py.
Updated•6 years ago
|
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•