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

RFC: Deduplicate Cargo workspace information #2906

Closed

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Apr 13, 2020

This proposal is aimed at deduplicating dependency and metadata
directives amongst a set of workspace crates in Cargo with extensions to
the [workspace] section in Cargo.toml and package manifests to
reference this [workspace] section.

Rendered

This proposal is aimed at deduplicating dependency and metadata
directives amongst a set of workspace crates in Cargo with extensions to
the `[workspace]` section in `Cargo.toml` and package manifests to
reference this `[workspace]` section.
@shepmaster
Copy link
Member

shepmaster commented Apr 13, 2020

Each workspace member can then reference this section in the workspace with a new dependency directive

This makes me remember the on-again off-again feature request of "use the same version of X as a dependency". Would it be worth thinking about leaving room in the syntax to support that?

(replied)

or "insert your own idea for how we can go all out" here. In general though I
think there's a lot to be gained from the simplicity of TOML and prioritizing
other tools reading Cargo manifests, so we may not want to go full-blown
templating language just yet.
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the value of inheriting the documentation field in particular will be limited without the ability to template-expand the crate name into the URL, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree yeah that the usefulness in all cases is pretty limited, but that's somewhat intentional as well. I suspect this will largely be used for homepage/license/etc information, but there wasn't much reason to leave out keys like documentation as well. It may be useful for 2-workspace crates though where one's a macro internals and the other is the real crate, where both effectively are documented through the same url.

empower more power users of Cargo to manage workspaces easily without taking
away any of the existing configurations that Cargo already supports.

## Alternative Syntax
Copy link
Member

Choose a reason for hiding this comment

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

Is the workspace = true entry in a dependency table actually necessary? It seems like that could be inferred if the dependency had no version, git, or path entry, right? So the log case would just look like

[dependencies]
log = {}

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have a current technical reason requiring something to be here in that we could likely assign workspace = true meaning to an empty table. That being said though I would personally find the log = {} a bit too "small" in that it's sort of weird seeing no other indication of where the dependency comes from. I'd hope that workspace = true wouldn't be too burdensome though to list on lots of repeated dependencies.

Put another way, I think that this is a technically viable option, but I would say that for learnability and skimming crates it'd be nice to have something listed there to remind readers to look at the workspace manifest. I'm curious what others think though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the common case is going to be

[dependencies]
foo1 = { workspace = true }
foo2 = { workspace = true }
foo3 = { workspace = true }
foo4 = { workspace = true }
foo5 = { workspace = true }
...

with all or almost all crates being redirected to the workspace.

Which kind of asks for refactoring it into something looking like

[dependencies.workspace]
foo1
foo2
foo3
foo4
foo5
...

