Closed Bug 1336555 Opened 3 years ago Closed 3 years ago

Ensure |mach vendor rust| is running a recent version of cargo-vendor

Categories

(Firefox Build System :: General, defect)

Other Branch
defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 1 obsolete file)

When you run |mach vendor rust| it downloads and installs cargo-vendor if it doesn't exist already. However if a newer version of cargo-vendor is released with bugfixes, |mach vendor rust| continues running the old version. This can result in badness because you might produce different vendoring results than somebody else. (A concrete example was a change to .gitattributes handling in cargo-vendor late last year that caused .gitattributes files to get added/removed from the tree depending on if you were running a cargo-vendor from before or after the change.)

It would be good if running |mach vendor rust| either automatically upgraded your cargo-vendor, or had a version check that prevented it from using older versions of cargo vendor that are known to have bugs.

From bug 1336528 comment 2, Alex Crichton said that the version of cargo-vendor in use can be determined by running |cargo install --list|.
It would be nice if there was a `cargo install --update foo` option. AFAIK right now we can only `cargo install foo`, which won't overwrite an existing install, or `cargo install --force foo`, which will always overwrite an existing install. I don't want to do the latter every time someone runs the command.
That sounds reasonable. I filed a github issue for it: https://github.com/rust-lang/cargo/issues/3645. Likely it will get duped to something else though.
Ah yeah that's tracked currently at https://github.com/rust-lang/cargo/issues/2082
Until the `cargo install --update` thing is working it's better to just abort on old cargo versions. Patches incoming.
Assignee: nobody → bugmail
Comment on attachment 8838229 [details]
Bug 1336555 - Ensure that cargo-vendor is new enough when running `mach vendor rust`.

https://reviewboard.mozilla.org/r/113188/#review116792

::: python/mozbuild/mozbuild/vendor_rust.py:46
(Diff revision 1)
> +        strips out .gitattributes files which we want.
> +        '''
> +        for l in subprocess.check_output([cargo, 'install', '--list']).splitlines():
> +            if l.startswith('cargo-vendor'):
> +                # The line looks like `cargo-vendor v0.1.3:` so we need to
> +                # extract the version number carefully.

Seems like this would be simpler with just a regex, honestly, like:
```
m = re.match('cargo-vendor v((\d\.)*\d):', line)
if m:
  version = m.group(1)
  # ...
```
Attachment #8838229 - Flags: review?(ted)
Comment on attachment 8838230 [details]
Bug 1336555 - Re-run `mach vendor rust` to remove spurious gitattribute files.

https://reviewboard.mozilla.org/r/113190/#review116794
Attachment #8838230 - Flags: review?(ted) → review+
Comment on attachment 8838229 [details]
Bug 1336555 - Ensure that cargo-vendor is new enough when running `mach vendor rust`.

https://reviewboard.mozilla.org/r/113188/#review116796

::: python/mozbuild/mozbuild/vendor_rust.py:119
(Diff revision 1)
>              self.log(logging.INFO, 'installing', {}, 'Installing cargo-vendor')
>              env = self.check_openssl()
>              self.run_process(args=[cargo, 'install', 'cargo-vendor'],
>                               append_env=env)
> +        elif not self.check_cargo_vendor_version(cargo):
> +            self.log(logging.ERROR, 'cargo_vendor', {}, 'cargo-vendor >= 0.1.3 required')

The nicer thing to do here would be to run `cargo install --force cargo-vendor`.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Seems like this would be simpler with just a regex, honestly, like:
> ```
> m = re.match('cargo-vendor v((\d\.)*\d):', line)
> if m:
>   version = m.group(1)
>   # ...
> ```

Good point, fixed.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> > +        elif not self.check_cargo_vendor_version(cargo):
> > +            self.log(logging.ERROR, 'cargo_vendor', {}, 'cargo-vendor >= 0.1.3 required')
> 
> The nicer thing to do here would be to run `cargo install --force
> cargo-vendor`.

Also fixed. I also tweaked the info messages to indicate that it takes a few minutes, since I was surprised by that the first time I ran this command.

The second patch is no longer needed, as the .gitattributes files are no longer in the tree anymore.
Attachment #8838230 - Attachment is obsolete: true
Comment on attachment 8838229 [details]
Bug 1336555 - Ensure that cargo-vendor is new enough when running `mach vendor rust`.

https://reviewboard.mozilla.org/r/113188/#review116824

::: python/mozbuild/mozbuild/vendor_rust.py:141
(Diff revisions 1 - 2)
>              self._run_command_in_srcdir(args=[cargo, 'update', '--manifest-path', mozpath.join(path, 'Cargo.toml'), '-p', lib])
>              self._run_command_in_srcdir(args=[cargo, 'vendor', '--sync', mozpath.join(path, 'Cargo.lock'), vendor_dir])
>          self.repository.add_remove_files(vendor_dir)
>  
> +        # 100k is a reasonable upper bound on source file size.
> +        FILESIZE_LIMIT = 100 * 1024

Where did this change come from?
Attachment #8838229 - Flags: review?(ted) → review+
Comment on attachment 8838229 [details]
Bug 1336555 - Ensure that cargo-vendor is new enough when running `mach vendor rust`.

https://reviewboard.mozilla.org/r/113188/#review116824

> Where did this change come from?

MozReview's interdiff - it seems to have picked up a change that landed in central already (http://searchfox.org/mozilla-central/diff/6bf34c2a331d0bb87cff96619506ccd57642d8a9/python/mozbuild/mozbuild/vendor_rust.py#122) when I rebased my patch across it.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86c5b56e42f6
Ensure that cargo-vendor is new enough when running `mach vendor rust`. r=ted
https://hg.mozilla.org/mozilla-central/rev/86c5b56e42f6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.