-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Copy all features from the features key in the Cargo.toml. #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I only skimmed the crate layout you posted in #145, but could you shed some light on why this wouldn't be considered a Cargo bug? If the generated Cargo.toml contains features foo = ["dependency_a/foo"]
, and dependency_a contains features foo = ["dependency_b/foo"]
, then trybuild executing cargo test --features foo
on the generated manifest should be resulting in dependency_b/foo
enabled. Have you looked into why or is there an easy way to understand why that isn't happening?
If it's a Cargo bug then I would prefer not to land this workaround and would ask you to follow up in the Cargo repo.
Digging into it I hadn't considered it could be a cargo bug. Your assessment of the layout is correct. I'll report it in cargo, if it's intended behavior I'll reopen the PR. |
I reported it as a This means that there in fact is a need for a workaround, but I am no longer sure that my simple PR is enough. As I understand it:
So I propose a couple of changes:
I would love to hear your thoughts on the matter :) |
Always propagating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the package.resolver
change in #149. Reviewing the repro in #145, I think this is otherwise behaving correctly as described in #145 (comment), so I'll close the PR. Thanks!
Thanks for the very fast resolution! |
Hey
This is my understanding of what is needed to fix issue #145
I've changed the way features are read from the source manifest.
Instead of only copying the single feature enable e.g.
crate_name/feature_name
, the code now adds all values from the given feature key in the source manifest in addition tocrate_name/feature_name
.To take the linked issue as an example, the current version creates a
Cargo.toml
features section:With this addition the test compiles as expected. The
Cargo.toml
features section now reads: