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

Support the [features] section in virtual manifests #4942

Closed
twistezo opened this issue Jan 15, 2018 · 18 comments
Closed

Support the [features] section in virtual manifests #4942

twistezo opened this issue Jan 15, 2018 · 18 comments
Labels
A-features Area: features — conditional compilation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-propose-close Status: A team member has nominated this for closing, pending further input from the team

Comments

@twistezo
Copy link

I have project structure:

+---workspace_a
¦   ¦   Cargo.toml
¦   ¦   
¦   +---src
¦           lib.rs
¦   Cargo.lock
¦   Cargo.toml

Main manifest ./Cargo.toml:

[workspace]
members = ["workspace_a"]

[dependencies.workspace_a]
path = "./workspace_a"

[features]
only_a = [
    "workspace_a/only_a"
]

Workspace manifest ./workspace_a/Cargo.toml:

[package]
name = "workspace_a"
version = "0.1.0"
authors = ["author.com"]

[features]
only_a = []

And ./workspace_a/lib.rs:

#[test]
fn test_a() {
    assert_eq!(1, 1);
}

#[cfg(not(feature = "only_a"))]
#[test]
fn test_b() {
    assert_eq!(1, 1);
}

When i'm in root and trying cargo test --all --features only_a virtual manifest doesn't see feature in my workspace.

It's working when i'm in ./workspace_a/ or when main manifest it's not virtual.

@alexcrichton
Copy link
Member

Thanks for the report! Unfortunately though the [features] section doesn't work in Cargo.toml as a virtual manifest (without a [package] section, but we should probably warn about this!

@twistezo
Copy link
Author

@alexcrichton any hope in future about this improvement ?

@alexcrichton alexcrichton added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Jan 16, 2018
@alexcrichton
Copy link
Member

@twistezo I'll tag it as a feature request.

@sanmai-NL
Copy link

sanmai-NL commented Jan 18, 2018

I have a different issue, that fits under this as an ‘umbrella issue’. Given these conditions:

  • The current working directory is the virtual workspace directory.
  • The virtual workspace has a crate member.
  • The crate member defines an executable in [[bin]] in the crate Cargo manifest.
  • That executable requires a feature defined in the crate Cargo manifest.

Then this fails:

cargo run --package crate --bin other_executable --features other_executable_feature
error: target `other_executable` requires the features: `other_executable_feature`
Consider enabling them by passing e.g. `--features="other_executable_feature"`

Whereas this succeeds:

cargo run --package crate --bin other_executable --all-features

As well as this:

cd "${virtual_workspace_directory:?}" &&
cargo run --package crate --bin other_executable --features other_executable_feature

@Michel-Haber
Copy link

Any updates on this?

@matklad
Copy link
Member

matklad commented Jun 26, 2018

@Michel-Haber it's unclear what semantics exactly [features] section in the virtual manifest should have, and what exact problems that should solve. So, the "feature request" part of it is in "needs design" phase. AFAIK, nobody is working on it.

As for "--features flag has unexpected behavior with workspaces", we've tried to fix it in #5364, but that unfortunately broke code in the wild.

@Michel-Haber
Copy link

Michel-Haber commented Jun 29, 2018

@matklad It seems reasonable to me for a workspace to be able to specify its features as re-exports of package features. i.e:

[features]
wsfeature = ["package1/feature1", "package2/feature2"]

A logical link may or may not exist between feature1 and feature2, but this allows for a cleaner selection of features for a set of projects, and, in its simplest form, would simply be a re-export of a package's feature.

This also fixes the unexpected behaviour of --features flag, since it becomes tied to the workspace itself, and not its packages.

brendanzab added a commit to brendanzab/moniker that referenced this issue Jul 10, 2018
This tripped me up and cased me a couple of hours of confusion. Cargo does not seem to pass on features to subcrates in a virtual workspace, nor does it respond to something you’d expect, like `cargo test -features=“moniker/codespan”`. This meant that I actually comitted buggy code to the repository!

I’m working around this for now using `--manifest-path` method - it’s pretty gross though. There is an issue at rust-lang/cargo#4942 recording other people’s experiences with this behaviour.
brendanzab added a commit to brendanzab/moniker that referenced this issue Jul 10, 2018
This tripped me up and caused me a couple of hours of confusion. Cargo does not seem to pass on features to subcrates in a virtual workspace, nor does it respond to something you’d expect, like `cargo test -features=“moniker/codespan”`. This meant that I actually comitted buggy code to the repository!

I’m working around this for now using `--manifest-path` method - it’s pretty gross though. There is an issue at rust-lang/cargo#4942 recording other people’s experiences with this behaviour.
brendanzab added a commit to brendanzab/moniker that referenced this issue Jul 10, 2018
This tripped me up and caused me a couple of hours of confusion. Cargo does not seem to pass on features to subcrates in a virtual workspace, nor does it respond to something you’d expect, like `cargo test -features=“moniker/codespan”`. This meant that I actually comitted buggy code to the repository!

