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

cargo add package@version shows features from oldest instead of current version #11073

Closed
djc opened this issue Sep 12, 2022 · 7 comments · Fixed by #11075
Closed

cargo add package@version shows features from oldest instead of current version #11073

djc opened this issue Sep 12, 2022 · 7 comments · Fixed by #11075
Labels

Comments

@djc
Copy link
Contributor

djc commented Sep 12, 2022

Problem

When adding cargo add with an explicit minimal version, cargo add will show features from the specific version, while actually adding the latest semver-compatible version. This seems misleading and/or confusing.

Steps

  1. cargo add chrono@0.4

Actual result:

djc-2021 main test-rs $ cargo add chrono@0.4
    Updating crates.io index
      Adding chrono v0.4 to dependencies.
             Features:
             - rustc-serialize
             - serde

Expected result:

    Updating crates.io index
      Adding chrono v0.4.22 to dependencies.
             Features:
             + clock
             + iana-time-zone
             + js-sys
             + oldtime
             + std
             + time
             + wasm-bindgen
             + wasmbind
             + winapi
             - __doctest
             - __internal_bench
             - alloc
             - criterion
             - libc
             - pure-rust-locales
             - rkyv
             - rustc-serialize
             - serde
             - unstable-locales

This is the output from cargo add chrono@0.4.22, which is the version that add chrono@0.4 actually pulls in at this time.

Possible Solution(s)

Bump the version to the latest semver-compatible one before extracting features, instead of after.

Version

cargo 1.65.0-nightly (4ed54cecc 2022-08-27)
release: 1.65.0-nightly
commit-hash: 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5
commit-date: 2022-08-27
host: aarch64-apple-darwin
libgit2: 1.5.0 (sys:0.15.0 vendored)
libcurl: 7.79.1 (sys:0.4.55+curl-7.83.1 system ssl:(SecureTransport) LibreSSL/3.3.6)
os: Mac OS 12.5.1 [64-bit]
@djc djc added the C-bug Category: bug label Sep 12, 2022
@epage
Copy link
Contributor

epage commented Sep 12, 2022

This was intentionally done to point out what features will be safe to use across whatever version of chrono gets selected in the end-user application. Even if your test-rs is a bin with a lock file, that isn't necessarily the case with cargo install

@djc
Copy link
Contributor Author

djc commented Sep 12, 2022

It would be better to make it obvious this is the intended behavior, because it was very surprising as it is. In my mind ideally it would show the union of these lists and make it clear which features are new in the latest version compared to the specified version.

@epage
Copy link
Contributor

epage commented Sep 12, 2022

Any suggestions for how to make this obvious? My main concern is how well it "fits" without adding extra noise.

@djc
Copy link
Contributor Author

djc commented Sep 12, 2022

Maybe a suffix like (in 0.4.22 -- not available in base requirement 0.4) only on those lines where it applies? This limit the noise for most crates that have a stable set of features and clearly repeats the features.

I guess a challenge is that a feature could be disabled by default in 0.4 and enabled in 0.4.22 (the other way around could happen too but probably not so likely?), so that the +/- value should be different across versions.

@epage
Copy link
Contributor

epage commented Sep 12, 2022

I feel like that adds too much noise.

What about something more basic like:

djc-2021 main test-rs $ cargo add chrono@0.4
    Updating crates.io index
      Adding chrono v0.4 to dependencies.
             Features as of v0.4.2:
             - rustc-serialize
             - serde

@djc
Copy link
Contributor Author

djc commented Sep 12, 2022

Well, it would just show the two features, right? Maybe it could at least also have a line saying "(17 more features available in current version 0.4.22)"?

@epage
Copy link
Contributor

epage commented Sep 12, 2022

Yup, copied the wrong one.

That I also lean towards being too much

epage added a commit to epage/cargo that referenced this issue Sep 12, 2022
This gives a hint to users that we might not be showing the feature list
for the latest version but the earliest version.

Also when using a workspace dependency or re-using an existing
dependency, this is a good reminder of what the version requirement is
that was selected.

However, when the user or add (the common case) selected a full
precision requirement, this is redundant.

I'm also mixed on whether the meta version should show up.

Fixes rust-lang#11073
bors added a commit that referenced this issue Sep 13, 2022
fix(add): Clarify which version the features are added for

### What does this PR try to resolve?

This gives a hint to users that we might not be showing the feature list
for the latest version but the earliest version.

Also when using a workspace dependency, this is a good reminder of what the version requirement is that was selected.  That could also be useful for reused dependencies but didn't want to bother with the relevant plumbing for that.

ie we are going from
```console
$ cargo add chrono@0.4
    Updating crates.io index
      Adding chrono v0.4 to dependencies.
             Features:
             - rustc-serialize
             - serde
```
to
```console
$ cargo add chrono@0.4
    Updating crates.io index
      Adding chrono v0.4 to dependencies.
             Features as of v0.4.2:
             - rustc-serialize
             - serde
```

### How should we test and review this PR?

I'd recommend looking at this commit-by-commit.  This is broken up into several refactors leading up the main change.  The refactors are focused on pulling UI logic out of dependency editing so we can more easily evolve the UI without breaking the editing API.  I then tweaked the behavior in the final commit to be less redundant / noisy.

The existing tests are used to demonstrate this is working.

### Additional information

I'm also mixed on whether the meta version should show up.

Fixes #11073
@bors bors closed this as completed in 73906ae Sep 13, 2022
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
This gives a hint to users that we might not be showing the feature list
for the latest version but the earliest version.

Also when using a workspace dependency or re-using an existing
dependency, this is a good reminder of what the version requirement is
that was selected.

However, when the user or add (the common case) selected a full
precision requirement, this is redundant.

I'm also mixed on whether the meta version should show up.

Fixes rust-lang#11073
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
This gives a hint to users that we might not be showing the feature list
for the latest version but the earliest version.

Also when using a workspace dependency or re-using an existing
dependency, this is a good reminder of what the version requirement is
that was selected.

However, when the user or add (the common case) selected a full
precision requirement, this is redundant.

I'm also mixed on whether the meta version should show up.

Fixes rust-lang#11073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants