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

Implicit Inheritance for Workspapce Inheritance #12208

Open
Tracked by #79 ...
epage opened this issue May 31, 2023 · 8 comments
Open
Tracked by #79 ...

Implicit Inheritance for Workspapce Inheritance #12208

epage opened this issue May 31, 2023 · 8 comments
Labels
A-manifest Area: Cargo.toml issues C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.

Comments

@epage
Copy link
Contributor

epage commented May 31, 2023

Problem

The workspace inheritance RFC deferred inheriting-by-default to not get in the way of merging a minimal solution

Use cases

  • Enable example packages to inherit optional fields while allowing the package to stand on its own (comment)

Proposed Solution

Possibilities:

  • Workspace directives could be implicitly and automatically inherited to members. In the future, however, Cargo will want to support nested workspaces, and it's unclear how these features will interact. In order to strik a reasonable middle-ground for now a simple solution which should address many use cases is proposed and we can continue to refine this over time as necessary.
  • Directives could be flagged to be explicitly inherited to workspace members as an optional way of specifying this. For now though to keep this proposal simple this is left out as a possible future extension of Cargo.

Notes

No response

@epage epage added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` A-manifest Area: Cargo.toml issues S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels May 31, 2023
@epage
Copy link
Contributor Author

epage commented May 31, 2023

I feel like we need to gather use cases to decide if this is even worth it, so I've created this as a place to gather them.

@Jasper-Bekkers
Copy link

We'd like to have this feature because that the moment having to opt-in to [lints] is a no-go for us. We have medium sized workspace with about ~150 crates in there and we're frequently adding new ones. Even though cargo new automatically adds the [lints] workspace = true section, it still requires us to catch in code reviews that this has been done properly (maybe not everybody uses cargo new etc).

Ideally we can just force the clippy lints for the entire workspace & project so that we don't have to keep this in mind and we can just set it once. It's how we're currently using / abusing the [target.'cfg(all())'] rustflags hack and it's serving us quite well.

@epage
Copy link
Contributor Author

epage commented Nov 16, 2023

Not great but a workaround would be to have a tool that errors if a workspace member does not have it inherited.

With #12235, that could be integrated as a cargo lint that is on by default.

If we had general implicit inheritance, we'd need a way to override it. Take workspace.package.rust-version and you want one package to not have it set at all. There is no "opt-out" value for that field. TOML doesn't have a None value. We could make it support false but that only works for that field and we'd need to handle it piece-meal. We could support package.rust-version.workspace = false to mean "don't inherut bit don't set it to something"

Say we don't want to go down that route, we'd then need to decide if we're ok with implicit inheritance being section by section. I feel like that could lead to its own confusion.

We also need to consider what routes (and their likelihood) we might want to take workspace inheritance. At least for [lints], I could see users wanting support for named groups of lints to inherit if there are very disparate use cases in a workspace, like firmware and application code. If we do implicit inheritance, we'd need to evaluate if we want to take into account features like this and look into their ramifications.

@Nemo157
Copy link
Member

Nemo157 commented Nov 16, 2023

For lints at least I feel like having explicit pushing from the workspace would work fine in most cases, it doesn't need to be implicit if you only have to configure it in one spot.

@epage
Copy link
Contributor Author

epage commented Nov 30, 2023

@Muscraft and I did some brainstorming on this.

Inspired by the inherits field for Custom Profiles,

inherits = true

[package]
name = "foo"

...

inherits = true would be the same as if all package fields and lints had workspace = true.

  • Dependencies are a weird case. We don't want to automatically add them. I'm also slightly uncomfortable with dependencies without any source set.
  • I dislike that the naming of inherits is decoupled from .workspace = true but I don't have a better idea.
  • I did inherits instead of package.inherits to make it obvious it applies to everything, like cargo-features. However, this doesn't work well in the presence of workspaces

With a new edition, we could have a rust-2027-compatibility lint that encourages people to set inherits = false when it is unset and make inherits = true the default. While the related Pre-RFC and RFCs shouldn't be procrastinated too long, I don't think 2027 is too bad of a time frame because the presence / absence of inherits = true is more likely to be obvious than lints.workspace = true. The biggest concern is [lints] but forgetting inherits will also make you lose out on everything else (inheriting package.version, package.rust-version, package.edition, etc).

One future possibility to keep in mind is the idea of workspace inheritance of sets/groups. These are sets of inheritable fields that you can opt-in on a named basis

[workspace.group.public.package]
rust-version = "1.60.0"
edition = "2018"

[workspace.group.internal.package]
inherits = "some-other-group"
rust-version = "1.73.0"
edition = "2021"

[package]
name = "foo"
rust-version.workspace = "internal"
edition.workspace = "internal"
inherits = "public"

[package]
name = "bar"
  • Maybe it can be group.internal.package without workspace.
  • We could possibly support packages that inherit from (workspace member) packages, like profiles, but that might encourage unnecessary coupling and gets weird with dependencies.

@jplatte
Copy link
Contributor

jplatte commented Dec 5, 2023

Maybe inherits = "lints" could be a thing too? … and a more conservative default for the upcoming edition, if other fields inheriting by default is controversial.

@jplatte
Copy link
Contributor

jplatte commented Jan 3, 2024

After re-reading, I had an idea for how use cases that include example crates could benefit without an edition bump: Treat lints.workspace = true outside a workspace as a warning, not an error (and act as if the lints table was absent). I feel like this shouldn't be hard to generalize to any field that can be inherited from the workspace, and from my POV combines well with .workspace = false acting as a way of explicitly resetting a field to its default (possibly null) value.

@epage
Copy link
Contributor Author

epage commented Jan 3, 2024

For myself, I dislike downgrading lints.workspace = true from an error to a warning as we are then running counter to what the user said. I don't feel like the example use case is large enough to justify that type of sloppiness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.
Projects
None yet
Development

No branches or pull requests

4 participants