I’m working around this for now using `--manifest-path` method - it’s pretty gross though. There is an issue at rust-lang/cargo#4942 recording other people’s experiences with this behaviour.
@brendanzab
Copy link
Member

brendanzab commented Jul 10, 2018

This meant I wasted a bit of time today, so I linked the PR above. I just went with the --manifest-path workaround for now. That said I would have expected cargo build --features="codespan" to warn, and cargo build --features="moniker/codespan" to 'just work'.

@to-mas-kral
Copy link

Please add a warning, so other poeple know about this, that would've saved me a couple of hours.

@Michael-F-Bryan
Copy link

I also encountered this as part of brendanzab/codespan#31.

At a glance, it seems like @Michel-Haber's solution of having top-level feature flags which can enable specific crate features seems to be the most intuitive. What would be required to get the ball rolling on this?

@alexcrichton
Copy link
Member

There's some more information about this in #5849, notably how we tried to fix this but ended up being unable to because it's technically a breaking change.

@brendanzab
Copy link
Member

brendanzab commented Aug 20, 2018

Is it possible to allow us to 'opt in' to this, by allowing us to explicitly forward features in virtual manifests?

[workspace]
members = [
    "./codespan",
    "./codespan-reporting",
    "./codespan-lsp",
]

[features]
"codespan/memory_usage" = ["codespan/memory_usage"]
"codespan-reporting/memory_usage" = ["codespan-reporting/memory_usage"]

It's not pretty, but it would at least clean up the icky workarounds we have to do when working with workspaced crates, and also allow us to set these features in the RLS config (which is not currently possible). I don't know if this would make future fixes more difficult though.

Would also be super nice to provide a warning when trying to use features without this explicit forwarding as well.

@Michel-Haber
Copy link

@alexcrichton One way of following the solution of top-level feature flags without introducing breaking change would be to actually ignore the feature flags if a features section is not present in the workspace toml file. If one is present but not well defined or used, this is a syntax error.

I rewrote @brendanzab 's proposition to show a more general approach to this:

[workspace]
members = [
"./crate1",
"./crate2",
"./crate3",
]

[features]
"feature1" = ["crate1/feature1", "crate2/feature1", "crate2/feature2"]
"feature2" = ["crate3/feature1"]

This can be used with:
cargo build --features "feature1"

But seeing as silently ignoring feature flags cannot be reasonably considered a feature but rather a bug, one can argue that any breakage, like the one caused by #5390 is in fact due to a bug fix!

@alexcrichton
Copy link
Member

Oh yes indeed! The previous fix may have been too big a hammer and I'd definitely believe it's possible to solve this without breaking crates. I don't personally (nor do I think @matklad) has time to work on this right now, but we can certainly help review PRs to finish implementing this!

@jonhoo
Copy link
Contributor

jonhoo commented Nov 23, 2018

@sanmai-NL I think your issue (and mine) are distinct from this, as it does not include a [feature] specification in the workspace root Cargo.toml. Maybe file a new issue?

@weihanglo
Copy link
Member

weihanglo commented Oct 14, 2023

I slightly object to adding [features] only for acting as a convenient alias in virtual manifest. Some thoughts:

  • Virtual manifests never be published to crates.io. If we ever get feature metadata RFC merged and implemented, what is the semantic of them in virtual manifest? I don't want a [features] section overloading with different meanings, which may exacerbate the already-complicated feature resolution.
  • This can somehow be done with [alias] section in .cargo/config.toml, though [alias] doesn't support variable interpolation so it is not an 100% substitution. We may want to work on that instead.
  • With -Zpackage-features stabilized as resolver v2 in 1.51, the need of forwarding features from virual manifests is resolved.
  • If two member packages have a feature under the same name my-feat, running cargo build --features my-feat enables my-feat for the two packages. That's a new CLI behavior of -Zpackage-features largely meeting the need of supporting [features] in virtual manifest IMO.
  • FWIW, Cargo already have an error rejecting [features] in virtual manifests Warn/err on some unused manifest keys in workspaces. #6276

If there is anything that only a [features] section of virtual manifests can achieve I am not aware of, please speak!

@weihanglo weihanglo added the S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. label Oct 14, 2023
@epage
Copy link
Contributor

epage commented Oct 16, 2023

Based on the above, I actually propose to the cargo team that we close this.

@epage epage added S-propose-close Status: A team member has nominated this for closing, pending further input from the team and removed S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels Oct 16, 2023
@weihanglo
Copy link
Member

Go ahead and close this. Please let us know if there is something wrong.

@weihanglo weihanglo closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features Area: features — conditional compilation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-propose-close Status: A team member has nominated this for closing, pending further input from the team
Projects
None yet
Development

No branches or pull requests