Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vendoring of git dependencies is sometimes incorrectly skipped #14525

Closed
stormshield-guillaumed opened this issue Sep 10, 2024 · 1 comment
Closed
Labels
C-bug Category: bug S-triage Status: This issue is waiting on initial triage.

Comments

@stormshield-guillaumed
Copy link
Contributor

Problem

The vendoring of git dependencies is incorrectly skipped under certain conditions. From my tests and by looking at the source code of cargo, the conditions for a crate A to be incorrectly skipped during vendoring seems to be :

  • A is a git dependency
  • a version of A is already vendored with a version suffix, i.e. log-0.4.22
  • you change the git revision of A to a different commit, but the crate version defined in the Cargo.toml of A is the same between the two commits

I made a minimal reproduction example in this repo with a CI job checking the update of the log dependency should change the vendor directory.

Steps

  1. git clone https://github.com/stormshield-guillaumed/vendor-repro.git
  2. sed -i s/d1a8306aadb88d56b74c73cdce4ef0153fb549cb/96dbf58ec05e844ec55aa2282f32f612372fbced/ Cargo.toml
  3. cargo vendor --versioned-dirs > .cargo/config.toml

The log's sources should have changed after this but they have not.

Possible Solution(s)

The part where cargo decides whether it skips the vendoring of a crate or not seems to assume there is nothing to do if the crate version hasn't changed, which is true for dependencies on crates.io, but not for git dependencies where the same crate version can span multiples commits. Maybe this particular skip could be done only for dependencies on crates.io and not for git dependencies. That way, the optimisation is kept for crates.io dependencies which are the most common.

Notes

The skip doesn't happen when the dependency is vendored without suffix, i.e. log.

Version

cargo 1.83.0-nightly (c1fa840a8 2024-08-29)
release: 1.83.0-nightly
commit-hash: c1fa840a85eca53818895901a53fae34247448b2
commit-date: 2024-08-29
host: x86_64-unknown-linux-gnu
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.9.0-DEV (sys:0.4.74+curl-8.9.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Ubuntu 24.4.0 (noble) [64-bit]
@stormshield-guillaumed stormshield-guillaumed added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Sep 10, 2024
@ehuss
Copy link
Contributor

ehuss commented Sep 10, 2024

Thanks for the report! I believe this is a duplicate of #8181 (and probably the same underlying cause as #11897), so closing in favor of that.

@ehuss ehuss closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2024
bors added a commit that referenced this issue Sep 13, 2024
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 added a commit that referenced this issue Sep 14, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

2 participants