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

A method to update manifest version requirements (like cargo-edit's cargo-upgrade) #10498

Open
Milo123459 opened this issue Mar 22, 2022 · 6 comments
Labels
A-new-subcommand Area: new subcommand C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-update S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@Milo123459
Copy link

Problem

I had a crate that was defined in my projects Cargo.toml, it updated the version in the Cargo.lock, but not the Cargo.toml

Proposed Solution

It would be cool if cargo update also updated Cargo.toml if applicable.

Notes

No response

@Milo123459 Milo123459 added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Mar 22, 2022
@epage
Copy link
Contributor

epage commented Mar 22, 2022

I do not know if there is a dedicated issue for this but I plan to follow up on the cargo-add issue by merging other cargo-edit subcommands, like cargo-rm and cargo-upgrade.

cargo-upgrade is probably the least defined for what it will look like merged with questions like

  • Should it be a separate command or a flag on cargo-update?
  • Should --skip-compatible be a default? the only behavior?

Something that someone can do to help is to scour other language packaging tools for existing precedence in how they handle cases like this like the cargo-add PR does.

@epage epage added A-new-subcommand Area: new subcommand Command-update labels Mar 22, 2022
bors added a commit that referenced this issue Apr 18, 2022
feat: Import cargo-add into cargo

### Motivation

The reasons I'm aware of are:
- Large interest, see #5586
- Make it easier to add a dependency when you don't care about the version (instead of having to find it or just using the major version if thats all you remember)
- Provide a guided experience, including
  - Catch or prevent errors earlier in the process
  - Bring the Manifest format documentation into the terminal via `cargo add --help`
  - Using `version` and `path` for `dependencies` but `path` only for `dev-dependencies` (see crate-ci/cargo-release#288 which led to killercup/cargo-edit#480)

### Drawbacks

1. This is another area of consideration for new RFCs, like rust-lang/rfcs#3143 (this PR supports it) or rust-lang/rfcs#2906 (implementing it will require updating `cargo-add`)

2. This is a high UX feature that will draw a lot of attention (ie Issue influx)

e.g.
- killercup/cargo-edit#521
- killercup/cargo-edit#126
- killercup/cargo-edit#217

We've tried to reduce the UX influx by focusing the scope to preserving semantic information (custom sort order, comments, etc) but being opinionated on syntax (style of strings, etc)

### Behavior

Help output
<details>

```console
$ cargo run -- add --help
cargo-add                                                                                                                                  [4/4594]
Add dependencies to a Cargo.toml manifest file

USAGE:
    cargo add [OPTIONS] <DEP>[`@<VERSION>]` ...
    cargo add [OPTIONS] --path <PATH> ...
    cargo add [OPTIONS] --git <URL> ...

ARGS:
    <DEP_ID>...    Reference to a package to add as a dependency

OPTIONS:
        --no-default-features     Disable the default features
        --default-features        Re-enable the default features
    -F, --features <FEATURES>     Space-separated list of features to add
        --optional                Mark the dependency as optional
    -v, --verbose                 Use verbose output (-vv very verbose/build.rs output)
        --no-optional             Mark the dependency as required
        --color <WHEN>            Coloring: auto, always, never
        --rename <NAME>           Rename the dependency
        --frozen                  Require Cargo.lock and cache are up to date
        --manifest-path <PATH>    Path to Cargo.toml
        --locked                  Require Cargo.lock is up to date
    -p, --package <SPEC>          Package to modify
        --offline                 Run without accessing the network
        --config <KEY=VALUE>      Override a configuration value (unstable)
    -q, --quiet                   Do not print cargo log messages
        --dry-run                 Don't actually write the manifest
    -Z <FLAG>                     Unstable (nightly-only) flags to Cargo, see 'cargo -Z help' for
                                  details
    -h, --help                    Print help information

SOURCE:
        --path <PATH>        Filesystem path to local crate to add
        --git <URI>          Git repository location
        --branch <BRANCH>    Git branch to download the crate from
        --tag <TAG>          Git tag to download the crate from
        --rev <REV>          Git reference to download the crate from
        --registry <NAME>    Package registry for this dependency

SECTION:
        --dev                Add as development dependency
        --build              Add as build dependency
        --target <TARGET>    Add as dependency to the given target platform

EXAMPLES:
  $ cargo add regex --build
  $ cargo add trycmd --dev
  $ cargo add --path ./crate/parser/
  $ cargo add serde serde_json -F serde/derive
```

</details>

Example commands
```rust
cargo add regex
cargo add regex serde
cargo add regex@1
cargo add regex@~1.0
cargo add --path ../dependency
```
For an exhaustive set of examples, see [tests](https://github.com/killercup/cargo-edit/blob/merge-add/crates/cargo-add/tests/testsuite/cargo_add.rs) and associated snapshots

Particular points
- Effectively there are two modes
  - Fill in any relevant field for one package
  - Add multiple packages, erroring for fields that are package-specific (`--rename`)
  - Note that `--git` and `--path` only accept multiple packages from that one source
- We infer if the `dependencies` table is sorted and preserve that sorting when adding a new dependency
- Adding a workspace dependency
  - dev-dependencies always use path
  - all other dependencies use version + path
- Behavior is idempotent, allowing you to run `cargo add serde serde_json -F serde/derive` safely if you already had a dependency on `serde` but without `serde_json`
- When a registry dependency's version req is unspecified, we'll first reuse the version req from another dependency section in the manifest.  If that doesn't exist, we'll use the latest version in the registry as the version req

### Additional decisions

Accepting the proposed `cargo-add` as-is assumes the acceptance of the following:
- Add the `-F` short-hand for `--features` to all relevant cargo commands
- Support ``@`` in pkgids in other commands where we accept `:`
- Add support for `<name>`@<version>`` in more commands, like `cargo yank` and `cargo install`

### Alternatives

- Use `:` instead of ``@`` for versions
- Flags like `--features`, `--optional`, `--no-default-features` would be position-sensitive, ie they would only apply to the crate immediate preceding them
  - This removes the dual-mode nature of the command and remove the need for the `+feature` syntax (`cargo add serde -F derive serde_json`)
  - There was concern over the rarity of position-sensitive flags in CLIs for adopting it here
- Support a `--sort` flag to sort the dependencies (existed previously)
  - To keep the scope small, we didn't want general manifest editing capabilities
- `--upgrade <POLICY>` flag to choose constraint (existed previously)
  - The flag was confusing as-is and we feel we should instead encourage people towards `^`
- `--allow-prerelease` so a `cargo add clap` can choose among pre-releases as well
  - We felt the pre-release story is too weak in cargo-generally atm for making it first class in `cargo-add`
- Offer `cargo add serde +derive serde_json` as a shorthand
- Infer path from a positional argument

### Prior Art

- *(Python)* [poetry add](https://python-poetry.org/docs/cli/#add)
  - `git+` is needed for inferring git dependencies, no separate  `--git` flags
  - git branch is specified via a URL fragment, instead of a `--branch`
- *(Javascript)* [yarn add](https://yarnpkg.com/cli/add)
  - `name@data` where data can be version, git (with fragment for branch), etc
  - `-E` / `--exact`, `-T` / `--tilde`, `-C` / `--caret` to control version requirement operator instead of `--upgrade <policy>` (also controlled through `defaultSemverRangePrefix` in config)
  - `--cached` for using the lock file (killercup/cargo-edit#41)
  - In addition to `--dev`, it has `--prefer-dev` which will only add the dependency if it doesn't already exist in `dependencies` as well as `dev-dependencies`
  - `--mode update-lockfile` will ensure the lock file gets updated as well
- *(Javascript)* [pnpm-add](https://pnpm.io/cli/add)
- *(Javascript)* npm doesn't have a native solution
  - Specify version with ``@<version>``
  - Also overloads `<name>[`@<version>]`` with path and repo
    - Supports a git host-specific protocol for shorthand, like `github:user/repo`
    - Uses fragment for git ref, seems to have some kind of special semver syntax for tags?
  - Only supports `--save-exact` / `-E` for operators outside of the default
- *(Go)* [go get](https://go.dev/ref/mod#go-get)
  - Specify version with ``@<version>``
  - Remove dependency with ``@none``
- *(Haskell)* stack doesn't seem to have a native solution
- *(Julia)* [pkg Add](https://docs.julialang.org/en/v1/stdlib/Pkg/)
- *(Ruby)* [bundle add](https://bundler.io/v2.2/man/bundle-add.1.html)
  - Uses `--version` / `-v` instead of `--vers` (we use `--vers` because of `--version` / `-V`)
  - `--source` instead of `path` (`path` correlates to manifest field)
  - Uses `--git` / `--branch` like `cargo-add`
- *(Dart)* [pub add](https://dart.dev/tools/pub/cmd/pub-add)
  - Uses `--git-url` instead of `--git`
  - Uses `--git-ref` instead of `--branch`, `--tag`, `--rev`

### Future Possibilities

- Update lock file accordingly
- Exploring the idea of a [`--local` flag](killercup/cargo-edit#590)
- Take the MSRV into account when automatically creating version req (killercup/cargo-edit#587)
- Integrate rustsec to report advisories on new dependencies (killercup/cargo-edit#512)
- Integrate with licensing to report license, block add, etc (e.g. killercup/cargo-edit#386)
- Pull version from lock file (killercup/cargo-edit#41)
- Exploring if any vendoring integration would be beneficial (currently errors)
- Upstream `cargo-rm` (#10520), `cargo-upgrade` (#10498), and `cargo-set-version` (in that order of priority)
- Update crates.io with `cargo add` snippets in addition to or replacing the manifest snippets

For more, see https://github.com/killercup/cargo-edit/issues?q=is%3Aissue+is%3Aopen+label%3Acargo-add

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

This is intentionally broken up into several commits to help reviewing
1. Import of production code from cargo-edit's `merge-add` branch, with only changes made to let it compile (e.g. fixing up of `use` statements).
2. Import of test code / snapshots.  The only changes outside of the import were to add the `snapbox` dev-dependency and to `mod cargo_add` into the testsuite
3. This extends the work in #10425 so I could add back in the color highlighting I had to remove as part of switching `cargo-add` from direct termcolor calls to calling into `Shell`

Structure-wise, this is similar to other commands
- `bin` only defines a CLI and adapts it to an `AddOptions`
- `ops` contains a focused API with everything buried under it

The "op" contains a directory, instead of just a file, because of the amount of content.  Currently, all editing code is contained in there.  Most of this will be broken out and reused when other `cargo-edit` commands are added but holding off on that for now to separate out the editing API discussions from just getting the command in.

Within the github UI, I'd recommend looking at individual commits (and the `merge-add` branch if interested), skipping commit 2.  Commit 2 would be easier to browse locally.

`cargo-add` is mostly covered by end-to-end tests written using `snapbox`, including error cases.

There is additional cleanup that would ideally happen that was excluded intentionally from this PR to keep it better scoped, including
- Consolidating environment variables for end-to-end tests of `cargo`
- Pulling out the editing API, as previously mentioned
  - Where the editing API should live (`cargo::edit`?)
  - Any more specific naming of types to reduce clashes (e.g. `Dependency` or `Manifest` being fairly generic).
- Possibly sharing `SourceId` creation between `cargo install` and `cargo edit`
- Explore using `snapbox` in more of cargo's tests

Implementation justifications:
- `dunce` and `pathdiff` dependencies: needed for taking paths relative to the user and make them relative to the manifest being edited
- `indexmap` dependency (already a transitive dependency): Useful for preserving uniqueness while preserving order, like with feature values
- `snapbox` dev-dependency: Originally it was used to make it easy to update tests as the UX changed in prep for merging but it had the added benefit of making some UX bugs easier to notice so they got fixed.  Overall, I'd like to see it become the cargo-agnostic version of `cargo-test-support` so there is a larger impact when improvements are made
- `parse_feature` function: `CliFeatures` forces items through a `BTreeSet`, losing the users specified order which we wanted to preserve.

### Additional Information

See also [the internals thread](https://internals.rust-lang.org/t/feedback-on-cargo-add-before-its-merged/16024).

Fixes #5586
@epage epage changed the title Can cargo update update a dependency specification in Cargo.toml? A method to update manifest version requirements (like cargo-edit's cargo-upgrade) Apr 22, 2022
@epage
Copy link
Contributor

epage commented Aug 1, 2023

Hmm, this issue is more about cargo update updating both files at once which is a subset of cargo upgrades behavior and might be part of the subset we don't merge immediately, so this probably isn't the best issue for tracking the cargo-upgrade integration

@epage
Copy link
Contributor

epage commented Aug 8, 2024

#12425 was created specifically for incompatible version upgrades.

The FCPed proposal intentionally left this use case out

Somewhere between deferred and rejected (speaking for myself): Support in cargo update for writing to the manifest for non-breaking changes, like bulk compatible upgrades of version requirements (ie a -save flag) which was one of our lower priority workflows. A --save flag is more about updating versions for your dependents, which while important for having valid lower-bounds on version requirements, doesn't fit with the existing model of cargo update. Maybe in the future we can find a way to express this in cargo update that fits with how it works or maybe another command can take on this role. We just aren't wanting to distract our efforts for handling most of the use cases to handle this one.

See also https://internals.rust-lang.org/t/feedback-on-cargo-upgrade-to-prepare-it-for-merging/17101

@epage epage added the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label Aug 8, 2024
@epage
Copy link
Contributor

epage commented Aug 8, 2024

In the past, we talked about a --save flag. As pointed out in that Internals thread, this seems to align with npm update

Note that by default npm update will not update the semver values of direct dependencies in your project package.json, if you want to also update values in package.json you can run: npm update --save (or add the save=true option to a configuration file to make that the default behavior).

This sounds like our case. Not being familiar with that ecosystem, I can't say whether there are semantics differences that could affect its use as prior art.

Not seeing any other relevant prior art from the tools sampled for cargo add (#10472).

@epage
Copy link
Contributor

epage commented Aug 8, 2024

Potential solutions

Regardless of solution, we need to consider

--save

Semantics:

  • Literally: raise lower bounds of version requirements to make the versions in Cargo.lock to be the minimal version.
  • Literally: Update not just for me but my dependents
  • Semantically: Unblock access to new functionality

Naming:

  • --save is used by npm update
  • But its already "saved" in the lockfile
  • Modifying lower bounds is not the same thing as "saving"

Which packages should be affected?

  • Only those selected by the user
  • Only those that actually got updated
  • All

Workfow impact

  • Editing Cargo.toml, cargo add changing a transitive dependency, etc will cause a mismatch
    • if we also added a flag to cargo add, you wouldn't know you need it until you ran the command and what to name it becomes harder
  • No way to --save without doing a dependency update
  • Requires always remembering the flag

Config

Similar to --save in semantics and in issues. Could also apply to cargo add

If we also do --save, we need a --no-save or --save=false

  • I vaguely remember seeing guidance to avoid --no- flags after I added cargo add but I don't remember where

Workfow impact

  • Editing Cargo.toml will cause a mismatch
  • Some projects want to minimize version req changes to reduce Cargo.toml merge conflicts (they are fine blowing away Cargo.lock on conflict).
    • These projects could just choose to not set the config.
    • Similar for people who maintain two lockfiles

"lockfile higher than version reqs" lint

With a cargo fix to adjust version reqs

Do we go as far as cargo update and cargo add seeing the presence of the lint and auto-fixing?

  • Would this do dirty tracking like cargo fix or not like cargo add, cargo remove, and cargo update

Workfow impact

  • Editing Cargo.toml will cause a mismatch but users will get a warning about it with a suggestion of a command to fix it
  • Some projects want to minimize version req changes to reduce Cargo.toml merge conflicts (they are fine blowing away Cargo.lock on conflict).

@epage
Copy link
Contributor

epage commented Aug 8, 2024

At #5657 (comment), I bring up an idea that came up in a Cargo team meeting for us to develop this feature in place of -Zminimal-versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-new-subcommand Area: new subcommand C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-update S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

2 participants