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

feat: Report some dependency changes on any command #13561

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Mar 8, 2024

What does this PR try to resolve?

The goal is to help users be informed when they are held back by either MSRV or semver.

Fixes #13539

How should we test and review this PR?

Additional information

@epage epage marked this pull request as draft March 8, 2024 13:51
@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-lockfile Area: Cargo.lock issues Command-generate-lockfile S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2024
@epage epage changed the title feat: Report some dependency changes on any command WIP: feat: Report some dependency changes on any command Mar 8, 2024
@epage
Copy link
Contributor Author

epage commented Mar 8, 2024

Test failures will show what the proposed output is

bors added a commit that referenced this pull request Mar 12, 2024
refactor(lockfile): Make diffing/printing more reusable

### What does this PR try to resolve?

This is prep for #13561 so we can tailor the printing of lockfile changes to each use without a bunch of flags.

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

### Additional information
@bors
Copy link
Contributor

bors commented Mar 12, 2024

☔ The latest upstream changes (presumably #13564) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added the A-testing-cargo-itself Area: cargo's tests label Mar 19, 2024
@epage epage changed the title WIP: feat: Report some dependency changes on any command feat: Report some dependency changes on any command Mar 19, 2024
@epage epage marked this pull request as ready for review March 19, 2024 01:09
@epage epage force-pushed the msrv-report branch 4 times, most recently from 9109d92 to 609ad02 Compare March 19, 2024 15:12
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through every file changes 💀. Thanks!

tests/testsuite/rust_version.rs Show resolved Hide resolved
@@ -417,7 +417,7 @@ fn frequency() {
.env("CARGO_GC_AUTO_FREQUENCY", "1 day")
.masquerade_as_nightly_cargo(&["gc"])
.run();
assert_eq!(get_index_names().len(), 0);
assert_eq!(get_index_names().len(), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess this is a consequence of cargo querying the latest versions for displaying lockfile changes after the build, while auto-gc was run ahead of the build.

Seems like a waste of energy but in real world that rarely happens and would be fine 🙆🏾‍♂️.

src/cargo/ops/lockfile.rs Show resolved Hide resolved
src/cargo/ops/cargo_generate_lockfile.rs Show resolved Hide resolved
src/cargo/ops/cargo_generate_lockfile.rs Show resolved Hide resolved
@weihanglo
Copy link
Member

This PR includes a UI change that prints a Locking status when any command modifies the lockfile, along with showing the latest version if any updated dependency in use is behind (or outdated, though it is way too strong a word).

The Cargo team has discussed and considered it as a two-way-door change so I'm gonna merge this.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 20, 2024

📌 Commit 4ab2797 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 20, 2024
@bors
Copy link
Contributor

bors commented Mar 20, 2024

⌛ Testing commit 4ab2797 with merge 266a5ef...

@bors
Copy link
Contributor

bors commented Mar 20, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 266a5ef to master...

@bors bors merged commit 266a5ef into rust-lang:master Mar 20, 2024
21 checks passed
@epage epage deleted the msrv-report branch March 20, 2024 14:45
epage added a commit to epage/cargo that referenced this pull request Mar 20, 2024
This is part of rust-lang#9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but
leaves validation up to the user as the resolver will pick newer
versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537 though it could be confusing to the workflow with an
MSRV-compatible lockfile.
PR rust-lang#13561 at least raises awareness of that discrepancy.

There is an unresolved question on differences in the resolver vs
`cargo add` for dealing with an unset `rust-version`.
However, we are only stabilizing the `cargo add` side which is a very
light two-way door as this is a UX-focused command and not a
programmatic command.

This hasn't gotten much end-user testing but, as its UX focused, that
seems fine.

As such, this seems like it is ready to stabilize.
epage added a commit to epage/cargo that referenced this pull request Mar 20, 2024
This is part of rust-lang#9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but
leaves validation up to the user as the resolver will pick newer
versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537 though it could be confusing to the workflow with an
MSRV-compatible lockfile.
PR rust-lang#13561 at least raises awareness of that discrepancy.

There is an unresolved question on differences in the resolver vs
`cargo add` for dealing with an unset `rust-version`.
However, we are only stabilizing the `cargo add` side which is a very
light two-way door as this is a UX-focused command and not a
programmatic command.

This hasn't gotten much end-user testing but, as its UX focused, that
seems fine.

As such, this seems like it is ready to stabilize.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2024
Update cargo

13 commits in d438c80c45c24be676ef5867edc79d0a14910efe..a510712d05c6c98f987af24dd73cdfafee8922e6
2024-03-19 16:11:22 +0000 to 2024-03-25 03:45:54 +0000
- Remove unnecessary test (rust-lang/cargo#13637)
- Use `gitoxide` for `list_files_git` (rust-lang/cargo#13592)
- fix: Warn on -Zlints (rust-lang/cargo#13632)
- feat: Add a basic linting system (rust-lang/cargo#13621)
- docs: remove untrue TODO for `native_dirs` (rust-lang/cargo#13631)
- refactor(testsuite): Rename lints to lints_table (rust-lang/cargo#13627)
- Fix debuginfo strip when using `--target` (rust-lang/cargo#13618)
- refactor(toml): Push diagnostic complexity on annotate-snippets (rust-lang/cargo#13619)
- Fix publish script due to crates.io CDN change (rust-lang/cargo#13614)
- fix(alias): dont panic when resolving an empty alias (rust-lang/cargo#13613)
- Update annotate snippets (rust-lang/cargo#13609)
- refactor(vendor): tiny not important refactors (rust-lang/cargo#13610)
- feat: Report some dependency changes on any command (rust-lang/cargo#13561)

r? ghost
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 26, 2024
Update cargo

13 commits in d438c80c45c24be676ef5867edc79d0a14910efe..a510712d05c6c98f987af24dd73cdfafee8922e6
2024-03-19 16:11:22 +0000 to 2024-03-25 03:45:54 +0000
- Remove unnecessary test (rust-lang/cargo#13637)
- Use `gitoxide` for `list_files_git` (rust-lang/cargo#13592)
- fix: Warn on -Zlints (rust-lang/cargo#13632)
- feat: Add a basic linting system (rust-lang/cargo#13621)
- docs: remove untrue TODO for `native_dirs` (rust-lang/cargo#13631)
- refactor(testsuite): Rename lints to lints_table (rust-lang/cargo#13627)
- Fix debuginfo strip when using `--target` (rust-lang/cargo#13618)
- refactor(toml): Push diagnostic complexity on annotate-snippets (rust-lang/cargo#13619)
- Fix publish script due to crates.io CDN change (rust-lang/cargo#13614)
- fix(alias): dont panic when resolving an empty alias (rust-lang/cargo#13613)
- Update annotate snippets (rust-lang/cargo#13609)
- refactor(vendor): tiny not important refactors (rust-lang/cargo#13610)
- feat: Report some dependency changes on any command (rust-lang/cargo#13561)

r? ghost
epage added a commit to epage/cargo that referenced this pull request Apr 6, 2024
This is part of rust-lang#9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but
leaves validation up to the user as the resolver will pick newer
versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537 though it could be confusing to the workflow with an
MSRV-compatible lockfile.
PR rust-lang#13561 at least raises awareness of that discrepancy.

There is an unresolved question on differences in the resolver vs
`cargo add` for dealing with an unset `rust-version`.
However, we are only stabilizing the `cargo add` side which is a very
light two-way door as this is a UX-focused command and not a
programmatic command.

This hasn't gotten much end-user testing but, as its UX focused, that
seems fine.

As such, this seems like it is ready to stabilize.
bors added a commit that referenced this pull request Apr 8, 2024
feat(add): Stabilize MSRV-aware version req selection

### What does this PR try to resolve?

This is part of #9930 for rust-lang/rfcs#3537

This will make it easier to maintain an MSRV-compliant `Cargo.toml` but leaves validation up to the user as the resolver will pick newer versions.
This helps the MSRV-aware workflows enumerated in
rust-lang/rfcs#3537

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

As for determining if this is ready for stabilization:

By stabilizing this without the MSRV-aware resolver, this could be confusing to the workflow with an MSRV-compatible lockfile.
PR #13561 at least raises awareness of that discrepancy.
In general there was interest in the RFC discussions to stabilize this ASAP, regardless of what resolver direction we went.

There is an unresolved question on differences in the resolver vs `cargo add` for dealing with an unset `rust-version` (noted in the tracking issue). However, we are only stabilizing the `cargo add` side which is a very light two-way door as this is a UX-focused command and not a programmatic command.

This hasn't gotten much end-user acceptance testing but, as its UX focused, that seems fine (light, two way door)

As such, this seems like it is ready to stabilize.

### Additional information
bors added a commit that referenced this pull request Apr 16, 2024
feat(update): Include a Locking message

### What does this PR try to resolve?

This extends #13561 to `cargo update`.  I previously left it out because the locking message was redundant.  However the `Locking` message has been extended in #13754 to include the resolving policy which can be a useful point of interest (e.g. "why does `cargo update` do nothing? Oh, `-Zminimal-versions` is enabled").

I still left out the message for `--precise` because the user is overriding all of that.

I'd still like to extend all of this to `cargo install` (and maybe all resolves) but that is taking more investigation.

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

### Additional information
bors added a commit that referenced this pull request Apr 17, 2024
feat(install): Including Locking message

### What does this PR try to resolve?

This extends #13561 to include `cargo install`, like #13759 did for `cargo update`.

As we switch to MSRV-aware resolver, this will help users work out why
MSRV-aware resolving isn't helping them.

This will also make it more obvious if we breaking things when
developing the MSRV-aware resolver.

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

### Additional information

This still leaves `cargo publish` and a couple other misc situations that I'm intentionally avoiding because
- They hit some weird cases that can confuse the user (e.g. causing `cargo install --locked` to show that 1 package is being added) and we can't distinguish these cases too well from where this is happening
- The value is lower
@ehuss ehuss added this to the 1.79.0 milestone Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lockfile Area: Cargo.lock issues A-testing-cargo-itself Area: cargo's tests Command-generate-lockfile S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report newly held back dependencies on all commands
5 participants