1628852 - Add checks to ensure people don't land things that make `./mach vendor rust` non-idempotent

Status: NEW

(Developer Infrastructure :: Lint and Formatting, task)

Description

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

A number of times I've seen stuff (rust crate updates) get landed in such a way that somebody who checks out the repo and runs ./mach vendor rust ends up with a dirty tree. Recent examples are here and here. I think we should enforce the fact that ./mach vendor rust should produce zero changes on a clean tree, because otherwise it can complicate things for others trying to do crate updates.

I don't know if it would be better to do this as a lint on phabricator or a job in CI or what, but it should probably make heavy use of network caching because running ./mach vendor rust on every push will trigger a lot of network activity.

Also note that different versions of cargo and/or cargo-vendor may have differences in behaviour, so there should be a "blessed" version that people can use to match what happens in CI.

Comment 1

[Lina Butler [:lina]]

Having both accidentally done this and also experienced it on a fresh checkout, a lint to catch this would be wonderful!

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[Mike Hommey [:glandium]]

Vendoring patches are a PITA to review. I'm tempted to say ideally, people would send e.g. Cargo.toml updates to phabricator, and the vendoring would be autogenerated or something.

Comment 4

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

Yeah I agree that would be pretty good. I'm not sure if there are existing use cases where people need to make changes to Cargo.lock/vendorings that can't be triggered by a Cargo.toml change but I can't think of any off the top of my head.

Comment 5

[Mike Hommey [:glandium]]

cargo update

Comment 6

[Bobby Holley (:bholley)]

(In reply to Mike Hommey [:glandium] from comment #3)

Vendoring patches are a PITA to review. I'm tempted to say ideally, people would send e.g. Cargo.toml updates to phabricator, and the vendoring would be autogenerated or something.

I think it's pretty important for the actual vendored diff to show up on phabricator, so that reviewers have a sense of what code changes are being pulled in. The current situation isn't ideal for auditing (sometimes it's impractical), but it's certainly better to have the diff go through somebody's visual queue than having it land silently.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Bobby Holley (:bholley) from comment #6)

(In reply to Mike Hommey [:glandium] from comment #3)

Vendoring patches are a PITA to review. I'm tempted to say ideally, people would send e.g. Cargo.toml updates to phabricator, and the vendoring would be autogenerated or something.

I think it's pretty important for the actual vendored diff to show up on phabricator, so that reviewers have a sense of what code changes are being pulled in. The current situation isn't ideal for auditing (sometimes it's impractical), but it's certainly better to have the diff go through somebody's visual queue than having it land silently.

The problem is that it usually makes it hard to find the relevant non-vendor diff parts. And that someone still needs to ensure that the diff actually matches what vendor would do.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Mike Hommey [:glandium] from comment #7)

(In reply to Bobby Holley (:bholley) from comment #6)

(In reply to Mike Hommey [:glandium] from comment #3)

Vendoring patches are a PITA to review. I'm tempted to say ideally, people would send e.g. Cargo.toml updates to phabricator, and the vendoring would be autogenerated or something.

I think it's pretty important for the actual vendored diff to show up on phabricator, so that reviewers have a sense of what code changes are being pulled in. The current situation isn't ideal for auditing (sometimes it's impractical), but it's certainly better to have the diff go through somebody's visual queue than having it land silently.

The problem is that it usually makes it hard to find the relevant non-vendor diff parts.

Those should be split into a separate "revendor" patch.

And that someone still needs to ensure that the diff actually matches what vendor would do.

Hm, why? In general, the people submitting patches with revendoring are trusted. The threat model is a supply-chain attack, in which somebody inserts malicious code into a dependency of Firefox and publishes it as a minor update, and that update gets automatically pulled in with less scrutiny than usual because the process is automatic.

Comment 9

[Mike Hommey [:glandium]]

Hm, why?

Not for rust so far, because it would actually require malicious intent, but I have seen honest mistakes happen with vendor patches for other things. And they wouldn't have been caught if I hadn't actually checked the patch contained the changes it intended to contain. I've also caught such problems after the fact, when running some in-tree update.sh didn't bring the code that was in-tree.

Comment 10

[Bobby Holley (:bholley)]

I think I may have misinterpreted the suggestion in comment 3. I thought it was proposing to have the reviewer inspect only the Cargo.toml and have the vendoring happen "behind the scenes". But given subsequent discussion, I think the actual suggestion is to have the infrastructure recompute/verify vendoring changes from Cargo.toml. Is that right?

If so, my objection from comment 6 would not apply. I don't have a clear sense of an ergonomic and practical way to implement it though.