Feedback on `cargo-upgrade` to prepare it for merging

tools and infrastructure cargo
#103

Mostly unprompted feedback from a random GitHub user:

cargo upgrade is pretty meh since it mixes both semver compatible and semver incompatible changes - you usually want to upgrade semver incompatible stuff one by one.

#104

… you usually want to upgrade semver incompatible stuff one by one.

While I also would prefer to do one incompatible upgrade at a time (and commit its compatibility-fixes separately from any other change), I wonder: how could a tool succeed at make that easier than editing Cargo.toml? It would have to arbitrarily pick one dep to upgrade, and that might also break things when related deps must match.

#105

At the moment, cargo upgrade does satisfy this need with the --package and --exclude flags. You can do cargo upgrade --dry-run, see what needs upgrading, and then do cargo upgrade --package clap --package clap_complete.

I find most breaking changes are minor enough that I can do them in bulk, so I do the opposite with cargo upgrade --exclude notify (where notify is a dep that will take more work so I break it out).

1 Like
#106

I've been experimenting with two use cases within this thread to see how they work and so I can consider how we can support them within cargo-upgrade.

  1. For my library, I want to stay on the latest major version but I want to keep my compatible version as low as possible. Using the latest dev-dependency is fine
  2. For my program, I'd like to get all the latest fixes

I've been getting frustrated with Dependabot sending a flood of emails for (2) and (1) is incompatible with it, so I decided to try out Renovate and so far it seems to be working out. I figured I'd share my config in case its useful for anyone else.

rust-cli/argfile/.github/renovate.json5 (library):

{
  "schedule": [
    "before 3am on the first day of the month"
  ],
  "semanticCommits": "enabled",
  "configMigration": true,
  "packageRules": [
    // Goals:
    // - Keep version reqs low, ignoring compatible normal/build dependencies
    // - Take advantage of latest dev-dependencies
    // - Rollup safe upgrades to reduce CI runner load
    // - Help keep number of versions down by always using latest breaking change
    // - Have lockfile and manifest in-sync
    {
      "matchManagers": ["cargo"],
      "matchDepTypes": ["build-dependencies", "dependencies"],
      "matchCurrentVersion": ">=0.1.0",
      "matchUpdateTypes": ["patch"],
      "enabled": false,
    },
    {
      "matchManagers": ["cargo"],
      "matchDepTypes": ["build-dependencies", "dependencies"],
      "matchCurrentVersion": ">=1.0.0",
      "matchUpdateTypes": ["minor"],
      "enabled": false,
    },
    {
      "matchManagers": ["cargo"],
      "matchDepTypes": ["dev-dependencies"],
      "matchCurrentVersion": ">=0.1.0",
      "matchUpdateTypes": ["patch"],
      "automerge": true,
      "groupName": "compatible (dev)",
    },
    {
      "matchManagers": ["cargo"],
      "matchDepTypes": ["dev-dependencies"],
      "matchCurrentVersion": ">=1.0.0",
      "matchUpdateTypes": ["minor"],
      "automerge": true,
      "groupName": "compatible (dev)",
    },
  ],
}

crate-ci/committed/.github/renovate.json5 (bin):

{
  "schedule": [
    "before 3am on the first day of the month"
  ],
  "semanticCommits": "enabled",
  "configMigration": true,
  "packageRules": [
    // Goals:
    // - Rollup safe upgrades to reduce CI runner load
    // - Have lockfile and manifest in-sync
    {
      "matchManagers": ["cargo"],
      "matchCurrentVersion": ">=0.1.0",
      "matchUpdateTypes": ["patch"],
      "automerge": true,
      "groupName": "compatible",
    },
    {
      "matchManagers": ["cargo"],
      "matchCurrentVersion": ">=1.0.0",
      "matchUpdateTypes": ["minor"],
      "automerge": true,
      "groupName": "compatible",
    },
  ],
}

(groupName causes matching dependencies to be rolled up into a single PR)

The main things I can think to improve are

  • Being MSRV aware (needs package.rust-version in the index)
  • Auto-selecting from the above for packages with bins vs those without
    • I can do this manually but I'm trying to make it as easy as possible to copy/paste config between my crates to make it easier to manage nearly 50 repos

EDIT: The one annoying part is that when you add the app to an org, it creates PRs against each repo to add configuration files.

#107

In a recent cargo PR, we were discussing clap putting deprecated attributes behind a feature flag.

And yea, I imagine communicating to users to use that feature is extremely difficult. I think this is an area where there is opportunity for better options. I keep thinking it is so very similar to cargo fix --edition for migrating editions, it would be nice if there was something similar for migrating to new major versions of a dependency. Or at least have some form of better communication.

