Extend pkgid syntax with `@` support #10582

epage · 2022-04-19T21:19:15Z

In addition to foo:1.2.3, we now support [email protected] for pkgids. We
are also making it the default way of rendering pkgid's for the user.

What does this PR try to resolve?

With cargo-add in #10472, we've decided to only use @ in it and to add
it as an alternative to : in the rest of cargo. cargo-add
originally used @. When preparing it for merge, I switched to : to
be consistent with pkgids. When discussing this, it was felt @ has
precedence in too many tools to switch to : but that we should instead
switch pkgid's to use @, in a backwards compatible way. #10472 served
as the change proposal for this

How should we test and review this PR?

The focus of the testing is on the parsers unit tests and on the end-to-end output. We are not explicitly testing end-to-end input in this PR, assuming the unit tests are sufficient.

Additional information

This only focuses on places we already accept pkgids. Looking into supporting [email protected] in cargo install and cargo yank is being left for a future PR.

In addition to `foo:1.2.3`, we now support `[email protected]` for pkgids. We are also making it the default way of rendering pkgid's for the user. With cargo-add in rust-lang#10472, we've decided to only use `@` in it and to add it as an alternative to `:` in the rest of cargo. `cargo-add` originally used `@`. When preparing it for merge, I switched to `:` to be consistent with pkgids. When discussing this, it was felt `@` has precedence in too many tools to switch to `:` but that we should instead switch pkgid's to use `@`, in a backwards compatible way. See also https://internals.rust-lang.org/t/feedback-on-cargo-add-before-its-merged/16024/26?u=epage

rust-highfive · 2022-04-19T21:19:18Z

r? @ehuss

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

ehuss · 2022-04-19T22:22:57Z

I seem to recall that pkg@vers was suitable when vers is a version requirement, but in package IDs they should be precisely specified. Maybe I'm a little confused, but is this related to Package IDs or Package ID Specs? Assuming the later, does it make sense to have @ behave as a requirement?

epage · 2022-04-20T02:21:16Z

I seem to recall that pkg@vers was suitable when vers is a version requirement, but in package IDs they should be precisely specified. Maybe I'm a little confused, but is this related to Package IDs or Package ID Specs? Assuming the later, does it make sense to have @ behave as a requirement?

My intention is to refer to Package ID Specs (as a user, I think of them being one in the same because of things like cargo pkgid and I forget that internally we have them as different concepts or that the --help refers to them as "specs").

Right now we have

cargo build -p foo:version
cargo add foo@version-req
cargo yank foo --vers version
cargo install foo --version version-req where the version req has an implicit-but-required = operand.

My intention with the change proposal embedded in #10472 was for this to change to

cargo build -p foo@version
cargo add foo@version-req
cargo yank foo@version
cargo install foo@version-req where the version req has an implicit-but-required = operand.

Though these are all slightly different, I think they make sense in context and aren't any more confusing then say the different meanings of the --version flag for yank and install.

Most of the original discussion on changing this happened on Zulip and I'd recommend catching up on that thread to get some more background on this.

Some highlights:

@Eh2406 (referring to a downside to being consistent)

We will have switching costs somewhere:

@joshtriplett (referring to soft-deprecating : in favor of @)

Yeah, I don't want anyone having to think about "what kind of version is this".

alexcrichton

in any case changing the pkgid syntax is pretty easy to do

alexcrichton

I actually really like the idea of cargo install [email protected] instead of cargo install --version 1.2.3 foo

ehuss · 2022-04-25T18:22:09Z

OK, seems reasonable to me. I don't have a strong preference one way or the other, but @ does seem to be a little more prevalent. It does incur a little churn. For example, any documentation out on the web using the old syntax may cause confusion for new users. However, I suspect content using the : syntax is probably pretty rare.

One concern I was thinking is if we want to change the "exact version" vs "version requirement" behavior in the future. For example, this PR makes it so that I can run cargo update -p [email protected] which requires an exact matching version. But let's say in the future we want to allow a partial version req like cargo update -p [email protected]. I think that should be safe to do? I can't immediately think of any back-compat hazards.

@rust-lang/cargo I'm going to go ahead and propose this to merge.

@rfcbot fcp merge

rfcbot · 2022-04-25T18:22:10Z

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

epage · 2022-04-25T18:27:23Z

One concern I was thinking is if we want to change the "exact version" vs "version requirement" behavior in the future. For example, this PR makes it so that I can run cargo update -p [email protected] which requires an exact matching version. But let's say in the future we want to allow a partial version req like cargo update -p [email protected]. I think that should be safe to do? I can't immediately think of any back-compat hazards.

Yes, that'd be safe to do and would make it more like cargo install.

The main concern is if we'd want to allow a command that accepts pkgid specs today to accept hex@^0.3

rfcbot · 2022-04-25T20:20:18Z

🔔 This is now entering its final comment period, as per the review above. 🔔

weihanglo · 2022-04-26T15:50:31Z

Would you like to update docs with this PR or a follow-up?

Also, in terms of display, are we going to switch to [email protected] universally in Cargo? Such as yankand future-compat.

Since the `cargo pkgid` docs were so brief, I figured I'd focus on the expected common case and only cover `@` while the reference mentions both in the grammar.

epage · 2022-04-26T19:17:11Z

Would you like to update docs with this PR or a follow-up?

https://doc.rust-lang.org/cargo/reference/pkgid-spec.html

https://doc.rust-lang.org/cargo/commands/cargo-pkgid.html

Thanks, forgot to do that somehow

Also, in terms of display, are we going to switch to [email protected] universally in Cargo? Such as yankand future-compat.

Missed those, thanks. Updating them.

epage · 2022-04-26T19:49:05Z

@weihanglo I've updated all of the places I could find pkgid spec's being manually rendered and modified them to make them more obvious.

epage · 2022-04-26T21:24:26Z

src/cargo/core/compiler/future_incompat.rs

    for per_package in per_package_reports {
        let package_spec = format!(
-            "{}:{}",
+            "{}@{}",
            per_package.package_id.name(),
            per_package.package_id.version()
        );


And with the following line:

let rendered = report.entry(package_spec).or_default();

This changes how the reports are created on disk. The report future-incompat --package flag doesn't take a package-spec but whatever was put into the report here.

We could just keep this as : but then report future-incompat will only accept :

We could have report future-incompat try to generate both formats

Any thoughts? @joshtriplett since I think you had worked on this

@epage We discussed this in the Cargo meeting; summary: go ahead and change it, don't worry about : at all. There's a format version number that you could bump so that old Cargo doesn't think it can read this.

I've not dug further but from what I've seen of the code and failure modes, I don't think we even need to bump the format version number. It looks like report future-incompat is completely neutral to the pkgid spec format. It accepts a parameter and uses it as a key. You should be able to have old cargo's read the file.

epage · 2022-04-26T21:25:41Z

tests/testsuite/future_incompat_report.rs

            .run();

-        p.cargo("report future-incompatibilities").arg("--package").arg("first-dep:0.0.1")
+        p.cargo("report future-incompatibilities").arg("--package").arg("first-dep@0.0.1")


This test failed with : until I updated it because the package id is being written to the report.

rfcbot · 2022-05-05T20:22:05Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

ehuss · 2022-05-06T19:07:01Z

Thanks!

@bors r+

bors · 2022-05-06T19:07:03Z

📌 Commit 709f1be has been approved by ehuss

bors · 2022-05-06T19:07:10Z

⌛ Testing commit 709f1be with merge 0f75da6...

bors · 2022-05-06T19:48:56Z

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

feat(yank): Support foo@version like cargo-add ### 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 #10582) because that allows syntax that is unsupported here. - This doesn't use `cargo-add`s 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](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Multiple.20ways.20of.20specifying.20versions) 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.

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 19, 2022

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

epage force-pushed the pkgid branch from eec3dc6 to f24122b Compare April 25, 2022 13:41

epage mentioned this pull request Apr 25, 2022

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

Merged

ehuss added the T-cargo Team: Cargo label Apr 25, 2022

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Apr 25, 2022

rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Apr 25, 2022

Document the use of @ in PKGID Specs

709f1be

Since the `cargo pkgid` docs were so brief, I figured I'd focus on the expected common case and only cover `@` while the reference mentions both in the grammar.

epage force-pushed the pkgid branch from db25f49 to 7abf648 Compare April 26, 2022 19:48

epage force-pushed the pkgid branch 2 times, most recently from 62f422d to b540c6d Compare April 26, 2022 21:19

epage commented Apr 26, 2022

View reviewed changes

epage force-pushed the pkgid branch 2 times, most recently from 75a83c3 to 709f1be Compare May 2, 2022 19:02

rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels May 5, 2022

rfcbot added the to-announce label May 5, 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 6, 2022

bors merged commit 0f75da6 into rust-lang:master May 6, 2022

epage deleted the pkgid branch May 6, 2022 20:07

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

epage mentioned this pull request Jul 5, 2023

Support versions starting with v in cargo add #12331

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend pkgid syntax with `@` support #10582

Extend pkgid syntax with `@` support #10582

epage commented Apr 19, 2022 •

edited

Loading

rust-highfive commented Apr 19, 2022

ehuss commented Apr 19, 2022

epage commented Apr 20, 2022

ehuss commented Apr 25, 2022

rfcbot commented Apr 25, 2022 •

edited by joshtriplett

Loading

epage commented Apr 25, 2022

rfcbot commented Apr 25, 2022

weihanglo commented Apr 26, 2022

epage commented Apr 26, 2022

epage commented Apr 26, 2022

epage Apr 26, 2022

joshtriplett May 3, 2022

epage May 3, 2022

epage Apr 26, 2022

rfcbot commented May 5, 2022

ehuss commented May 6, 2022

bors commented May 6, 2022

bors commented May 6, 2022

bors commented May 6, 2022

Extend pkgid syntax with @ support #10582

Extend pkgid syntax with @ support #10582

Conversation

epage commented Apr 19, 2022 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented Apr 19, 2022

ehuss commented Apr 19, 2022

epage commented Apr 20, 2022

ehuss commented Apr 25, 2022

rfcbot commented Apr 25, 2022 • edited by joshtriplett Loading

epage commented Apr 25, 2022

rfcbot commented Apr 25, 2022

weihanglo commented Apr 26, 2022

epage commented Apr 26, 2022

epage commented Apr 26, 2022

epage Apr 26, 2022

Choose a reason for hiding this comment

joshtriplett May 3, 2022

Choose a reason for hiding this comment

epage May 3, 2022

Choose a reason for hiding this comment

epage Apr 26, 2022

Choose a reason for hiding this comment

rfcbot commented May 5, 2022

ehuss commented May 6, 2022

bors commented May 6, 2022

bors commented May 6, 2022

bors commented May 6, 2022

Extend pkgid syntax with `@` support #10582

Extend pkgid syntax with `@` support #10582

epage commented Apr 19, 2022 •

edited

Loading

rfcbot commented Apr 25, 2022 •

edited by joshtriplett

Loading