Closed Bug 1423236 Opened 6 years ago Closed 6 years ago

Need a way to replace a rust crate with a custom version and land it in the tree

Categories

(Firefox Build System :: General, enhancement)

Other Branch
enhancement
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

This is similar to bug 1323557 but specifically includes landing a replacement crate into m-c. This might be needed in cases where upstreaming the desired changes and re-vendoring might take a while or comes with many incidental changes that are undesirable.

A concrete example that I can think of is where we ship a Firefox where one of the upstream rust crates has a security bug. Upon discovery, we want to patch the security bug in the upstream rust crate and ship a chemspill with the least risk possible. The best way to do this would be to fork the upstream crate at the version we shipped, apply the fix, and [replace] the buggy crate with the good one.

(Right now I'm trying to do this in order to incorporate servo/webrender#2128 into the webrender copy in m-c. This PR replaces the serde/serde-derive dependencies in webrender with a git branch to which Gankro has applied custom changes. It's turning out to be rather complex... some details in alexcrichton/cargo-vendor#64)
Just to check - does cargo edit-locally not do the trick here?
I hadn't heard of cargo-edit-locally before, but I just tried it and it doesn't work for my use case. It seems to be a wrapper around manually editing the Cargo.toml file; if that's all it is it won't help anyway as the problem happens later, during vendoring and/or building.
It would be very much better, from mercurial's perspective, to be able to edit in-place. Cargo is really being obnoxious with its behavior of not actively rejecting that. I guess we could update the hashes it checks...
We also have to consider that Servo's VCS syncing will periodically run `mach vendor rust` and push changes. So we potentially need to teach `mach vendor rust` and/or `cargo vendor` to ignore locally-modified crates (if that is the route we're taking).

For version control sanity, you likely want to reuse existing files. The way I would naively design this is to temporarily "exclude" a crate from vendoring and not have Cargo complain if there are local changes and hashes don't align.
I guess if we want to go that route, then we already have a mechanism that works, and that we're already using for libudev-sys. That is, add a [replace] for the crate [1], and point it to the locally-modified version which lives somewhere in the tree outside of third_party/rust. The vendoring process will still pull the unmodified crate from crates.io and put it in third_party/rust, but at build time it will be the modified version that is used.

Note1: In this setup, putting the modified version into third_party/rust is undesirable because the first step of the `mach vendor rust` is to rm -rf the contents of third_party/rust, and if that's the only place the modifications exist we won't be able to get them back.

Note2: The fact that the vendoring process pulls the unmodified crate into third_party/rust is dumb, and has been complained about previously [2]. It's easy enough to fix in cargo-vendor, but that just exposes what I believe to be a cargo bug that causes compilation to fail. I have a simplified repro of this awaiting Alex's input at [3].

I guess the difference between this and what I was looking for was that instead of manually putting the locally-modified version into the tree, I was trying to find a way to specify a git repo/branch in the [replace] entry and have it automatically vendored into third_party/rust instead of the unmodified crate. This would solve both note1 (since the modifications would exist in the git branch) and note2 (since we would vendor the modified rather than the unmodified crate).

[1] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/library/rust/Cargo.toml#50
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1388843#c31
[3] https://github.com/alexcrichton/cargo-vendor/issues/64#issuecomment-349355917
Here's a try push [1] that does what I described above for a modified serde{,_derive} that we'll want for webrender. I don't really like it but if it's the way we want to handle this I'm not going to spend too much on getting a better solution. The ugliness in this comes mostly from the patch at [2] wherein I hand-vendor the entire modified serde repo into third_party/rust-modified/. This includes a bunch of junk (docs, metadata, tests) that is not actual code but just happened to be living in the repo. Since I'm hand-vendoring this I'm probably pulling in more things than cargo-vendor would have but cleaning up by hand is error-prone so I don't really want to.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb52743b2e3b17fce51b79a5830c47921afb9278
[2] https://hg.mozilla.org/try/rev/349fb8dc40edc0e3daa6b01d8198ba1cf88f9ab0
Alex pointed me to [patch] which is new hotness to use instead of [replace]. This seems to basically do what I want. It seems I can't mix [replace] with [patch] so in order to use [patch] for the serde stuff I'll need to upgrade the existing [replace] for libudev-sys as well, so I'll do that in this bug to prove that it works.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4540f2e897579caffec7838c926da2890aa76919
Assignee: nobody → bugmail
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> [2] https://hg.mozilla.org/try/rev/349fb8dc40edc0e3daa6b01d8198ba1cf88f9ab0

Note that's exactly what we would *not* want.
Comment on attachment 8935120 [details]
Bug 1423236 - Use patch instead of replace to eliminate redundant vendored copy of libudev-sys.

https://reviewboard.mozilla.org/r/205966/#review211782
Attachment #8935120 - Flags: review?(ttaubert) → review+
Comment on attachment 8935121 [details]
Bug 1423236 - Rerun mach vendor rust.

https://reviewboard.mozilla.org/r/205968/#review211784
Attachment #8935121 - Flags: review?(ttaubert) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82c4bf2512de
Use patch instead of replace to eliminate redundant vendored copy of libudev-sys. r=ttaubert
https://hg.mozilla.org/integration/autoland/rev/3c57b31afc7f
Rerun mach vendor rust. r=ttaubert
https://hg.mozilla.org/mozilla-central/rev/82c4bf2512de
https://hg.mozilla.org/mozilla-central/rev/3c57b31afc7f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: