fix(vendor): trust crate version only when coming from registries #14530

stormshield-guillaumed · 2024-09-11T13:36:03Z

What does this PR try to resolve?

Fixes #8181
Relates to #11897 and #14525

How should we test and review this PR?

As mentioned in the contribution guide, I made a first commit adding a test that passes with the actual behaviour. Then, I made a second commit with a fix and modified the test with the new expected behaviour.

Additional information

The fix doesn't take into account switching from a git dependency to crates.io, which is not handled correctly on master either, and would probably require the vendoring to serialize the source ID to detect source changes.

I specifically limited the trust of immutable version to crates.io, but it could be extended to other registries.

rustbot · 2024-09-11T13:36:09Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

weihanglo · 2024-09-11T14:58:14Z

src/cargo/ops/vendor.rs

-        if dir_has_version_suffix && cksum.exists() {
-            // Always re-copy directory without version suffix in case the version changed
+        // The standard registry is the only immutable source,
+        // path and git dependencies' versions cannot be trusted to mean "no change"


Path dependencies don't get vendored yet, see #13347.

And I feel like we can assume that all registry sources are immutable?

Could we keep this comment for when they will be or would you prefer I remove the mention to path dependencies ?
I wasn't sure whether to include all registries or not, but you are right, if a registry is not immutable, it will probably cause other problems before that. I'll change this.

We can leave the comment there. It just a note that Cargo doesn't vendor local path deps.

I wasn't sure whether to include all registries or not, but you are right, if a registry is not immutable, it will probably cause other problems before that. I'll change this.

Rebuild detection also holds that assumption, so yeah thanks for the update.

jsgf · 2024-09-11T22:04:49Z

src/cargo/ops/vendor.rs

-            // Always re-copy directory without version suffix in case the version changed
+        // The standard registry is the only immutable source,
+        // path and git dependencies' versions cannot be trusted to mean "no change"
+        if dir_has_version_suffix && id.source_id().is_crates_io() && cksum.exists() {


Will this have the effect of always revendoring git-sourced packages even if they haven't changed?

Yes. Unfortunately in .cargo-checksum.json there is no checksum recorded for git dependencies.

I think there is no way currently to retrieve the git rev of the vendored dependency.

…hange

weihanglo

While this may affect performance loss as it becomes always-re-vendor for git dependencies, the bug itself is more severe because it vendors the wrong stuff.

To get further for vendor correctness, cargo vendor could always compare file checksums, though it implies more non-trivial refactors than this bugfix. I think we can proceed with this simple patch first.

weihanglo · 2024-09-13T02:17:22Z

@bors r+

bors · 2024-09-13T02:17:25Z

📌 Commit a53b81a has been approved by weihanglo

It is now in the queue for this repository.

fix(vendor): trust crate version only when coming from registries ### What does this PR try to resolve? Fixes #8181 Relates to #11897 and #14525 ### How should we test and review this PR? As mentioned in the contribution guide, I made a first commit adding a test that passes with the actual behaviour. Then, I made a second commit with a fix and modified the test with the new expected behaviour. ### Additional information The fix doesn't take into account switching from a git dependency to crates.io, which is not handled correctly on master either, and would probably require the vendoring to serialize the source ID to detect source changes. I specifically limited the trust of immutable version to crates.io, but it could be extended to other registries.

bors · 2024-09-13T02:18:32Z

⌛ Testing commit a53b81a with merge 9045155...

bors · 2024-09-13T02:28:26Z

💔 Test failed - checks-actions

ehuss · 2024-09-13T03:22:09Z

CI issue is tracked in rust-lang/rust#130291, will be blocked until that is fixed.

weihanglo · 2024-09-14T18:26:43Z

@bors retry

bors · 2024-09-14T18:56:09Z

⌛ Testing commit a53b81a with merge d949021...

bors · 2024-09-14T19:23:51Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing d949021 to master...

Update cargo 24 commits in c1fa840a85eca53818895901a53fae34247448b2..468f1500bdca6591555b204ef31f92d725053190 2024-08-29 21:03:53 +0000 to 2024-09-14 19:24:54 +0000 - Disable the shell_completions tests (rust-lang/cargo#14546) - fix(vendor): trust crate version only when coming from registries (rust-lang/cargo#14530) - docs: Feature resolver version 2: clarify use of 'target' (rust-lang/cargo#14540) - Update docs for how cargo is published (rust-lang/cargo#14539) - feat: Add native comlpetion with CompleteEnv under the nightly (rust-lang/cargo#14493) - fix(new): Add to workspace relative to manifest, not current-dir (rust-lang/cargo#14505) - Fix parsing of comma separated values in --crate-type flag (rust-lang/cargo#14499) - Include public/private dependency status in `cargo metadata` (rust-lang/cargo#14504) - Remove unnecessary symbols (rust-lang/cargo#14519) - docs: bin source can be `src/main.rs` and/or in `src/bin/` (rust-lang/cargo#14515) - fix(toml): Don't require MSRV bump for pub/priv (rust-lang/cargo#14507) - bail before packaging on same version (rust-lang/cargo#14448) - Implement path-bases (RFC 3529) 2/n: `cargo [add|remove|update]` support (rust-lang/cargo#14427) - Publish workspace (rust-lang/cargo#14433) - Bump ci's version of cargo-semver-version (rust-lang/cargo#14503) - Document -Zpackage-workspace (rust-lang/cargo#14496) - uplift windows gnullvm import libraries (rust-lang/cargo#14451) - Bump to 0.84.0; update changelog (rust-lang/cargo#14495) - Fix cargo add behaving different when translating package name (rust-lang/cargo#13765) - chore(deps): update rust crate core-foundation to 0.10.0 (rust-lang/cargo#14475) - feat(resolve): Report MSRV compatible version instead of incomptible (rust-lang/cargo#14471) - Don't automatically include the current crate when packaging (rust-lang/cargo#14488) - Fix elided lifetime (rust-lang/cargo#14487) - chore(deps): update rust crate pasetors to 0.7.0 (rust-lang/cargo#14478) r? ghost

Update cargo 25 commits in c1fa840a85eca53818895901a53fae34247448b2..a9a418d1a22f29e7dfd034e3b93f15657e608a29 2024-08-29 21:03:53 +0000 to 2024-09-15 19:13:12 +0000 - chore: revert change to Cargo.lock in f25806c (rust-lang/cargo#14547) - Disable the shell_completions tests (rust-lang/cargo#14546) - fix(vendor): trust crate version only when coming from registries (rust-lang/cargo#14530) - docs: Feature resolver version 2: clarify use of 'target' (rust-lang/cargo#14540) - Update docs for how cargo is published (rust-lang/cargo#14539) - feat: Add native comlpetion with CompleteEnv under the nightly (rust-lang/cargo#14493) - fix(new): Add to workspace relative to manifest, not current-dir (rust-lang/cargo#14505) - Fix parsing of comma separated values in --crate-type flag (rust-lang/cargo#14499) - Include public/private dependency status in `cargo metadata` (rust-lang/cargo#14504) - Remove unnecessary symbols (rust-lang/cargo#14519) - docs: bin source can be `src/main.rs` and/or in `src/bin/` (rust-lang/cargo#14515) - fix(toml): Don't require MSRV bump for pub/priv (rust-lang/cargo#14507) - bail before packaging on same version (rust-lang/cargo#14448) - Implement path-bases (RFC 3529) 2/n: `cargo [add|remove|update]` support (rust-lang/cargo#14427) - Publish workspace (rust-lang/cargo#14433) - Bump ci's version of cargo-semver-version (rust-lang/cargo#14503) - Document -Zpackage-workspace (rust-lang/cargo#14496) - uplift windows gnullvm import libraries (rust-lang/cargo#14451) - Bump to 0.84.0; update changelog (rust-lang/cargo#14495) - Fix cargo add behaving different when translating package name (rust-lang/cargo#13765) - chore(deps): update rust crate core-foundation to 0.10.0 (rust-lang/cargo#14475) - feat(resolve): Report MSRV compatible version instead of incomptible (rust-lang/cargo#14471) - Don't automatically include the current crate when packaging (rust-lang/cargo#14488) - Fix elided lifetime (rust-lang/cargo#14487) - chore(deps): update rust crate pasetors to 0.7.0 (rust-lang/cargo#14478)

rustbot assigned ehuss Sep 11, 2024

rustbot added Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2024

weihanglo reviewed Sep 11, 2024

View reviewed changes

jsgf reviewed Sep 11, 2024

View reviewed changes

stormshield-guillaumed added 2 commits September 12, 2024 09:03

test(vendor): add test case updating git dependency without version c…

5fa43ee

…hange

fix(vendor): trust crate version only when coming from registries

a53b81a

stormshield-guillaumed force-pushed the vendor branch from 0e4d399 to a53b81a Compare September 12, 2024 07:11

stormshield-guillaumed changed the title ~~fix(vendor): trust crate version only when coming from crates.io~~ fix(vendor): trust crate version only when coming from registries Sep 12, 2024

weihanglo approved these changes Sep 13, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 13, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2024

bors merged commit d949021 into rust-lang:master Sep 14, 2024
22 checks passed

weihanglo mentioned this pull request Sep 15, 2024

Update cargo rust-lang/rust#130377

Merged

rustbot added this to the 1.83.0 milestone Sep 16, 2024

stormshield-guillaumed deleted the vendor branch September 16, 2024 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(vendor): trust crate version only when coming from registries #14530

fix(vendor): trust crate version only when coming from registries #14530

stormshield-guillaumed commented Sep 11, 2024

rustbot commented Sep 11, 2024

weihanglo Sep 11, 2024

stormshield-guillaumed Sep 12, 2024

weihanglo Sep 13, 2024

jsgf Sep 11, 2024

weihanglo Sep 11, 2024

stormshield-guillaumed Sep 12, 2024

weihanglo left a comment

weihanglo commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 13, 2024

ehuss commented Sep 13, 2024

weihanglo commented Sep 14, 2024

bors commented Sep 14, 2024

bors commented Sep 14, 2024

fix(vendor): trust crate version only when coming from registries #14530

fix(vendor): trust crate version only when coming from registries #14530

Conversation

stormshield-guillaumed commented Sep 11, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Sep 11, 2024

weihanglo Sep 11, 2024

Choose a reason for hiding this comment

stormshield-guillaumed Sep 12, 2024

Choose a reason for hiding this comment

weihanglo Sep 13, 2024

Choose a reason for hiding this comment

jsgf Sep 11, 2024

Choose a reason for hiding this comment

weihanglo Sep 11, 2024

Choose a reason for hiding this comment

stormshield-guillaumed Sep 12, 2024

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 13, 2024

bors commented Sep 13, 2024

ehuss commented Sep 13, 2024

weihanglo commented Sep 14, 2024

bors commented Sep 14, 2024

bors commented Sep 14, 2024