(but with valid syntax etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

One of the more compact ways I've thought of is

[dependencies]
foo = "ws"

Copy link
Member

Choose a reason for hiding this comment

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

Note that the main syntax can also be written

[dependencies]
foo.workspace = true

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the foo = "ws".

Choose a reason for hiding this comment

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

I like the foo = "workspace". Elaborate and concise at the same time.

Copy link

@hbina hbina Aug 15, 2020

Choose a reason for hiding this comment

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

[dependencies]
foo1 = { workspace = true }
foo2 = { workspace = true }
foo3 = { workspace = true }
foo4 = { workspace = true }
foo5 = { workspace = true }

I disagree, I like this. I think this allows for future updates to this features. Frankly, using strings are arbitrary and will possibly make for a mess down the line when people try to extend this stuff. How about supporting something like

[dependencies]
foo1, foo2, foo3, foo4, foo5 = { workspace = true }

Which is a lot better imo because it solves the problem of repetition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that valid toml?

* Allow all keys to be inherited from a workspace
* Also allow all keys to reference other packages explicitly.
Comment on lines +503 to +504
Put another way, `Cargo.toml` files published to crates.io, or metadata found
through crates.io, won't change from what they are today.

Choose a reason for hiding this comment

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

Overall, this is a great RFC that addresses some papercuts I've seen in https://github.com/tokio-rs/tracing. I do have a question: does this RFC make a cargo publish --all (that publishes all unpublished crates in a workspace) easier to support or is RFC orthogonal to that feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this makes that either harder or easier per se, since this is, internally in Cargo at least, mostly just "syntax sugar". I would guess that an implementation of publish --all wouldn't have to specifically handle anything in this rfc (in that it doesn't make it harder), and it would also have to handle everything still (this doesn't make it easier)

Choose a reason for hiding this comment

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

@davidbarsky I implemented a nice way of doing that (using dep graph of only the crates you are publishing) in https://github.com/pksunkara/cargo-workspaces

@joshtriplett
Copy link
Member

When defining the workspace table keys, and paths in workspace dependencies, you should explicitly state at that point that all paths are relative to the workspace Cargo.toml and will be adjusted to be relative to where they're substituted.

Copy link
Member

@joshtriplett joshtriplett left a comment

Choose a reason for hiding this comment

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

Couple of typos.

text/0000-cargo-workspace-deduplicate.md Outdated Show resolved Hide resolved
text/0000-cargo-workspace-deduplicate.md Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

Thank you for working on this!

@joshtriplett
Copy link
Member

On the question of whether to explicitly mark inherited metadata, I can see reasons for both directions there.

I do absolutely think that publish should be inherited by default; that provides substantial value.

One thought, though: suppose you specify a field in the workspace metadata, and a crate in the workspace wants to omit that field. Not fill it in with a blank string, omit it. How could it do so? (Example: description, or license-file.)

@alexcrichton
Copy link
Member Author

@joshtriplett

When defining the workspace table keys, and paths in workspace dependencies, you should explicitly state at that point that all paths are relative to the workspace Cargo.toml and will be adjusted to be relative to where they're substituted.

Done now! (I think)

Good suggestion!

One thought, though: suppose you specify a field in the workspace metadata, and a crate in the workspace wants to omit that field. Not fill it in with a blank string, omit it. How could it do so?

That's a good point yeah and one that isn't supported with the proposal as-is. TOML doesn't have a natural "null" value so we don't have an easy opt-in, but we could perhaps co-opt documentation = {} if this is necessary.

One alternative to what's written down is that we could only allow the documentation = { workspace = "other-member" } form and disallow inheritance from [workspace] entirely. It does mean that you'll still have a big header in Cargo.toml with lots of workspace = "..." pieces, but it would solve this issue.

Do you have a feeling for how often you think this'll come up in practice?

@joshtriplett
Copy link
Member

joshtriplett commented Apr 15, 2020

@alexcrichton I do like the workspace = true syntax to get a value from the workspace metadata rather than a specific other crate. That feels more natural to me.

Regarding how often workspace crates will not want the "automatic inheritance" behavior: more often with some fields than others. The pattern of "most crates have this value, but a few don't" is not at all uncommon, particularly for internal usage. Internal-only crates may have publish = false and a different license value, for instance. A large workspace may have the same repository value for most crates, but a few might get pulled in by submodule or subtree and also not specify repository. Or a workspace using edition = "2018" for almost everything may vendor a crate that implicitly uses the 2015 edition.

Here's another alternative proposal, which has the advantage of minimizing boilerplate while still being explicit. What if we default to requiring you to write field = { workspace = true } to get metadata from the workspace, but allow you to explicitly write things like [workspace] field = { value = "field value", inherit = true } if you want auto-inheritance? That way, if you have a workspace where you want the same values everywhere, you can easily get that by writing all the metadata with inherit = true in your workspace Cargo.toml once, but if you have some values you want to deduplicate in your workspace but not use unless explicitly specified, you won't accidentally get a value inherited without expecting that behavior.

How does that sound? That would avoid requiring people to type field = { workspace = true } everywhere (which may be longer than the actual value, partially defeating the purpose).

@alexcrichton
Copy link
Member Author

@shepmaster

This makes me remember the on-again off-again feature request of "use the same version of X as a dependency". Would it be worth thinking about leaving room in the syntax to support that?

Oops sorry I forgot to reply to this earlier!

I didn't really consider this at all when writing up this proposal, and thinking about it a bit now I would say that we probably don't need to consider it much here. I would be pretty hesitant to try to lump it in and I suspect that the desired syntax for doing this would likely not conflict with the syntax we choose here, or we could always design it later to not conflict.

bors added a commit to rust-lang/cargo that referenced this pull request Apr 13, 2022
Part 5 of RFC2906 - Add support for inheriting `rust-version`

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548

This PR focuses on adding support for inheriting `rust-version`. While this was not in the original RFC it was decided that it should be added per [epage's comment](#8415 (comment)).
- Changed `rust-version` to `Option<MaybeWorkspace<String>>` in `TomlProject`
- Added `rust-version` to `TomlWorkspace` to allow for inheritance
- Added `rust-version` to `InheritableFields1 and a method to get it as needed
- Updated tests to include `rust-version`

Remaining implementation work for the RFC
- Switch the inheritance source from `workspace` to `workspace.package`, see [epage's comment](#8415 (comment))
- Add `include` and `exclude` now that `workspace.package` is done, see [epage's comment](#8415 (comment))
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations, as needed
bors added a commit to rust-lang/cargo that referenced this pull request Apr 13, 2022
Part 6 of RFC2906 - Switch the inheritance source from `workspace` to…

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563

This PR focuses on switching the inheritance source from `workspace` to `workspace.package`. The RFC specified `workspace` but it was decided that it should be changed to `workspace.package` per [epage's comment](#8415 (comment)).
- Moved fields that can be inherited to ` package: Option<InheritableFields>` in `TomlWorkspace`
  - Making `package` `Option<InheritableFields>` was done to remove duplication of types and better signify what fields you can inherit from in one place
- Added `#[serde(skip)]` to `ws_root` and `dependencies` in `InheritableFields` since they will never be present when deserializing and we don't want them present when serializing
- Added a method to update `ws_root` and `dependencies` in `InheritableFields`
  - Added for `ws_root` since it will never be present in a `Cargo.toml` and is needed to resolve relative paths
  - Added for `dependencies` since they are under `workspace.dependencies` not `workspace.package.dependencies`
- `workspace.dependencies` was left out of `workspace.package` to better match `package` and `package.dependencies`
- Updated error messages from `workspace._` to `workspace.package._`
- Updated `unstable.md` to reflect change to `workspace.package`
- Updated tests to use `workspace.package`

Remaining implementation work for the RFC
- Add `include` and `exclude` now that `workspace.package` is done, see [epage's comment](#8415 (comment))
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations, as needed
bors added a commit to rust-lang/cargo that referenced this pull request Apr 14, 2022
Part 7 of RFC2906 - Add support for inheriting `exclude` and `include`

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564

This PR focuses on adding support for inheriting `include` and `exclude`. While they were not in the original RFC it was decided that they should be added per [epage's comment](#8415 (comment)).
- Changed `include` and `exclude` into `Option<MaybeWorkspace<Vec<String>>>` inside `TomlProject`
- Added `include` and `exclude` to `InheritbaleFields`
- Updated tests to verify `include` and `exclude` are inheriting correctly

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations, as needed
bors added a commit to rust-lang/cargo that referenced this pull request Apr 14, 2022
Part 8 of RFC2906 - Keep `InheritableFields` in a `LazyCell` inside `…

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564
Part 7: #10565

This PR focuses on optimizations surrounding `InheritabeFields` and searching for a `WorkspaceRootConfig`:
- Keep `InheritableFields` in a `LazyCell` inside `to_real_manifest` so we only search for a workspace root once per `to_real_manifest`
- Changed calls for getting a workspace root to use `inherit_cell.try_borrow_with()`

[Testing Framework](https://gist.github.com/Muscraft/14f6879af27500e34584296edb468d15)
Test Results:
nest=1, runs=7, members=75, step=25
- 25 Members:
  - Optimized: 0.145s
  - Not Optimized: 0.181s
  - Normal: 0.141s
  - Percent change from Normal: 2.837%

- 50 Members
  - Optimized: 0.152s
  - Not Optimized: 0.267s
  - Normal: 0.141s
  - Percent change from Normal: 7.801%

nest=2, runs=7, members=75, step=25
- 25 Members
  - Optimized: 0.147s,
  - Not Optimized: 0.437s
  - Normal: 0.14s
  - Percent change from Normal: 5.0%

- 50 Members
  - Optimized: 0.159s
  - Not Optimized: 1.023s
  - Normal: 0.141s
  - Percent change from Normal: 12.766%

Using a `LazyCell` for `InheritableFields` brought down runtimes to more acceptable levels. It is worth noting that this is a very harsh test case and not one that represents real-world scenarios. That being said there are still some optimizations that could be done if we need to. The biggest one is making sure we only parse a `Cargo.toml` once.

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- More optimizations, as needed
bors added a commit to rust-lang/cargo that referenced this pull request 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
bors added a commit to rust-lang/cargo that referenced this pull request Apr 20, 2022
Prefer `key.workspace = true` to `key = { workspace = true }`

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

Part 1: #10497
Part 2: #10517
Part 3: #10538
Part 4: #10548
Part 5: #10563
Part 6: #10564
Part 7: #10565
Part 8: #10568

This PR fell out of [this discussion](#10497 (comment)) regarding if `key.workspace = true` is better than `key = { workspace = true }`.

Changes:
- Made all fields into `field.workspace = true`
- Made dependencies into `dep.workspace = true` when a they only specify `{ workspace = true }`

Remaining implementation work for the RFC
- `cargo-add` support, see [epage's comment](#8415 (comment))
- Optimizations as needed
bors added a commit to rust-lang/cargo that referenced this pull request Apr 28, 2022
Cargo add support for workspace inheritance

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

This PR adds all the required support for workspace inheritance within `cargo-add`. It was split up across a few different PRs as it required `snapbox` support from #10581 and a new `toml_edit` version from #10603. `@epage` and I decided to go ahead with this PR and add in some of the changes those PRs made. `@epage's` name on the commits is from helping to rewrite commits and some very minor additions.

Changes:
  - #10585
  - Muscraft#1
  - Muscraft#3
  - Muscraft#2
  - Muscraft#4

r? `@epage`
bors added a commit to rust-lang/cargo that referenced this pull request Apr 28, 2022
…=epage

Update documentation for workspace inheritance

Tracking issue: #8415
RFC: rust-lang/rfcs#2906

This updates documentation about workspace inheritance in the Cargo Book. This is meant to move the documentation into a state that is acceptable to move after stabilization. It currently proposes adding sections to `workspaces.md` and `specifying-dependencies.md`.

r? `@epage`
@folex
Copy link

folex commented Jan 12, 2023

workspace = true is an amazing feature! It allows to keep all inner dependencies definitions (specified by path) in one place!

But if I understand correctly, it leads to some repetitions between workspace.members and workspace.dependencies. Here's an example that I've stumbled upon:

Crate configuration

/Cargo.toml

[workspace]
members = [
    "crates/libp2p",
    "crates/waiting-queues",
    "spell-storage",
    "foo"
]

[workspace.dependencies]
libp2p = { path = "crates/libp2p" }
waiting-queues = { path = "crates/waiting-queues" }
spell-storage = { path = "spell-storage" }
foo = { path = "foo" }

/foo/Cargo.toml

libp2p = { workspace = true }
waiting-queues = { workspace = true }
spell-storage = { workspace = true }

Problem

You can see that workspace.members and workspace.dependencies repeat each other for the inner (local) crates.

Here's a PR that moves to workspace = true for all inner dependencies. You can see it has a lot of path duplication in /Cargo.toml fluencelabs/nox#1388

Cargo.toml from fluencelabs/rust-peer PR 1388

Possible solution

One solution I see is to have workspace.members automatically among workspace.dependencies. Something like that:

/Cargo.toml

[workspace]
members = [
    "crates/libp2p",
    "crates/waiting-queues",
    "spell-storage",
    "foo"
]

[workspace.dependencies]
# empty!

/foo/Cargo.toml

libp2p = { workspace = true }
waiting-queues = { workspace = true }
spell-storage = { workspace = true }

@epage
Copy link
Contributor

epage commented Jan 12, 2023

@folex

Before answering Looking over my comments below, it feels like there is enough to discuss that burying this in a closed RFCs comments might make it harder to follow. It might be good for us to move to internals or a cargo issue

Instead of that proposal, the RFC included

This behavior should mean that you no longer need to write version = "..." with path dependencies if you publish to crates.io. Coupled with the workspace-level dependencies above this means you never have to write the version of a path dependency anywhere!

which was rejected in prep for stablization. see my comment

It doesn't look like your proposal runs into similar issues.

The questions that would need to be answered

  • What do the implicit workspapce.dependencies look like?
    • I'm assuming member = { version = "<x>.<y>.<z>", path = "...." }
    • Is always requiring the latest of a workspace member a good enough default? I assume so as you can't test with prior versions
    • Should it be context-sensitive and we skip the version field on dev-dependencies to allow publishing with circular dependencies, like what cargo add does?
    • Is default-features = true a good enough default?
    • Does this break down for alternative registry users and is that sufficient enough to block crates.io users getting this?
  • What happens when an explicit entry is created?
    • I assume it overrides the implicit entry
  • What happens when a package name is ambiguous?
    • I hadn't thought of this before but I think I recently saw a discussion of someone who had reused package names within a workspace, so I guess its possible
  • Seeing as the generated result isn't easily viewable (cargo package and then break apart the package), are there enough opportunities for users to feel uncertainty or confusion over what is happening from the lack of transparency? How do we help these users?
    • This would be a point against being context-dependent
    • Without context-dependence, it feels like this would be harder to for someone to discover the root cause of their circular dependency publishing issue and know how to resolve it (or understand what someone's explanation says)

Use cases we need to consider besides the standard one:

  • clap needs a = dependency on clap_derive because clap_derives proc macro generates code for a specified version of clap
    • Can be handled by an explicit override or not using dep.workspace = true
  • clap_complete doesn't want to force clap features on dependents, so uses default-features = false and specifies only what is needed
    • Can be handled by an explicit override or not using dep.workspace = true
    • Concern: While internal and binary packages don't matter, public libs packages likely should be doing this (when features are available) which makes me wonder if we would be encouraging wrong practices by having this feature.
  • At one point clap_derive had tests that required clap despite clap depending on clap_derive. To publish, we had to make clap_derives dev-dependency exclude the version field so it'd be ignored on publish. While we no longer have that situation (tests moved to clap), other crates do
    • Can be handled by an explicit override or not using dep.workspace = true
    • See also the "questions" section for the idea of it being context-dependent

panhania added a commit to google/fleetspeak-rs that referenced this pull request Jun 29, 2023
It was initially removed because [RFC #2906][1] mentioned it is no
longer necessary and the version is inferred. However, the version that
was actually stabilized does not include this change. For more details
details, see the "Changes from RFC" section of the [Cargo issue][2].

[1]: rust-lang/rfcs#2906
[2]: rust-lang/cargo#8415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
No open projects
Status: Done (Stabilized)
Development

Successfully merging this pull request may close these issues.