fix(replace): Partial-version spec support #12806

epage · 2023-10-11T13:32:37Z

What does this PR try to resolve?

#12614 changed package ID specs to allow fields in the version number to be optional. This earliest branch with this change is rust-1.74.0 (beta). While @Eh2406 was investigating version metadata issues in #12772, problems with the partial version change were found

replaces that specify version metadata were ignored (fixed with this PR)
- This also extends out to any other place a PackageIDSpec may show up, like cargo check -p <name>@<spec>
- We explicitly kept the same semantics of version requirements that pre-releases require opt-in. If nothing else, this gives us more room to change semantics in the future if we ever fix the semantics for pre-release.
replaces that don't specify version metadata when the Cargo.lock contained a version metadata, it would previously be ignored (with a warning) but now match (unchanged with this PR)
- When the version metadata in Cargo.lock differed from the overriding Cargo.toml, cargo would panic (now an error in this PR)

With this PR, we are acknowledging that we changed behavior in taking ignored replaces (because of differences with version metadata) and applying them. Seeing as version metadata is relatively rare, replaces are relatively rare, and differences in it for registries is unsupported, the impact seems very small.

The questions before us are

Do we revert feat(pkgid): Allow incomplete versions when unambigious #12614 in master and rust-1.74.0 or merge this PR into master
If we merge this PR into master, do we cherry-pick this into rust-1.74.0 or revert feat(pkgid): Allow incomplete versions when unambigious #12614, giving ourselves more time to find problems

How should we test and review this PR?

The initial commit adds tests that pass as of #12614. Prior to #12614, these tests would have warned that the replace was unused and failed because bar::bar didn't exist. Each commit then changes the behavior (or not) and updates the corresponding test.

Additional information

rustbot · 2023-10-11T13:32:43Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

Eh2406 · 2023-10-11T19:15:02Z

Thank you for the quick fix!

ehuss

From what I can tell, this fixes much more than just [replace], right?

For example, any command that takes a Package ID Spec (like cargo build -p foo@1.2.3+123), which on 1.73 (stable) will error if the metadata doesn't match, on beta it ignores it, and this PR fixes it to again error if it doesn't match, correct?

src/cargo/core/resolver/dep_cache.rs

src/cargo/core/package_id_spec.rs

src/cargo/util/semver_ext.rs

ehuss · 2023-10-15T23:25:23Z

I'd be fine with backporting this to beta, since there is still about a month left. (Though reverting #12614 would also be fine, but I would lean towards just fixing it.)

We already tested with missing minor/patch

PR rust-lang#12614 - Changed ignored replaces to not-ignored - Has bugs in matching replaces

epage · 2023-10-16T16:21:34Z

From what I can tell, this fixes much more than just [replace], right?

For example, any command that takes a Package ID Spec (like cargo build -p foo@1.2.3+123), which on 1.73 (stable) will error if the metadata doesn't match, on beta it ignores it, and this PR fixes it to again error if it doesn't match, correct?

Yes, one of the bugs identifier and fixed in this (allowing different build metadata to still match) affects PackageIDSpecs everywhere. I've updated the description to call this out.

In adding the unit tests, the fix for the above made it so foo@1.2 will now match foo@1.2.3-alpha.0. I'll call that out in the description as well so we can more explicitly decide if we want to special case pre-release or not.

epage · 2023-10-17T15:14:17Z

We talked about this in the cargo team meeting. We were fine with the change in
behavior and with backporting this

This required a cargo expert to detect the bug and was not found through users using nightly or beta
With package id specs and replaces (deprecated) combined with build metadata and pre-release, the bast radius if a bug is fairly small and we can live with it broken in a release
Though we were probably comfortable with 1.2.3 matching 1.2.3-alpha.0, we decided to require opting into pre-releases, like version requirements, so we have more room for changing things if someone takes a broad view of pre-release and works to solve it. d318ed4 has this change

Eh2406 · 2023-10-18T15:41:17Z

Looks good, and if we missed something then new PR.
@bors r+

bors · 2023-10-18T15:41:19Z

📌 Commit 1e34066 has been approved by Eh2406

It is now in the queue for this repository.

bors · 2023-10-18T15:42:26Z

⌛ Testing commit 1e34066 with merge 67271fd...

bors · 2023-10-18T16:28:00Z

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 67271fd to master...

[beta-1.74.0] Partial-version spec support Beta backports: - #12806 In order to make CI pass, the following PRs are also cherry-picked:

[beta-1.74.0] Partial-version spec support Beta backports: - #12806 In order to make CI pass, the following PRs are also cherry-picked: - f0d3cdf from #12800

