add a flag to `cargo add` to put dependency in workspace and inherit it

[Muscraft]

Problem

There is no way to tell cargo add that you want to add a dependency from a workspace explicitly. It is currently only done implicitly if you run cargo add foo and there happens to be a workspace dependency that matches it. There is also no way to add a dependency to a workspace and some of its members at the same time.

Proposed Solution

add flags similar to --inherit and --workspace foo,bar that explicitly allows you to add a dependency from a workspace and add a dependency to a workspace and some of its members.

Notes

This fell out of #10606. During the discussion of what should be included in cargo add support for workspace inheritance before workspace inheritance gets stabilized. The implementation of this should ideally not happen before stabilization of workspace inheritance.

--inherit: This would be the main one to add a dependency from a workspace. Usage similar to cargo add foo --workspace -p bar, which would add foo to bar from a workspace. This could error if the workspace does not define that dependency or add it to the workspace if not found. features and optional are allowed to be used with this. features needs to be talked about as it might be good to add it to the workspace dependency features if the workspace dependency is not found. optional can only go to the inheriting package.

--workspace foo,bar This would be to add a new workspace dependency to a workspace and then to the members that are specified, 1 member is required. Any flags does not conflict with [workspace.dependencies] would be allowed to be defined as well here and they would always go to the workspace dependency.

[epage]

Right now, you can explicitly name the source for a dependency (--path, --git, foo@version) but we will also infer the source (foo). We currently

• Check for an existing dependency in the same dependency table in the package
• Check for a workspace dependency
• Check for a workspace member
• Check for an existing dependency in a different dependency table in the package