This got me thinking, what if cargo upgrade --incompatible had a --verify step like cargo publish that effectively does a RUST_FLAGS="-Awarnings -Ddeprecaref" cargo check?

  • We'd need to give the user control over the feature flags used for building the crate, like cargo publish
  • To help with clap, we'd need to adopt a deprecated feature flag convention and detect it, like "for every direct dependency, check if deprecated is in the feature list and pass in --features <dep>/deprecated"
    • I'm just unsure whether cargo would want to adopt feature conventions like this. Someone proposed hiding features starting with _ and instead the conversation went in the direction of explicitly marking features as hidden. If a convention isn't acceptable, would a dedicated field to specify the feature should be enabled on upgrade be too specialized?

Thoughts?

#108

Effectively, "try a build, and upgrade if and only if it works"?

#109

Upgrade if and only if there are no deprecation messages, yes

Ideally, we'd only care about deprecations for what are candidates for upgrade. If deprecated feature flags are used, that is easy (selectively enable it). Where deprecated feature flags aren't used, I do not see a solution within the current tools we have available to meet the ideal behavior.

#110

That sounds like a great additional feature. I don't think it should be a blocker for merging cargo upgrade, but I'd love to have it in the future.

1 Like
#111

Sounds like dependabot. I think of cargo upgrade as a TOML editor, not as a dependency manager. Running compilation with speculative feature flags is more than I expect from it.

I'm not sure if such test is useful. In case of clap I know my projects will fail to build, as they require extensive changes. In some cases I plan to stay on clap v2 or v3 forever, so I wouldn't want other cargo upgrade runs complicated or slowed down by retrying clap over and over.

1 Like
#112

Sounds like dependabot. I think of cargo upgrade as a TOML editor, not as a dependency manager. Running compilation with speculative feature flags is more than I expect from it.