Update cargo 22 commits in 8eb8acbb116e7923ea2ce33a50109933ed5ab375..d2f6a048529eb8e9ebc55d793abd63456c98fac2 2023-10-17 11:55:04 +0000 to 2023-10-20 18:25:30 +0000 - chore(deps): bump rustix from 0.38.18 to 0.38.19 (rust-lang/cargo#12851) - refactor: centralize logic of getting max resolve version (rust-lang/cargo#12860) - If there's a version in the lock file only use that exact version (rust-lang/cargo#12772) - Make the precise field of a source an Enum (rust-lang/cargo#12849) - fix(cli): Provide next steps for bad -Z flag (rust-lang/cargo#12857) - fix(remove): Preserve feature comments (rust-lang/cargo#12837) - docs: fix typo (rust-lang/cargo#12844) - chore(triagebot): auto label when PR review state changes (rust-lang/cargo#12856) - fix(add): Preserve more comments (rust-lang/cargo#12838) - ci: big ⚠️ to ensure the CNAME file is always there (rust-lang/cargo#12853) - docs(cargo-bench): `--bench` is passed in unconditionally to bench harnesses (rust-lang/cargo#12850) - docs(contrib): generate redirection HTML pages in CI (rust-lang/cargo#12846) - docs: remove review capacity notice (rust-lang/cargo#12842) - fix(help):Clarify install's positional (rust-lang/cargo#12841) - Adjust `-Zcheck-cfg` for new rustc syntax and behavior (rust-lang/cargo#12845) - fix(replace): Partial-version spec support (rust-lang/cargo#12806) - Print environment variables for build script executions with `-vv` (rust-lang/cargo#12829) - fix(cli): Suggest cargo-search on bad commands (rust-lang/cargo#12840) - docs(contrib): Policy on manifest editing (rust-lang/cargo#12836) - ci/contrib: use separate concurrency group (rust-lang/cargo#12835) - ci/contrib: do not fail on missing gh-pages (rust-lang/cargo#12834) - Clarify flag behavior in `cargo remove --help` (rust-lang/cargo#12823) r? ghost

rustbot assigned ehuss Oct 11, 2023

rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-semver Area: semver specifications, version matching, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2023

epage force-pushed the replace branch from 8c56b7f to 6fce7a3 Compare October 11, 2023 14:09

epage mentioned this pull request Oct 11, 2023

feat(pkgid): Allow incomplete versions when unambigious #12614

Merged

epage force-pushed the replace branch from 6fce7a3 to 6f28a86 Compare October 11, 2023 17:54

epage added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Oct 11, 2023

ehuss reviewed Oct 15, 2023

View reviewed changes

src/cargo/core/resolver/dep_cache.rs Outdated Show resolved Hide resolved

src/cargo/core/resolver/dep_cache.rs Outdated Show resolved Hide resolved

src/cargo/core/package_id_spec.rs Show resolved Hide resolved

src/cargo/util/semver_ext.rs Show resolved Hide resolved

epage force-pushed the replace branch 3 times, most recently from 48ebcaf to 6714191 Compare October 16, 2023 15:54

epage added 3 commits October 16, 2023 11:03

test(spec): Make room for more tests

2fc644a

test(spec): Cover meta/pre cases for partial versions

1bf12a5

We already tested with missing minor/patch

test(replace): Show behavior post-rust-lang#12614

ae068fb

PR rust-lang#12614 - Changed ignored replaces to not-ignored - Has bugs in matching replaces

epage force-pushed the replace branch 2 times, most recently from 9c5b959 to 79d884e Compare October 16, 2023 16:17

Eh2406 mentioned this pull request Oct 16, 2023

If there's a version in the lock file only use that exact version #12772

Merged

epage added 4 commits October 17, 2023 10:09

fix(spec): Ensure PackageIdSpec respects version 'build' field

0ec3274

fix(spec): Require opt-in for pre-release

d318ed4

fix(replace): Error, rather than assert, on version mismatch

7502318

test(replace): Clarify name for remaining new test

1e34066

epage force-pushed the replace branch from 79d884e to 1e34066 Compare October 17, 2023 15:09

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 Oct 18, 2023

bors merged commit 67271fd into rust-lang:master Oct 18, 2023
20 checks passed

epage deleted the replace branch October 18, 2023 16:43

epage mentioned this pull request Oct 18, 2023

[beta-1.74.0] Partial-version spec support #12847

Merged

bors added a commit that referenced this pull request Oct 18, 2023

Auto merge of #12847 - epage:backport, r=weihanglo

e84bb5d

[beta-1.74.0] Partial-version spec support Beta backports: - #12806 In order to make CI pass, the following PRs are also cherry-picked:

bors added a commit that referenced this pull request Oct 18, 2023

Auto merge of #12847 - epage:backport, r=weihanglo

ecb9851

[beta-1.74.0] Partial-version spec support Beta backports: - #12806 In order to make CI pass, the following PRs are also cherry-picked: - f0d3cdf from #12800

weihanglo mentioned this pull request Oct 20, 2023

Update cargo rust-lang/rust#117000

Merged

ehuss added this to the 1.75.0 milestone Nov 6, 2023

ehuss removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Dec 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(replace): Partial-version spec support #12806

fix(replace): Partial-version spec support #12806

epage commented Oct 11, 2023 •

edited

Loading

rustbot commented Oct 11, 2023

Eh2406 commented Oct 11, 2023

ehuss left a comment •

edited

Loading

ehuss commented Oct 15, 2023

epage commented Oct 16, 2023

epage commented Oct 17, 2023

Eh2406 commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

fix(replace): Partial-version spec support #12806

fix(replace): Partial-version spec support #12806

Conversation

epage commented Oct 11, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Oct 11, 2023

Eh2406 commented Oct 11, 2023

ehuss left a comment • edited Loading

Choose a reason for hiding this comment

ehuss commented Oct 15, 2023

epage commented Oct 16, 2023

epage commented Oct 17, 2023

Eh2406 commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

bors commented Oct 18, 2023

epage commented Oct 11, 2023 •

edited

Loading

ehuss left a comment •

edited

Loading