But there is no way to explicitly state you want a workspace dependency (and have it error if it doesn't exist).

There is also no way to explicitly add a dependency to workspace.dependencies

Some questions I have

• Should adding a dependency with a workspace source error or automatically add the dependency
  • If automatically add, which dependency do we add the features to?
• Do we need a dedicated way of editing workspace.dependencies without touching a package's dependencies?
• Would it be confusing to --workspace flag for either case (seeing as that normally means "apply to all workspace members")

[epage]

--workspace foo,bar This would be to add a new workspace dependency to a workspace and then to the members that are specified, 1 member is required. Any flags does not conflict with [workspace.dependencies] would be allowed to be defined as well here and they would always go to the workspace dependency.

imo I find it confusing that you pass package names through the --workspace flag.

[Muscraft]

I thought about suggesting --workspace and --workspace --members foo,bar but I didn't love that. It could be changed to --inherit and --inherit --members foo,bar

[epage]

imo there isn't really a reason to specify multiple members at once. We don't have any other way of adding dependencies to multiple packages. The only case I can think of is an option to migrate a workspace to workspace dependencies but since that is "one time", I don't see as much value.

[Muscraft]

In that case, do we go with --inherit and --workspace?

[epage]

Could you clarify how the flags differ? I feel like that wasn't too clear to me

[Muscraft]

I think there should be two different flags one for inheriting a workspace dependency and one for adding a dependency to a workspace and then inheriting it (it would also just be for adding a dependency to a workspace or overriding one).

--inherit would be for just inheriting a workspace dependency. That way you can limit the other flags to be used with it to only those supported and have nothing related to changing a source. This would be used for inheriting, adding features, or setting optional.

--workspace would be for adding a dependency to [workspace.depndencies], setting the source/version, and changing features. You could maybe extend this so that when trying to add a dependency and it is defined in a member it would switch it to foo.workspace = true if they're compatible.

Removing setting members for --workspace removes some of its functionality and I can understand if only one flag is wanted or needed.

[bindsdev]

I created an issue today proposing similar functionality, but it was closed in favor of this one (wasn't aware I was making a duplicate). Just going to present my ideas to further the conversation and to help eventually determine what the best approach would be. My proposal was as follows:

Adding to [workspace.dependencies]

Adding to the [workspace.dependencies] section could be done through a --workspace option. Chapter 3.3 of the Cargo Book, under The dependencies table section, states that:

• Dependencies from this table cannot be declared as optional

This would mean that the --workspace option would have to be mutually exclusive with the --optional option.

Adding workspace-inherited dependencies

Workspace-inherited dependencies could be added through a --workspace-inherited option. Chapter 3.1 of the Cargo Book, under the Inheriting a dependency from a workspace section, states that:

• Other than optional and features, inherited dependencies cannot use any other dependency key (such as version or default-features).

This would mean that the --workspace-inherited option would have to be mutually exclusive with all of the Source options, and all of the Dependency options besides --optional, --no-optional, --default-features (since it doesn't add to the dependency keys), --features, and --dry-run.

The --workspace-inherited option would just add crate_name.workspace = true to the desired dependencies section or crate_name = { workspace = true, ... } if --optional and/or --features were specified.

[NickLarsenNZ]

I think a -w shorthand for --workspace would be nice too.

~And also think it wouldn't hurt to automatically add the crate_name.workspace = true to the member's Cargo.toml if that dependency was added a the workspace level.~ That already works :+1:

I'm not a fan of --members foo,bar as I believe the -p option does a good enough job (unless I have missed an important distinction).

[epage]

In my experience, it seems people are all or nothing about inheriting from a workspace.

We could

• Detect the presence of inheriting dependencies and fallback to detecting inheritable dependencies if present and continue the pattern
  • This would act like sorting of dependencies
  • Do we make this the default i# a [workspace] is present or require people to move a dep over first?
• Add a lint for inheriting and check if its in [lints] and allowed. We wouldn't be able to check group membership if this is in clippy

I feel like inheriting non-source information was a mistake so I think we should only put that in the workspace.

[epage]

To add real quick, we have several potential heuristic ideas

• if workspace.dependencies is present, use it
  • Downside: if people only use workspace.dependencies for de-duplication, they will be annoyed (but I discourage that style anyways)
• if dependencies is only inherited, use it
  • Downside: this will mean new packages within a workspace or packages with exceptions won't get the functionality. Once a package has their first inherited dependency, it will work going forward.

[heaths]

I don't think we need new flags: if someone specifies both --package with the package name(s?) to which they want the dependency added and --manifest-path pointing to the workspace, if the --manifest-path is indeed a workspace e.g., has [workspace.dependencies], add the dependency details to the workspace and an inherited dependency to the listed package(s).

But, in a monorepo, ideally my partners don't have to specify anything differently. A lot of this discussion is about trying to avoid regressions. I appreciate that. So instead, why not have monorepo maintainers opt into this behavior via .cargo/config.toml e.g.,

[dependencies]
inherit = true

If present and the workspace indeed has [workspace.dependencies], do the same behavior I mentioned above but without requiring additional flags e.g., within a crate directory, I can simply run cargo add serde and serde is added, if not already, to the workspace dependencies and an inherited dependency in the current crate?

[epage]

I agree about not having a new flag for this. There is a UX and maintenance cost to any flag or config we add. I think we can go far with heuristics.

I don't think we should be doing heuristics off of --manifest-path. That doesn't tell us much, especially if there is root package.

As I hinted before, I don't think this justifies a config.

[epage]

On a different note, I wonder if the situation with #12162 is bad enough that we should block on that issue.

[heaths]

I agree about not having a new flag for this. There is a UX and maintenance cost to any flag or config we add. I think we can go far with heuristics.

Is it commonly expected that having a workspace means devs want workspace dependencies? In a lot of projects I've seen that use workspaces, they still have package dependencies. That's the only reason I suggested a config: the default behavior will change. Maybe it is for the better, but the change may not be expected. Or would heuristics include whether existing dependencies are inherited or not?

[epage]

See https://github.com/rust-lang/cargo/issues/10608#issuecomment-1984549127

[heaths]

Sorry. I did skim through the comments but obviously missed that one. That said, thoughts about whether there's a mix and for any /.*dpenendencies/ section? I can imagine a case where some or even most dependencies are inherited, but some the maintainers feel are specific to a crate are declared therein. I imagine this especially true in dev-dependencies. Maybe it doesn't happen a lot, but still feels like something to consider. IMO, erring on the side of crate-specific dependencies seems right. They can almost more things to inherit, but inheriting has downstream ramifications other crate maintainers may not expect (when in a monorepo).

[epage]

The second option says to only inherit if everything is inherited. In the mixed situation you mentioned, users would not get anything inherited automatically.

Overall, I'm fine with not covering every use case thereby making this more opinionated.

[robjtede]

if dependencies is only inherited, use it

This is a very safe assumption, and a subset of the behavior on the first point that could be amended.

(but I discourage that style anyways)

+1. By enabling this feature via the latter point, we'd be actively discouraging the deduplication-only style (without needing to display errors to the hold-outs).

[Rigidity]

I'd like to move forward this feature, since I work with workspaces often.

I've read through a bit of the discussion - here is a summary of my thoughts:

The core idea is that there should be some way to add a dependency to the workspace dependency list, and inherit it in any number of workspace members, at the same time. Personally, only the version should exist in the workspace dependencies, and the inherited dependency should include any features added.

Side note: The formatting should be consistent, whether you're already using thing.workspace = true or thing = { workspace = true } (personally the latter is better since you can easily add features later, but it currently uses the former indiscriminately.

A few options for UX:

1. A --workspace or -w flag, such as cargo add serde -F derive -p my-member -w
2. Automatically do this if the workspace already has workspace dependencies
3. Automatically do this based on a config parameter
4. Always do this for workspaces

Personally, 4 seems most controversial since many workspaces don't use workspace dependencies, and there would be no clear way to opt out. It would also be a backwards incompatible change.

I think that either 1 or 2 is ideal, but don't have strong opinions on it.

I'm not sure what the next steps should be on this, but perhaps I should work on a draft PR implementation?

[Rigidity]

I'll expand on some possibilities for the first option:

Add to workspace dependencies only: cargo add serde -w

Inherit dependency only (or add without inheriting if it's not a workspace dependency): cargo add serde -p my-member

Do both: cargo add serde -w -p my-member

Add a feature for the dependency in the member crate: cargo add serde -w -p my-member -F derive

Add a dependency to multiple members (this isn't clearly necessary but could be useful): cargo add serde -w -p first,second

Hoist the feature into the workspace dependency (and remove it from members?): There could be a --hoist-features flag or something like that, but I'm not convinced this is a common enough use case.

[epage]

The core idea is that there should be some way to add a dependency to the workspace dependency list, and inherit it in any number of workspace members, at the same time. Personally, only the version should exist in the workspace dependencies, and the inherited dependency should include any features added.

(emphasis added)

Adding to any number of workspace members, migrating to workspace dependencies, or only adding to the workspace have previously not been a part of this plan. The plan for this has been "when you add the specified deps to the current package, write through to workspace.dependencies

In general, I am concerned about moving forward with this with the state of default-features, see #12162.

A few options for UX:

We were already leaning towards (2), see https://github.com/rust-lang/cargo/issues/10608#issuecomment-1984549127

Side note: The formatting should be consistent, whether you're already using thing.workspace = true or thing = { workspace = true } (personally the latter is better since you can easily add features later, but it currently uses the former indiscriminately.

The only existing-style check we do is for sort order. In every other way, we generate output in an opinionated format.

[Rigidity]

If it's automatic, here's what I'd propose:

cargo add serde -p member -F derive

1. If you're not in a workspace, add it normally
2. If it is already in the workspace dependencies, inherit
3. Otherwise, if all dependencies of the member crate (or perhaps all members are considered in this heuristic) you're adding it to are already set to workspace = true, it can be inherited
4. In the case of no dependencies yet or mixed, it will default to not inheriting (I'd prefer to allow --inherit to be explicit if you want to override this)

[Rigidity]

The only existing-style check we do is for sort order. In every other way, we generate output in an opinionated format.

Is it not possible to add such a check? I think consistency is important in Cargo.toml, and this is one of the few places where the CLI produces inconsistent output that I have ended up having to retype manually.

As you mentioned, overrides for features, optionality, etc, should be in the inherited dependency in members rather than the workspace dependency list, so it makes more sense for the default to be an inline table rather than the shorthand boolean value IMO. So if nothing else, maybe a separate proposal to change the default formatting rather than checking style.

[epage]

Is it not possible to add such a check? I think consistency is important in Cargo.toml, and this is one of the few places where the CLI produces inconsistent output that I have ended up having to retype manually.

This was a deliberate choice when we merged cargo add into cargo. As the Cargo.toml style guide is developed, we'll move what we can to that. Otherwise, this is in a state like cargo fix where formatting may need to be applied afterwards.

[Rigidity]

Thanks for the context, I suppose it might make sense to add a minimal implementation now and it can be expanded on in the future.

[Rigidity]

Do you think it's fine for me to open a PR this weekend to implement what has been discussed thus far?

[epage]

As I said earlier

In general, I am concerned about moving forward with this with the state of default-features, see #12162.

Maybe we could just not support this, either

• Erroring if we'd add or edit a workspace.dependencies entry but --default-features or --no-default-features is set
• Always add to dependencies when --default-features or --no-default-features is set, error if we'll edit a workspace = true dependency.

[Rigidity]

I think the first would be fine for now, and if you want to enable or disable default features it could be done manually until that issue is resolved.

The error could be something along the lines of:

Default features of workspace dependencies cannot be disabled via cargo add, since it could have unintended side effects. You can edit the workspace manifest as needed to achieve this instead.

[heaths]

Could we go even further given rust's generally actionable error messages, pointing to further information if needed? The crux of the issues with default-features is, of course, that they are additive. If a manifest already inherits a dependency but in a crate manifest has default-features = false, they are already not getting the expected result. Erring makes sense, but perhaps an error message could prescribe a solution e.g.,

Package dependencies inherited from a workspace cannot disable default features since features are additive within a workspace. You can instead disable default features in the workspace manifest and enable only those features you need in each package manifest. Note that all referenced features will be compiled, so any mutually exclusive features will have unintended side effects.

If a bit wordy, perhaps it could link to more information about workspace dependencies.

Erroring if we'd add or edit a workspace.dependencies entry but --default-features or --no-default-features is set

For clarification, do you mean erring on any entry or just one being cargo added to a package manifest that already exists in a workspace? Though, in either case, the dev is likely already facing possible unintended side effects so this shouldn't maybe isn't a blocker to improving cargo add now, is it?