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 update should not highlight non-semver-compatible "(latest: v0.23.5)" for indirect dependencies #13908

Closed
djc opened this issue May 13, 2024 · 16 comments
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. C-bug Category: bug Command-update S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@djc
Copy link
Contributor

djc commented May 13, 2024

Problem

I just ran my weekly cargo update against the workspace at work, and it highlighted this change:

    Updating rustls v0.21.11 -> v0.21.12 (latest: v0.23.5)

This workspace does not have a direct dependency on rustls. As such, I would suggest this information isn't actionable enough to merit a mention in the non-verbose cargo update output (it would definitely fit in with the verbose output).

Steps

No response

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.78.0 (54d8815d0 2024-03-26)
release: 1.78.0
commit-hash: 54d8815d04fa3816edc207bbc4dd36bf18014dbc
commit-date: 2024-03-26
host: aarch64-apple-darwin
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.4.0 (sys:0.4.72+curl-8.6.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.4.1 [64-bit]
@djc djc added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels May 13, 2024
@heisen-li
Copy link
Contributor

@rustbot label +Command-update +A-console-output

@rustbot rustbot added A-console-output Area: Terminal output, colors, progress bar, etc. Command-update labels May 13, 2024
@weihanglo
Copy link
Member

Thanks for the suggestion!

Agree it is not actionable locally and quite verbose, especially when you have only few direct dependencies and that's very noisy. However, I see it as an opportunity to encourage user to collaborate with upstreams. Putting in behind --verbose would reduce the change of it. It is also a more consistent console output that an update always comes with latest version information, but not some are behind --verbose some are not.

One similar work we've done is future-compatible warnings. Cargo indeed puts them behind cargo report, though I am not sure if they should have the same behavior. Should also weigh on the complexity added and the verbosity it reduces.

@weihanglo weihanglo added S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-triage Status: This issue is waiting on initial triage. labels May 13, 2024
@djc
Copy link
Contributor Author

djc commented May 14, 2024

Agree it is not actionable locally and quite verbose, especially when you have only few direct dependencies and that's very noisy. (..) Putting in behind --verbose would reduce the change of it. It is also a more consistent console output that an update always comes with latest version information, but not some are behind --verbose some are not.

This seems like bad reasoning, since cargo update --verbose reveals a bunch more crates that similarly have semver-incompatible versions (but somehow these are not being shown without --verbose):

Unchanged hyper-rustls v0.24.2 (latest: v0.27.1)

I suppose that the (latest ..) notice for rustls is only shown because there was a semver-compatible bump, but IMO that's not a good reason to also show the (latest ..) notice for non-direct semver-incompatible versions.

If anything, I would be happy if you showed Unchanged semver-incompatible bumps for my workspace's direct dependencies, but it currently doesn't -- so that also feels inconsistent.

To summarize:

  • I think Updating notices should not show (latest ..) if the latest version is semver-incompatible with the selected version (because that doesn't fit in with the scope of cargo update)
  • Maybe the color should change for (latest ..) notices advertising a version that is semver-incompatible with the selected version, making it green instead?
  • Even for Unchanged notices from cargo update --verbose it's not obvious to me that it makes sense to show (latest ..) notices for semver-incompatible versions, since that's historically outside of the scope of update
  • On the other hand, it would be great to start extending cargo update to help users address semver-incompatible bumps, and one good way to go about that would be to start noting available semver-incompatible updates for direct dependencies using a (latest ..) notice independent from if the dependency got updated

However, I see it as an opportunity to encourage user to collaborate with upstreams.

(I happen to do a lot of this, trying to push the ecosystem forward -- but I think I'm in a small minority, and anyway it's still a different level of actionable/effort to effect changes like this, so any production user will have to timebox these efforts.)

(cc @epage who's done a lot of work in this area.)

@epage
Copy link
Contributor

epage commented May 28, 2024

Sorry for the delay, I've been doing some focused time trying to wrap things up before I get to my 100+ github notifications between MSRV, 2024 Edition, and RustNL (wish timing worked to talk to you in person about this).

This also caused confusion in #13873 (comment)

This was previously discussed in #13372. The idea behind this comes from

If anything, I would be happy if you showed Unchanged semver-incompatible bumps for my workspace's direct dependencies, but it currently doesn't -- so that also feels inconsistent.

I'd have to check the implementation and issues, but I have a feeling this runs counter to the feedback you gave for cargo upgrade.

CC @jonhoo as you mentioned in the release changes stream your excitement for reporting changes and want to make sure we capture multiple perspectives

@djc
Copy link
Contributor Author

djc commented May 28, 2024

If anything, I would be happy if you showed Unchanged semver-incompatible bumps for my workspace's direct dependencies, but it currently doesn't -- so that also feels inconsistent.

I'd have to check the implementation and issues, but I have a feeling this runs counter to the feedback you gave for cargo upgrade.

I'm curious what feedback you're thinking of exactly!

I think giving this (latest: ..) feedback during cargo update is excellent and, if implemented well (consistently), takes a lot of the pressure off of cargo upgrade IMO -- if cargo upgrade will reliably tell me when my direct dependencies are outdated, actually upgrading them is the easy part. So I think there is a lot of potential here. What I'm unhappy about is the random nature (only dependencies for a semver-compatible update is available show whether there is a newer incompatible version available), which might make sense for the MSRV scenario specifically but doesn't make sense when the current workspace does not specify an MSRV target.

@epage
Copy link
Contributor

epage commented May 28, 2024

With cargo upgrade, didn't you push back on a lot of our output unrelated to the actual upgrade, having us put it behind --verbose? Showing unchanged direct dependencies (that could be updated) runs counter to that.

What I'm unhappy about is the random nature (only dependencies for a semver-compatible update is available show whether there is a newer incompatible version available), which might make sense for the MSRV scenario specifically but doesn't make sense when the current workspace does not specify an MSRV target.

changed dependencies are shown, regardless of the nature of the change (cargo upgrade like functionality is my priority after MSRV/cargo script).

Why doesn't this make sense when an MSRV isn't set? Cargo is preventing an upgrade and we want the user to know. Right now, resolving some of those upgrades is more manual (edit version requirements, bug dependencies) but we will be adding support for a flag to help with the version requirements.

@djc
Copy link
Contributor Author

djc commented May 29, 2024

With cargo upgrade, didn't you push back on a lot of our output unrelated to the actual upgrade, having us put it behind --verbose? Showing unchanged direct dependencies (that could be updated) runs counter to that.

IIRC the output in cargo upgrade I pushed back on was when it displayed a large table of dependencies many of which had no available updates at all, or only had semver-compatible updates where I was only expressing an interest in incompatible updates -- there was a bunch of inactionable output. In this case I'm arguing both that there is output that is inactionable and that there's missing output that would be actionable.

What I'm unhappy about is the random nature (only dependencies for a semver-compatible update is available show whether there is a newer incompatible version available), which might make sense for the MSRV scenario specifically but doesn't make sense when the current workspace does not specify an MSRV target.

changed dependencies are shown, regardless of the nature of the change (cargo upgrade like functionality is my priority after MSRV/cargo script).

Why doesn't this make sense when an MSRV isn't set? Cargo is preventing an upgrade and we want the user to know. Right now, resolving some of those upgrades is more manual (edit version requirements, bug dependencies) but we will be adding support for a flag to help with the version requirements.

Yes, I understand what it's doing and why it is the way it is. I'm arguing that whether the "(latest ..)" badge is shown should be independent of whether a dependency is changed (due to a semver-compatible upgrade/downgrade), because whether a semver-compatible update happened since the last time I ran cargo update or not is not meaningful signal -- it doesn't have any bearing on how actionable the "latest" hint is.

Including the plan for the MSRV-aware resolver, I think there are 3 different categories here:

  • There is a semver-compatible newer version that was not selected because of a workspace MSRV restriction: this should be shown always (whether there was a semver-compatible update or not this time, because otherwise I would miss it if I'm stuck at the latest semver-compatible update when all newer versions are MSRV-incompatible).
  • There is a semver-incompatible newer version in a direct dependency: I would argue that this should also always be shown, because it is likely actionable for the user, and again, whether a semver-compatible update happened since the last cargo update run is immaterial (it's likely that no semver-compatible updates will happen anymore now that a semver-incompatible newer version has been published).
  • There is a semver-incompatible newer version in an indirect dependency: I would probably not show this by default (only for --verbose), since it is much less likely to be actionable for the user -- at the very least it will be different level of effort. But if we're showing this for dependencies that got a semver-compatible update during this run, I think we should also show it for dependencies that did not get an update during this run, because otherwise it's pretty inconsistent for users. (Also perhaps more likely to increase mob mentality in users pressuring dependencies to upgrade.)

@jonhoo
Copy link
Contributor

jonhoo commented May 29, 2024

Thanks for CCing me! Yeah, I think I agree with @djc above on the things that are important to highlight. In particular, I agree that dependencies that could not be updated to the latest version for a reason that is under the user's control (i.e., direct dependency or MSRV) should be highlighted independently of whether that dependency happened to be updated in this particular cargo update run. I also agree that showing non-actionable available updates should be placed behind --verbose.

I very much empathize with @weihanglo wanting to encourage upstream contribution, but as @djc points out there is a trade-off here where overloading users with hard-to-action information is likely to lead to worse overall outcomes (people ignoring warnings) rather than the virtuous effects we wish to see. I sadly don't have a great suggestion for how to highlight updates that are blocked indirectly. Personally, I also tend to run cargo outdated -R much more often than cargo outdated precisely because it's so hard to really do anything about the transitive blocks. I think that's a problem that's likely to need some materially different solution.

epage added a commit to epage/cargo that referenced this issue Aug 14, 2024
…cted

In discussin this in rust-lang#13873, it highlighted that we need to make sure we
tell people when we get in this state.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at rust-lang#13908.
epage added a commit to epage/cargo that referenced this issue Aug 14, 2024
…cted

In discussin this in rust-lang#13873, it highlighted that we need to make sure we
tell people when we get in this state.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at rust-lang#13908.
epage added a commit to epage/cargo that referenced this issue Aug 15, 2024
…cted

In discussin this in rust-lang#13873, it highlighted that we need to make sure we
tell people when we get in this state.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at rust-lang#13908.
epage added a commit to epage/cargo that referenced this issue Aug 16, 2024
…cted

In discussin this in rust-lang#13873, it highlighted that we need to make sure we
tell people when we get in this state.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at rust-lang#13908.
bors added a commit that referenced this issue Aug 16, 2024
feat(update): Report when incompatible-rust-version packages are selected

### What does this PR try to resolve?

In discussin this in #13873, it highlighted that we need to make sure we
tell people when incompatible-rust-version packages are selected.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

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

### Additional information

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at #13908.
bors added a commit that referenced this issue Aug 16, 2024
feat(update): Report when incompatible-rust-version packages are selected

### What does this PR try to resolve?

In discussin this in #13873, it highlighted that we need to make sure we
tell people when incompatible-rust-version packages are selected.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

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

### Additional information

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at #13908.
bors added a commit that referenced this issue Aug 16, 2024
feat(update): Report when incompatible-rust-version packages are selected

### What does this PR try to resolve?

In discussin this in #13873, it highlighted that we need to make sure we
tell people when incompatible-rust-version packages are selected.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

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

### Additional information

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at #13908.
bors added a commit that referenced this issue Aug 22, 2024
refactor(update): Prepare for smarter update messages

### What does this PR try to resolve?

This is to make it easier to
- Make #14401 path aware
- Work on #13908, depending on what is decided
- Improve the heuristics for when we show `Locking` messages

There are fixes along the way that were found by making the code more consistent in prep for consolidating it, mostly for #14401.

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

Along the way, some odd code structures are used for the sake of making the diff easier to follow.  These get cleaned up towards the end

I didn't add tests for the fixes because of the code consolidation that happens that causes us to cover more real-world scenarios with fewer tests.

### Additional information
@epage
Copy link
Contributor

epage commented Aug 27, 2024

#14445 tweaked this to skip showing workspace members being locked when doing the regular manifest->lockfile sync

@epage epage added S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels Aug 27, 2024
epage added a commit to epage/cargo that referenced this issue Aug 27, 2024
Instead of always listing the absolute latest version as a warning
color, we now differentiate
- compatible updates are always actionable
- incompatible, direct deps are always actionable

These get reported and made yellow while non-actionable messages are
unstyled.

This is not intended as *the* solution for rust-lang#13908 though it makes
improvements in that direction.
This is prep work for improved MSRV reporting where we will
differentiate this further by only considering MSRV-compatible updates as actionable
(or rustc-compatible when not using MSRV-aware reslver).

I just used a broad stroke to say "compatible" in the message means "semver
compatible" and use `^`
- We could focus on "compatible with dependent version reqs" which is
  what will be most actionable but seeing if we can get away without
  having to track all in-coming version reqs.
- We could be more nuanced in language but the more verbose we are, the
  more we take away from higher priority messages
@epage
Copy link
Contributor

epage commented Aug 27, 2024

#14461 further tweaks things. As noted in the PR, this is more of a stepping stone for some MSRV changes. As a side benefit, it experiments with how to handle this issue. After the rest of the MSRV reporting changes are in, I'll be looking for input on where we are at and discuss it with the cargo team.

bors added a commit that referenced this issue Aug 29, 2024
fix(resolve): With `latest` message, differentiate actionable updates

### What does this PR try to resolve?

Instead of always listing the absolutely latest version as a warning
color, we now differentiate
- compatible updates are always actionable
- incompatible, direct deps are always actionable

These get reported and made yellow while non-actionable messages are
unstyled.

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

I just used a broad stroke to say "compatible" in the message means "semver
compatible" and use `^`
- We could focus on "compatible with dependent version reqs" which is
  what will be most actionable but seeing if we can get away without
  having to track all in-coming version reqs.
- We could be more nuanced in language but the more verbose we are, the
  more we take away from higher priority messages

### Additional information

This is not intended as *the* solution for #13908 though it experiments with what to do for that.
This is prep work for improved MSRV reporting where we will
differentiate this further by only considering MSRV-compatible updates as actionable
(or rustc-compatible when not using MSRV-aware reslver).
@epage
Copy link
Contributor

epage commented Aug 30, 2024

#14471 is the last step. Once that hits nightly, I'll be looking for a new round of feedback and then take it to the Cargo team.

bors added a commit that referenced this issue Sep 3, 2024
feat(resolve): Report MSRV compatible version instead of incomptible

### What does this PR try to resolve?

This expands on #14461 to where only MSRV-compatible versions are
"actionable".  MSRV-incompatible versions are therefore unstyled.

We report the MSRV needed so people can choose to unblock by updating
their MSRV.  I had wondered if we should report the the absolute latest
MSRV-incompatible version or the one with the next higher MSRV from
where the user is at.  Both are reasonable use cases, so I erred with
absolute latest version.

```console
$ cargo update --workspace -v
```
![image](https://github.com/user-attachments/assets/27e40dda-287b-4223-a377-0233205307a2)

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

I changed the label from `latest` to `available` to not have to keep coming up with relevant terms for each case.

### Additional information

This is the final step before asking for new feedback on #13908
antoniospg pushed a commit to antoniospg/cargo that referenced this issue Sep 8, 2024
Instead of always listing the absolute latest version as a warning
color, we now differentiate
- compatible updates are always actionable
- incompatible, direct deps are always actionable

These get reported and made yellow while non-actionable messages are
unstyled.

This is not intended as *the* solution for rust-lang#13908 though it makes
improvements in that direction.
This is prep work for improved MSRV reporting where we will
differentiate this further by only considering MSRV-compatible updates as actionable
(or rustc-compatible when not using MSRV-aware reslver).

I just used a broad stroke to say "compatible" in the message means "semver
compatible" and use `^`
- We could focus on "compatible with dependent version reqs" which is
  what will be most actionable but seeing if we can get away without
  having to track all in-coming version reqs.
- We could be more nuanced in language but the more verbose we are, the
  more we take away from higher priority messages
antoniospg pushed a commit to antoniospg/cargo that referenced this issue Sep 8, 2024
…cted

In discussin this in rust-lang#13873, it highlighted that we need to make sure we
tell people when we get in this state.

I decided to keep "latest" and "required rust version" messages mutually
exclusive to avoid too much noise.  I gave "required rust version"
higher precedence as its the more critical to operation and, if you are
using an MSRV-incompatible package, it likely is "latest" already.

I was tempted to change colors to make "required rust version" stand out
from "latest" but was unsure what direction to go, so I held off.
Options included
- red for "required rust version", yellow for "latest"
- yellow for "required rust version", nothing for "latest"

There is also more discussion on how to format "latest" at rust-lang#13908.
@epage
Copy link
Contributor

epage commented Sep 17, 2024

#14471 should now be in nightlies. Could people give it a try and let us know what works well and what doesn't?

dingxiangfei2009 pushed a commit to dingxiangfei2009/cargo that referenced this issue Sep 17, 2024
Instead of always listing the absolute latest version as a warning
color, we now differentiate
- compatible updates are always actionable
- incompatible, direct deps are always actionable

These get reported and made yellow while non-actionable messages are
unstyled.

This is not intended as *the* solution for rust-lang#13908 though it makes
improvements in that direction.
This is prep work for improved MSRV reporting where we will
differentiate this further by only considering MSRV-compatible updates as actionable
(or rustc-compatible when not using MSRV-aware reslver).

I just used a broad stroke to say "compatible" in the message means "semver
compatible" and use `^`
- We could focus on "compatible with dependent version reqs" which is
  what will be most actionable but seeing if we can get away without
  having to track all in-coming version reqs.
- We could be more nuanced in language but the more verbose we are, the
  more we take away from higher priority messages
@djc
Copy link
Contributor Author

djc commented Sep 19, 2024

I tried this with rustc 1.83.0-nightly (f79a912d9 2024-09-18) and it's better but still not ideal:

Image

rustls-native-certs does not appear in this workspace's Cargo.toml, so the note here is not actionable.

@epage
Copy link
Contributor

epage commented Sep 19, 2024

@djc part of the intent for that was to surface technical debt in your dependency tree. I tried to distinguish actionable items by giving them a color. The overall challenge is finding the right balance for what to include and how to include it without overwhelming the user. We had talked about the transitive breaking changes in the cargo team meeting before your post and decided to go ahead and cut them for now.

epage added a commit to epage/cargo that referenced this issue Sep 19, 2024
bors added a commit that referenced this issue Sep 21, 2024
fix(resolve): Don't list transitive, incompatible dependencies as available

### What does this PR try to resolve?

We have limited capability to clearly communicate to the user the different update states without overwhelming them.
Before we showed all versions that were behind, with color distinguishing how actionable they are.
Color doesn't communicate enough though and we don't want to add footnotes to clarify everything.
For now, the least useful messages will be removed, available, transitive, incompatible versions.  These only give the user a hint of tech debt within their dependencies and can't be acted upon.

This is part of #13908

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

### Additional information
@epage
Copy link
Contributor

epage commented Oct 28, 2024

@djc are things in a satisfactory state for closing this?

@djc
Copy link
Contributor Author

djc commented Oct 28, 2024

Yes, I think things are in a much better state now. Thanks!

@djc djc closed this as completed Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. C-bug Category: bug Command-update S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

6 participants