Personally, I feel that is too limited of a view. Breaking changes aren't something to be done blindly and we should be finding ways to help people through the process, whether that is

  • checking for unresolved deprecations pre-upgrade
  • linking out to or showing changelogs or migration guides (see also issues #1369, #2188)
  • running cargo semver-check and to provide an automated report of what changed
  • automating the upgrade process
1 Like
#113

Thinking about the upcoming toml / toml_edit releases and a comment in #1369, one limitation of the pre-upgrade verify step is that a naive solution would only report deprecations for the version the user is currently on and not for the latest version within the current major number. The user could also be upgrading across multiple major versions.

#114

(I say cargo update throughout, considering the merge path of adding new modes to cargo update instead of a separation between "update" and "upgrade" functionality. The logic applies equally to a distinct upgrade, but I currently agree with the line of thought that officially differentiating between the two essentially synonyms isn't a good idea.)

Unless you're also doing a post-upgrade check, you're still going to end up with broken upgrades. Because major version bumps being compatible modulo removing deprecated API is the exception rather than the norm. Doing upgrades for crates individually is also likely to cause failures as well, due to "peer/public dependencies" where the version used by the local crate needs to match the version used upstream.

Absolutely we'd benefit from some sort of cargo fix --edition like migration tool hookable by crates, and integrating that into cargo upgrade can make sense, but it's significantly more complicated than just blocking upgrades in the face of deprecation warnings.

If the behavior of cargo update --allow-incompatible --verify would just be to do a precheck and bail on deprecation warnings... The user is capable of doing that step themselves. If you're doing incompatible upgrades, you're taking on the responsibility of knowing what incompatible changes you need to resolve. It's also sort of a false promise if you don't verify the post-upgrade state rather than just try to divine it from the pre-upgrade state.

If it can apply some migrations beyond just whatever cargo fix --deprecated would, then it becomes meaningful to bundle into cargo update.

To that "shiny future" vision, I'd say that dependency crates (the lower version) should opt in to providing an update path across semver-incompatible versions. The first step is essentially extending the semver trick; cargo update --persist can be told to upgrade from autocfg@0.1.8 to autocfg@1, because the former is just a transparent reexport of the latter with no changes.

This then fairly naturally can extend to allowing the crate to provide metadata on how to detect use of deprecated/removed functionality, do automated migrations, etc. It's probably still prudent to still limit cargo update --persist to the fully compatible upgrades, and have a separate --allow-migrations to request the more involved process with potentially building and editing the local crate.

Anyone doing cargo update --allow-incompatible is fully aware that they're potentially getting put into a noncompiling state; they've explicitly requested as such. At a minimum, doing automatic validation (pre or post) of incompatible version upgrades should probably stay a separate opt-in flag from requesting the incompatible upgrades. It should also probably be possible to mark a dependency as "pinned" and ignored by cargo update --allow-incompatible without renaming it, for cases like deliberately staying on an older clap version despite it (presumably in this future) providing at least partially automated migration assistance.

#115

While I can understand the idea that checking for deprecations isn't complete, this statement makes me feel like you missed the point. From my own experience, I repeatedly see people upgrade clap without checking the migration guide or even the changelog. The process is likely more frustrating and there are enough situations where they pull me in for help.

Some problems I see with the upgrade process include

  • The aides for upgrade are not inline with the process. This is exacerbated by the fact that many times, they don't exist so people are less likely bother to search.
  • If an upgrade appears easy, someone might try to muddle along through changes in the API and then be strung along by problem after problem due to changes in behavior but not re-evaluate the decision to not look for a changelog or migration guide.

In contrast to the blind upgrade, someone could do:

  1. Run cargo upgrade -i
  2. For every dependency, browse to libs.rs/<crate> (libs.rs is being used as it removes some steps)
  3. Then find the repository link
  4. Then check for the changelog
  5. If the changelog wasn't there, check for Github Releases
  6. If a Release isn't there, browse the commits, finding the right range to use
  7. Map what you have found to how you use the crate

Both processes are frustratingly flawed. It'd be great if we could come up with ways to help the user through it where cargo upgrade could be viewed as a companion like rustc compiler errors are.

#116

How about cargo upgrade designed to work in stages, choosing what to do depending on the current state of the project:

  1. If there are minor/compatible deps to upgrade, upgrade just these.
  2. If there are no minor/compatible deps left to upgrade, then try either the features = deprecated test, or upgrade some incompatible deps.

That would make cargo upgrade "just work" without needing extra flags.

It'd be cool if it could tell the difference between semver-incompatible but actually still fine without changes vs really-really incompatible upgrades that require manual code changes, and first upgrade the easy ones.

From a workflow perspective, I'd strongly prefer cargo upgrade to do something useful without having to add or remove extra flags.

So a workflow where I run cargo upgrade; cargo test; <fix what needs to be fixed>; goto 10 in a loop until everything is fixed and upgraded would be nice.

#117

Over the last 3 months or so I've been re-thinking the command merge. We quickly settled on that in this thread and we've had 6 months or so of runtime on it. While usually those with problems are the loudest, I don't remember seeing any appreciation for the current behavior. I think it was Josh who had a proposed tweak to the flags that I've not had a chance to talk with him further about but I don't think that will fundamentally change things if we found it was better than what we have.

Stated reasons for doing the merge:

From my original post

Separate cargo update (for lock file) and cargo upgrade (for manifest) commands is confusing to some users

From CLI Guidelines

Don’t have ambiguous or similarly-named commands. For example, having two subcommands called “update” and “upgrade” is quite confusing. You might want to use different words, or disambiguate with extra words.

Unfortunately, I do not remember examples of "user confusion" I referenced. I also think people are used to the distinction between "software updates" (patches, safe) and "software upgrades" (here be dragons but new goodies).

As highlighted in kornel's post, the current model is cumbersome. People have requested specialized flags but that doesn't compose well and would be just as cumbersome.

My current thinking

  • Add a --save to cargo update
    • If we feel so bold, in a new edition make this the default or only behavior
    • Unsure how I feel about the name if we keep this opt-in long term as the dependency is already saved, in the lock file.
  • Scale back cargo upgrade to incompatible upgrades with "pinned" dependency detection
  • Change cargo upgrade to only update the lock file in a non-recursive fashion
  • Maybe add --dev flags to both cargo upgrade and cargo update that only updates dependencies that are not direct build or normal dependencies.
    • One of the workflows is for library authors to keep their direct dependency requirements low. An exception to this is dev-dependencies as they don't affect the callers. While I asked around about more general ways of specifying dependency kinds, I've found that for upgrades, the only distinction I care about is between what my dependents see (build, normal) and what they don't see (dev), making it so we only really need a --dev flag.

It'd be cool if it could tell the difference between semver-incompatible but actually still fine without changes vs really-really incompatible upgrades that require manual code changes, and first upgrade the easy ones.

Earlier I mentioned the idea of using cargo-semver-check to tell you what changed. You bring up a point I forgot,: it'd be even better if it looked at what parts of the API you used and filter the report to just that.

That still leaves behavior changes. We have #[deprecated] to tell people not to use an API item (possibly in lead up to removal). I've seen people talk about stablizing #[stable] so we can tell when an API item was added. What if we had another attribute for last behavior change? Granted, all of these are only as useful as people using them but I could see that helping a lot with people upgrading across the upcoming toml release.

Even more off topic but I wish #[deprecated] had an "after` attribute rather than a "since" attribute so I didn't have to speculate what my next version is going to be (usually predictable but I don't want to have to think about it and "usually" is the operative word)

1 Like
#118

I have now skimmed through the 115 comments in this thread so far, and one question still eludes me: what is the motivation for cargo update --save that isn't solved by checking in Cargo.lock? The use-cases I've seen so far are:

  1. I want to do (or check for) major version upgrades for my dependencies
  2. I want to make my manifest match my lockfile because I started using a new feature from a dependency

1 can be done with cargo update --allow-major --save.
2 can be done with cargo update -p foo --save

If I had to come up with an argument for cargo update --save that isn't solved by "check in Cargo.lock", I think it'd be:

I don't know/want to list all the crates that I used new features from, so just bump all of them.

That is a fine position to take for "application" crates (i.e., those at the leaves with no dependents). But I think it's something we want to discourage for anything that may have dependents. And Cargo has (at least in the past) not been opposed to encoding incentives in what is easy/hard to do. Which makes me think that we should not optimize for cargo update --save. Maybe it should be possible, but that should be the "most annoying to get to" mode, not the easiest one.

Which gets me to the update/upgrade discussion. It seems like the main argument (?) for separating them is that upgrade can imply --save and possibly --allow-major, whereas we can't make update do that. I think there's a lot of merit to that argument. But there is also a lot of merit to the argument that update and upgrade are too similar, and people will be confused. Now, granted, confusion here isn't too destructive, but even so it's friction. I wonder if we can mitigate that (as was proposed by at least one past comment) by choosing a better name for either (or both?) commands through a two-level subcommand:

cargo dependencies add # equivalent to cargo add
cargo dependencies upgrade # equivalent to the proposed cargo update --allow-major --save
cargo dependencies upgrade --to-locked # equivalent to the proposed cargo update --save
cargo dependencies upgrade -p foo --to-locked # equivalent to the proposed cargo update -p foo --save
cargo dependencies upgrade -n/--dry-run # equivalent to cargo update --allow-major (no --save)

Crucially, like for other cargo subcommands this could also be shortened — e.g., cargo b == cargo build — so cargo dep up would work for folks who don't want to type a lot.

If we wanted to go one step further, we'd also deprecate cargo update and encourage the use of cargo generate-lockfile (with a flag?) in its place, and in the next edition make cargo update map to cargo dependencies upgrade.

2 Likes
#119

I think it's perfectly valid to say "I tested with these versions, so I know these versions work". In the absence of a Cargo change to make "test with minimal versions" usable, together with widespread ecosystem fixes and CI changes, I think "upgrade to latest versions and test those" is a valid strategy for making sure your crate works with the versions it declares.

3 Likes
#120

True, that's a good point. And I guess the argument is that checking in your Cargo.lock won't help your dependents, and therefore your only way of communicating "I tested with this" is to bump the versions in Cargo.toml. I still think that's not a state of affairs we want to be in though — we would rather that folks test with the minimum version they specify, and avoid bumping it unnecessarily (again, unless they're an end application). That's supported with what I proposed above, it's just a little cumbersome, which I think is a decent way to balance "we need to do this today" with "we don't generally want this to be the way it's done longer-term".

1 Like
#121

Tbqh, I also agree somewhat. I don't like moving the functionality behind a subcommand so much, but I do agree that a blanket cargo update --save is probably something we don't want to directly encourage libraries to be doing.

But on the other hand,

  • cargo is already biased towards pushing people to use the latest version of dependencies. cargo install's default mode isn't --locked[1], --minimal-versions remains nearly useless since most crates aren't "minimal versions correct" (and the way to get correct typically requires requiring the latest version), and we still don't have even an unstable resolver for "minimal just for me." Updating versions is presumed to be desirable and low risk, although it is limited to only be done on user interaction. (Though it should also be noted that cargo update -p can and does result in updating arbitrary other packages in the tree! If the updated package's dependency constraints aren't satisfied, those get updated to latest as well.) To that end, we're slowly adding features to eliminate minor breaking changes[2].
  • cargo's modality supports that there are more binary crates than library crates, and that defaults should bias towards the mode better suited for binaries (e.g. cargo new defaulting to --bin) if the two prefer different modes.

Given complete carte blanche to redo things, I think my current picture of an ideal world would be roughly:

Collapsed because long

Choice of flag names was the first naming to come to mind that didn't suffer from undesired ambiguity. Better naming for usability probably exists; this sketch mostly exists as a way to illustrate the modes/switches I would ideally like to exist, not to say that this naming is ideal.

  • cargo publish works on "minimal just for me" rules; if the lockfile version of direct dependencies is higher than the version specified in the manifest, verification fails.
    • --no-verify now takes an optional argument to allow skipping individual verification steps, of which there are now multiple.
  • Introduce a concept of "phantom dependencies." These are dependencies which aren't actual dependencies, but just register a version constraint to be satisfied iff the crate is included in a dependency tree through other means.
    • Intended for forcing transitive dependencies to meet a specific version (e.g. to ensure a bugfix is included or force-fix minimal versions compliance) without introducing a real dependency edge.
    • When doing version resolution for the package locally, warns if a phantom dependency had no impact on the acceptable version range.
  • Actual cargo update flags:
    • --exclude spec...: the update is conservative w.r.t. the specified package(s), as if --package was used. Error if the package(s) are also requested to update via --package or --aggressive.
    • --transitive: the update is conservative w.r.t. direct dependencies specified in the workspace, as if all direct dependencies were given to --exclude.
    • --direct: the updatd is conservative w.r.t. all transitive dependencies, as if all direct dependencies were given to --package.
    • Combining --transitive --direct is not an error; it updates the entire lockfile.
    • If neither --transitive or --direct are specified AND the workspace includes at least one library package without publish = false, --transitive is implied.
    • --save: write the resolved versions of direct dependencies into the depending manifest(s). Show note if --transitive was implied to indicate direct dependencies weren't updated. Preflight verifies nondirty VCS state.
    • --breaking: only update direct dependencies with semver-major version updates available. Implies --direct --save; error when combined with --transitive. Upgrades to direct dependencies without semver-major updates are conservative, as if given to --exclude.
  • Dependencies can be marked as "pinned" to be omitted from --breaking unless explicitly requested. Packages are implicitly considered pinned if they've been renamed in the manifest (but not if the "lib" = { package = "name" } syntax was used without renaming the library crate), but can opt back out of being pinned.

This doesn't do the "maintain precision" that cargo-upgrade does. I maintain that underprecise caret bounds (the default bound is caret) are an antipattern; if you support a minimum version of 1.0.0, specify a dependency constraint of ^1.0.0, not ^1.0. Instead, we address a similar (same?) desire by just not updating the direct dependencies of libraries unless requested.

It may be possible to incrementally migrate to something approaching this model, although I don't know; but I do know that deprecating and/or changing the behavior of basic cargo update commands that work today needs to be done with extreme caution. Editions aren't free reign to make breaking changes, and it's important to note that workspaces don't even have editions, only packages. (This is already kinda painful for the version resolver, which needs to be explicitly set if in a workspace without a package at the root manifest.)

A core guiding principle of editions is that we don't want to make old documentation / forum posts / etc. incorrect, even in the new edition. What edition is in use is a mostly transparent property, with the compiler stepping in to offer help when there's a mismatch. At worst, someone following advice aimed at a different edition than they're in should ideally[3] result in a reasonably helpful compile error that can reference the edition concept. The case where a crate requires the new (or rarely, old) version resolution is an unfortunate case where determining that's likely the issue is difficult[4].

  1. I think this one is slated to change? If an application included a lockfile in the package, we probably should respect it by default. ↩︎

  2. The currently proposed one is still std-only and very restricted, but it lays the precedent groundwork for deprioritizing names added after the library version a package was developed against in name resolution. Alternatively, it's long been discussed to have cargo publish upload the "fully elaborated" name resolution information and use that for dependencies, to address name resolution breakage that way. (Although how that interacts with upstream macros is unclear.) ↩︎

  3. The edge cases where code compiles in multiple current editions with different meaning are extremely rare. Confusing async blocks with a struct literal is the simplest one... but that effectively requires not using the resulting value, so the semantics of both versions likely actually matches. ↩︎

  4. I don't actually recall if we print a suggestion that you might want to use the new version resolver when version resolution fails with the old resolver. If we don't, we probably should. Running into this is unfortunately decently common since virtual workspaces still default to the old resolver and wgpu is a notable crate that requires the new version resolver. ↩︎

2 Likes
#122

One should never check in a lockfile. I really gotta find the time to write the full-length essay about how bad a practice this is.

(I might even go as far as to say that lockfiles shouldn't exist. I've seen only one or two legitimate uses and it's not clear to me that these are sufficient to justify how easy it is to screw both yourself and all your downstreams over with the mere existence of a lockfile.)