feat(yank): Support foo@version like cargo-add #10597

epage · 2022-04-25T13:49:38Z

What does this PR try to resolve?

In #10472, cargo-add was merged with support for an inline version
syntax of cargo add foo@version. That also served as the change proposal for
extending that syntax to cargo yank for convenience and consistency.

How should we test and review this PR?

Documentation updates are split into their own commit to not clog up browsing the code.

The ops API is generic enough that this is implemented purely in the command.

For now, the foo@version syntax parser is being left in the command, rather than being shared, as we see how the behavior of these different parsers diverge for their target needs to see what makes sense for refactoring. See also The Rule of Three

This doesn't use the full pkgid syntax (modified in Extend pkgid syntax with @ support #10582) because that allows syntax that is unsupported here.
This doesn't use cargo-adds parser because that is for version reqs.

Tests were added for various combinations of flags and behavior.

Additional information

The major difference is that cargo-add is specifying a version-req
while cargo-yank is specifying a version. This was originally discussed on zulip and there seemed to be a desire to have one syntax rather than the user thinking about a syntax per type of version (which users won't always think about). See also #10582 which extended the pkgid spec syntax and has some more discussion on this general trend.

cargo-install will be updated in a subsequent PR.

In rust-lang#10472, cargo-add was merged with support for an inline version syntax of `foo@version`. That also served as the change proposal for extending that syntax to `cargo yank` for convinience and consistency. The major difference is that `cargo-add` is specifying a version-req while `cargo-yank` is specifying a version. This doesn't use the full `pkgid` syntax because that allows syntax that is unsupported here. This doesn't use `cargo-add`s parser because that is for version reqs.

rust-highfive · 2022-04-25T13:49:42Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

src/doc/man/generated_txt/cargo-yank.txt

arlosi · 2022-04-25T15:17:32Z

src/bin/cargo/commands/yank.rs

    ops::yank(
        config,
-        args.value_of("crate").map(|s| s.to_string()),
-        args.value_of("version").map(|s| s.to_string()),
+        krate.map(|s| s.to_string()),


Could we remove the .map(|s| s.to_string()) for all these arguments and make ops::yank take Option<&str> instead?

imo

this is out of scope for this PR

the use of String is wider than just yank, its most Op options (though sometimes we do see a &str or a mix of String and &str)

I personally find this a point I don't have a strong preference on besides consistency. String is slightly more ergonomic though that is diminished by having a &Config in the Options and no one will ever notice the performance difference in these (thinking back to the RustConf talk about "just clone and stop worrying about it).

ehuss · 2022-05-10T17:17:38Z

Thanks, looks good! Sorry about the delay.

I'm going to go ahead and approve without FCP since we had an FCP in #10582 and had discussions in our team meetings about moving to this syntax. If anyone has concerns, feel free to raise them and we can revert.

@bors r+

bors · 2022-05-10T17:17:39Z

📌 Commit 6f78fdce959def9d40deee2b454a2d9b00735ef5 has been approved by ehuss

bors · 2022-05-10T17:29:43Z

⌛ Testing commit 6f78fdce959def9d40deee2b454a2d9b00735ef5 with merge d2abb00f93eea9ad91cf87a6ef817353d3742600...

bors · 2022-05-10T17:43:51Z

💔 Test failed - checks-actions

epage · 2022-05-10T18:10:25Z

Looks like #10582 conflicted with this. Tests are now updated.

ehuss · 2022-05-10T19:10:29Z

@bors r+

bors · 2022-05-10T19:10:30Z

📌 Commit 790e4ce has been approved by ehuss

bors · 2022-05-10T19:10:36Z

⌛ Testing commit 790e4ce with merge 59ccffc08fbb9a0dfac6a613d64b8ce2492a1419...

bors · 2022-05-10T19:33:41Z

💔 Test failed - checks-actions

epage · 2022-05-10T19:48:36Z

@ehuss I'm not recognizing this failure, doesn't look related, and can't reproduce it locally.

 ---- features2::pm_with_int_shared stdout ----
running `/Users/runner/work/cargo/cargo/target/debug/cargo build --workspace --all-targets --all-features -v`
running `/Users/runner/work/cargo/cargo/target/debug/cargo build --workspace --all-targets --all-features -v`
thread 'features2::pm_with_int_shared' panicked at '
test failed running `/Users/runner/work/cargo/cargo/target/debug/cargo build --workspace --all-targets --all-features -v`
error: Expected lines did not match (ignoring order):
0   3            Fresh shared v0.1.0 (/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/shared)
1        -       Fresh pm [..]
2        -       Fresh foo [..]
3   0         Finished dev [unoptimized + debuginfo] target(s) in 0.49s
    2    +   Compiling pm v0.1.0 (/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/pm)
    3    +   Compiling foo v0.1.0 (/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/foo)
    4    +     Running `rustc --crate-name pm_test pm/tests/pm_test.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --emit=dep-info,link -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 --test -C metadata=b6d57ebff39aeea5 -C extra-filename=-b6d57ebff39aeea5 --out-dir /Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps -L dependency=/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps --extern pm=/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps/libpm-1964bb76bedd689d.dylib --extern shared=/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps/libshared-0cf057ef93f8497d.rlib`
    5    +     Running `rustc --crate-name foo --edition=2018 foo/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --emit=dep-info,link -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 --test -C metadata=d58953c7da7c68b4 -C extra-filename=-d58953c7da7c68b4 --out-dir /Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps -L dependency=/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps --extern pm=/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps/libpm-1964bb76bedd689d.dylib --extern shared=/Users/runner/work/cargo/cargo/target/tmp/cit/t1009/foo/target/debug/deps/libshared-0cf057ef93f8497d.rlib`

', tests/testsuite/features2.rs:2279:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

ehuss · 2022-05-10T21:53:50Z

I'm not sure what went wrong, I haven't seen that test fail before. It looks like there was a fingerprint issue. It's definitely not related to this PR.

I'm having a hard time trying to come up with any theories as to why it failed. I ran some stress tests, and couldn't get it to fail. 🤷

@bors retry

bors · 2022-05-10T21:53:57Z

⌛ Testing commit 790e4ce with merge dd53196...

bors · 2022-05-10T22:54:28Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing dd53196 to master...

feat(install): Support `foo@version` like cargo-add ### What does this PR try to resolve? This aims to make `cargo install` consistent with - `cargo add foo@version` from #10472 - pkgid changes in #10582 - `cargo yank foo@version` from #10597 It also offers a shorthand for people installing a specific version. ### How should we test and review this PR? #10582 acted as the FCP for this, see #10597 Documentation updates are split into their own commit to not clog up browsing the code. Examine the tests to see if they make sense ### Additional information While the `foo@vewrsion` syntax is the same, each's semantics are different. We had decided it was better to have the same syntax with different semantics than having the user worry about what syntax they use where. In `cargo install`s case, it has an implicit-but-required `=` operand while `cargo-add` allows any operand. This doesn't use the full `pkgid` syntax because that allows syntax that is unsupported here. This doesn't use `cargo-add`s parser because that is for version reqs. I held off on reusing the parser from `cargo-yank` because they had different type system needs and the level of duplication didn't seem worth it (see Rule of Three).

Update cargo 20 commits in a44758ac805600edbb6ba51e7e6fb81a6077c0cd..3f052d8eed98c6a24f8b332fb2e6e6249d12d8c1 2022-05-04 02:29:34 +0000 to 2022-05-12 15:19:04 +0000 - pre-stabilization documentation for workspace inheritance (rust-lang/cargo#10659) - test: Make curr_dir work in/out of workspace (rust-lang/cargo#10658) - Fix no_cross_doctests race condition. (rust-lang/cargo#10660) - Fix typo (rust-lang/cargo#10657) - feat(install): Support `foo@version` like cargo-add (rust-lang/cargo#10650) - fix typos found by the `typos-cli` crate (rust-lang/cargo#10649) - feat(yank): Support foo@version like cargo-add (rust-lang/cargo#10597) - add `cargo-features` to unstable docs for workspace inheritance (rust-lang/cargo#10648) - Use the traits added to the Rust 2021 Edition prelude (rust-lang/cargo#10646) - Pass `--target` to `rustdoc` for `cargo test` if specified with host target. (rust-lang/cargo#10594) - Fix use of .. in dep-info-basedir (rust-lang/cargo#10281) - fix some typos (rust-lang/cargo#10639) - Move snapshot tests into testsuite (rust-lang/cargo#10638) - Improve support of condition compilation checking (rust-lang/cargo#10566) - When documenting private items in a binary, ignore warnings about links to private items (rust-lang/cargo#10142) - Extend pkgid syntax with ``@`` support (rust-lang/cargo#10582) - move one `snapshot/add` test into `testsuite/cargo_add/` (rust-lang/cargo#10631) - Add caveat for covering features (rust-lang/cargo#10605) - Improve CARGO_ENCODED_RUSTFLAGS and CARGO_ENCODED_RUSTDOCFLAGS variables docs (rust-lang/cargo#10633) - reorganize `snapshot` tests to better work in contexts that sort by extension (rust-lang/cargo#10629)

rust-highfive assigned ehuss Apr 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2022

arlosi reviewed Apr 25, 2022

View reviewed changes

doc(yank): Tell users about foo@version syntax

790e4ce

epage force-pushed the yank branch from 6e75913 to a0a95ff Compare April 25, 2022 16:58

arlosi approved these changes Apr 25, 2022

View reviewed changes

epage force-pushed the yank branch 2 times, most recently from 8901a94 to 6f78fdc Compare May 2, 2022 19:02

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 May 10, 2022

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 May 10, 2022

epage force-pushed the yank branch from 6f78fdc to 790e4ce Compare May 10, 2022 18: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 May 10, 2022

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 May 10, 2022

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 May 10, 2022

bors merged commit dd53196 into rust-lang:master May 10, 2022

epage deleted the yank branch May 11, 2022 00:08

epage mentioned this pull request May 11, 2022

feat(install): Support foo@version like cargo-add #10650

Merged

ehuss mentioned this pull request May 12, 2022

Update cargo rust-lang/rust#96984

Merged

ehuss added this to the 1.62.0 milestone May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(yank): Support foo@version like cargo-add #10597

feat(yank): Support foo@version like cargo-add #10597

epage commented Apr 25, 2022

rust-highfive commented Apr 25, 2022

arlosi Apr 25, 2022

epage Apr 25, 2022

ehuss commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

epage commented May 10, 2022

ehuss commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

epage commented May 10, 2022

ehuss commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

feat(yank): Support foo@version like cargo-add #10597

feat(yank): Support foo@version like cargo-add #10597

Conversation

epage commented Apr 25, 2022

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented Apr 25, 2022

arlosi Apr 25, 2022

Choose a reason for hiding this comment

epage Apr 25, 2022

Choose a reason for hiding this comment

ehuss commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

epage commented May 10, 2022

ehuss commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

epage commented May 10, 2022

ehuss commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022