Disallow specifying features of transitive deps#2821
Conversation
Before this commit, it was possible to activate a feature in a transtive
dependency, using a Cargo.toml like the following one:
...
[features]
# this will enable feature fast in package bar, which is a
# dependency of foo
default = [ foo/bar/fast ]
This is a bug, and was never intended, and it is checked in other places
already. The behavior was possible because `build_features::add_feature`
treats the specification "foo/bar/fast" as just another feature. So when
we require the feature "foo/bar/fast", add_feature for foo will generate a
dependency on "foo" requiring that feature "bar/fast" is enabled. Then,
when resolving foo, add_feature will find that "bar/fast" is a required
feature, so it'll happily add "fast" as the required feature for the
dependency "foo".
The fix for this is to make sure that the `add_feature` function does
not treat `a/b` specifications as just another feature. Instead, it now
handles that case without recursion directly when it encounters it.
We can see how this resolves the above problem: when resolving foo,
add_feature for the required feature "bar/fast" will be called.
Because add_feature no longer treats such specifciations differently at
the top level, it will try to enable a feature with the exact name
"bar/fast", and Context::resolve_features will later find that no such
feature exists for package foo.
To give a friendlier error message, we also check in
Context::resolve_features that we never ever require a feature with a
slash in a name from a dependency.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Disallow specifying features of transitive deps
Before this commit, it was possible to activate a feature in a transtive
dependency, using a Cargo.toml like the following one:
...
[features]
# this will enable feature fast in package bar, which is a
# dependency of foo
default = [ foo/bar/fast ]
This is a bug, and was never intended, and it is checked in other places
already. The behavior was possible because `build_features::add_feature`
treats the specification "foo/bar/fast" as just another feature. So when
we require the feature "foo/bar/fast", add_feature for foo will generate a
dependency on "foo" requiring that feature "bar/fast" is enabled. Then,
when resolving foo, add_feature will find that "bar/fast" is a required
feature, so it'll happily add "fast" as the required feature for the
dependency "foo".
The fix for this is to make sure that the `add_feature` function does
not treat `a/b` specifications as just another feature. Instead, it now
handles that case without recursion directly when it encounters it.
We can see how this resolves the above problem: when resolving foo,
add_feature for the required feature "bar/fast" will be called.
Because add_feature no longer treats such specifciations differently at
the top level, it will try to enable a feature with the exact name
"bar/fast", and Context::resolve_features will later find that no such
feature exists for package foo.
To give a friendlier error message, we also check in
Context::resolve_features that we never ever require a feature with a
slash in a name from a dependency.
|
What is the plan for existing crates that uses transitive features? Are there any on crates.io? |
|
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
|
@sfackler this was already documented against and other situations already gave an error on this, so it's clearly a bug fix of something that wasn't intended to work. A quick grep of crates.io shows no instances of this, but if it comes up we can either (a) recommend how to fix this or (b) consider implementing a warning cycle or such. |
5 participants
Before this commit, it was possible to activate a feature in a transtive
dependency, using a Cargo.toml like the following one:
This is a bug, and was never intended, and it is checked in other places
already. The behavior was possible because
build_features::add_featuretreats the specification "foo/bar/fast" as just another feature. So when
we require the feature "foo/bar/fast", add_feature for foo will generate a
dependency on "foo" requiring that feature "bar/fast" is enabled. Then,
when resolving foo, add_feature will find that "bar/fast" is a required
feature, so it'll happily add "fast" as the required feature for the
dependency "foo".
The fix for this is to make sure that the
add_featurefunction doesnot treat
a/bspecifications as just another feature. Instead, it nowhandles that case without recursion directly when it encounters it.
We can see how this resolves the above problem: when resolving foo,
add_feature for the required feature "bar/fast" will be called.
Because add_feature no longer treats such specifciations differently at
the top level, it will try to enable a feature with the exact name
"bar/fast", and Context::resolve_features will later find that no such
feature exists for package foo.
To give a friendlier error message, we also check in
Context::resolve_features that we never ever require a feature with a
slash in a name from a dependency.