1324462 - mach vendor rust should allow updating specific libraries

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

On the graphics project branch we have added webrender as a dependency of libgkrust, and that pulls in all sorts of third-party dependencies. We are also doing regular merges from m-c to graphics. When somebody does |mach vendor rust| on m-c (e.g. to update the url library), that results in a merge conflict when merging m-c to graphics (because of all the extra libraries in the gkrust Cargo.lock files). Merging this by hand is difficult at best, and maybe impossible. So there are a couple of options to do this merge:

1) Before doing the merge, apply the same update (to the url library) on the graphics branch. Then merge. This should in theory result in no conflicts during the merge. However, this has the problem that there is no way to *just* update the url library when running |mach vendor rust|. Instead it pulls in updates to all the webrender-specific libraries, which may or may not be desirable[1]. This bug is requesting a way to just update the url library.

2) Do the merge, skipping conflict resolution. And then do another |mach vendor rust| on top of the merge to make everything work. Again this results in updates to webrender-specific libraries, which may or may not be desirable[1].

[1] Even though semantic versioning should mean that library updates are always ok, in practice there might be mistakes in version bumps that make it not true. For example right now on the graphics branch I seem to be getting a rust compile error after running an update and rebuilding, even though only minor version updates are happening.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

After discussing with :ted on IRC, it seems that I can accomplish the desired effect by doing this:

cd toolkit/library/rust/
cargo update -p url
cd ../gtest/rust
cargo update -p url
hg commit -m "Bump url version"
# comment out the line at [1]
mach vendor rust

[1] http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/python/mozbuild/mozbuild/vendor_rust.py#82

Comment 2

[(not currently active) Ted Mielczarek]

I'm not sure what the right thing to do is here--I thought `cargo generate-lockfile` would simply resolve anything in Cargo.toml that was missing from Cargo.lock, but it sounds like it also will update anything in Cargo.lock that has a newer compatible version on crates.io? Alex: is there a better way to accomplish that?

Also: I had heard it expressed elsewhere, but having a tool to merge Cargo.lock conflicts would be nice.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I think for the purposes of this bug the workflow in comment 1 is ok, we just need to add an option to |mach vendor rust| that allows skipping the generate-lockfile step.

Comment 4

[Alex Crichton [:acrichto]]

Yes unfortunately `cargo generate-lockfile` blows away the lockfile and starts from scratch. (unsure if this should be considered a bug or not)

As a workaround, you instead do something like:

    cargo update -p gecko

where "gecko" is some crate that can't actually be updated because it's a path dependency. I typically just use the top-level project's crate as part of this. That'll ask Cargo to update *only* the `gecko` crate, but at the same time it will produce an entire resolution graph with minimal changes. 

Does that work for this purpose?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Alex Crichton [:acrichto] from comment #4)
> As a workaround, you instead do something like:
> 
>     cargo update -p gecko
> 
> where "gecko" is some crate that can't actually be updated because it's a
> path dependency. I typically just use the top-level project's crate as part
> of this. That'll ask Cargo to update *only* the `gecko` crate, but at the
> same time it will produce an entire resolution graph with minimal changes. 

Just to make sure I understand this: in the Gecko context, we wouldn't have to say |cargo update -p gkrust|, where gkrust is the top-level crate for Gecko's purposes, we could say |cargo update -p foo|, where foo is some crate that gkrust depends directly or transitively on?  That should be very straightforward to add to |mach vendor rust|

Comment 6

[Alex Crichton [:acrichto]]

Oh so I understood the problem is that you just want to sync Cargo.lock and Cargo.toml, not actually update anything, in that case I think the command you want is `cargo update -p gkrust`. If you execute `cargo update -p foo` where foo is a dependency then that'll actually attempt to update foo (e.g. check crates.io for a newer version).

Note, though, that if foo is a path dependency (e.g. in tree, not on crates.io) then it will have the same effect as `cargo update -p gkrust`. Which is to say that updating a path dependency just syncs Cargo.lock and Cargo.toml.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1324462 - Don't automatically upgrade dependent libraries when running |mach vendor rust|.

This allows developers to manually upgrade packages as desired by running
|cargo update -p <package>| or |cargo update| on gkrust and gkrust-gtest.
Previously this update would happen implicitly as part of |mach vendor rust|,
and made it hard to upgrade specific packages without upgrading everything.

Review commit: https://reviewboard.mozilla.org/r/99780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99780/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8820244 [details]
Bug 1324462 - Don't automatically upgrade dependent libraries when running |mach vendor rust|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99780/diff/1-2/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

^ Rebased on top of the patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1277338#c26 - see the last few comments on that bug for discussion.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8820244 [details]
Bug 1324462 - Don't automatically upgrade dependent libraries when running |mach vendor rust|.

https://reviewboard.mozilla.org/r/99780/#review100320

LGTM; your call on implementing some of the below suggestions.

::: python/mozbuild/mozbuild/vendor_rust.py:79
(Diff revision 2)
> -        for crate_root in ('toolkit/library/rust/',
> -                           'toolkit/library/gtest/rust',
> -                           'js/src'):
> +        for (crate_root, lib) in (('toolkit/library/rust/', 'gkrust'),
> +                                  ('toolkit/library/gtest/rust', 'gkrust-gtest'),
> +                                  ('js/src', 'mozjs_sys')):

As long as we're making this more complicated, WDYT about making it:

```
crates_and_roots = (
    ('gkrust', 'toolkit/library/rust'),
    ...
    ('mozjs_sys', 'js/src'),
)

for (lib, crate_root) in crates_and_roots:
    ...
```

so any additions are an actual single-line change, and the loop header is not quite so complex?

::: python/mozbuild/mozbuild/vendor_rust.py:83
(Diff revision 2)
>          # just do this once on the workspace root crate.
> -        for crate_root in ('toolkit/library/rust/',
> -                           'toolkit/library/gtest/rust',
> -                           'js/src'):
> +        for (crate_root, lib) in (('toolkit/library/rust/', 'gkrust'),
> +                                  ('toolkit/library/gtest/rust', 'gkrust-gtest'),
> +                                  ('js/src', 'mozjs_sys')):
>              path = mozpath.join(self.topsrcdir, crate_root)
> -            self._run_command_in_srcdir(args=[cargo, 'generate-lockfile', '--manifest-path', mozpath.join(path, 'Cargo.toml')])
> +            self._run_command_in_srcdir(args=[cargo, 'update', '--manifest-path', mozpath.join(path, 'Cargo.toml'), '-p', lib])

Do you think it's worth providing a comment here about why we chose `update -p`?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Made both changes as suggested, landing updated patch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bac03e62480e
Don't automatically upgrade dependent libraries when running |mach vendor rust|. r=froydnj

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Made both changes as suggested, landing updated patch.

Thank you!

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bac03e62480e