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

New behavior of `--feature` + `--package` combination #5364

Open
matklad opened this issue Apr 16, 2018 · 35 comments

Comments

Projects
None yet
@matklad
Copy link
Member

commented Apr 16, 2018

A tracking issue for #5353 and #5351. Historically, --feature and related flags applied to the current package, and not to the package, specified via --package. This is a bug, but fixing it could break someone's code, so currently new behavior exists under -Z package-features opt-in. We need to check if it breaks code in the wild to see what we should do next.

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

#5362 is I think the manifestation of the old odd behavior.

@matklad

This comment has been minimized.

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

I think #4942 is also this issue.

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2018

And #4753 as well

@dwijnand

This comment was marked as resolved.

Copy link
Member

commented Apr 16, 2018

(typo in the issue description: --pacakge)

@matklad matklad changed the title New behavior of `--feature` + `--pacakge` combination New behavior of `--feature` + `--package` combination Apr 16, 2018

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2018

This broke Servo unfortunately, so we probably would need to back this out, at least in this, maximally aggressive, form.

However, I wonder what we plan to do with feature-selection problems in general?

I assume that we want to eventually fix the main problem with features. Currently, features are global per compilation, and that means that features bleed from dev-deps to normal deps and that cargo build -p foo and cargo build -p foo -p bar build the foo package differently.

Fixing that, however, would be a breaking change: suddenly, Cargo would select a smaller set of features :-(

But, if we do fix that, then behavior of the combination of --package and --features would necessary change as well. In other words, this PR is sort of a subset of that larger issue.

The core question I think is do we feel comfortable with eventually changing the set of activated features? If we are, then I think we can modify current fix to not flatly error-out, but to ignore the features argument for several packages, printing a warning like "this was accepted in previous versions of Cargo and activated features in surprising way, and not it is ignored".

@pravic

This comment has been minimized.

Copy link

commented Apr 28, 2018

$ cargo +stable test --all --features xyz

Used to work. And expectation was "Enable the specified set of futures for crates that support it".

$ cargo +nightly test --all --features xyz
error: cannot specify features for more than one package

Now we see this (thanks to CI to catch it soon and not after Rust release) and it does not matter whether all of workspace members have xyz or not, error is the same.

How come that the original issue (cargo --package foo --feature feat) has affected builds without --package? #5390 wasn't a good idea without a compatibility explanation (what is it? how to fix that mystical error? how it will work in future?). That error must have contained at least the issue number (since it is a breaking future and it is in nightly (i.e. unstable) anyway).

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2018

And expectation was "Enable the specified set of futures for crates that support it".

Not that this expectation is wrong. What actually happens is that the feature is enabled only for the package in the current working directory. In other words, it is irrelevant whether all members have the xyz feature, because this applies only to a single member, and not the one that is specified via the -p argument.

That error must have contained at least the issue number

That was definitely an oversight on my part :(

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2018

Here's a small example which demonstrates the difference in behavior: https://github.com/matklad/features-behavior

@matklad

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2018

has affected builds without --package?

--all is a shorthand for --package member1 --package member2 --package member3 etc, and suffers the same problem: --feature flag does not apply to the selected packages. I've extended the example at https://github.com/matklad/features-behavior to show how this affects --all.

bors added a commit that referenced this issue Apr 28, 2018

Auto merge of #5430 - matklad:bring-old-features-back, r=alexcrichton
Revert "Enable new behavior of `--feature`"

This reverts commit 038eec5.

As discussed at #5364, the new behavior unfortunately causes real-life breakage, so we have to revert it.

This is kinda sad, this is a part of the larger issue with feature selection, which, at the moment, has a behavior which I would classify (loosely speaking) as unsound:

* `cargo build -p foo` and `cargo build -p foo -p bar` might produce different artifacts for `foo` ([repro](https://github.com/matklad/workspace-vs-feaures))
* `cargo build -p foo` might produce different artifacts, depending on cwd ([repro](https://github.com/matklad/features-cwd))

The new feature behavior specifically addressed the second point.

It is unclear what we could do with this... One option, instead of flatly erroring out, as the revreted commit does, is to print a warning, but change the set of activated features. It will still be a breaking change, but it at least has  a chance of working by accident.

r? @alexcrichton

bors added a commit that referenced this issue Apr 28, 2018

Auto merge of #5430 - matklad:bring-old-features-back, r=alexcrichton
Revert "Enable new behavior of `--feature`"

This reverts commit 038eec5.

As discussed at #5364, the new behavior unfortunately causes real-life breakage, so we have to revert it.

This is kinda sad, this is a part of the larger issue with feature selection, which, at the moment, has a behavior which I would classify (loosely speaking) as unsound:

* `cargo build -p foo` and `cargo build -p foo -p bar` might produce different artifacts for `foo` ([repro](https://github.com/matklad/workspace-vs-feaures))
* `cargo build -p foo` might produce different artifacts, depending on cwd ([repro](https://github.com/matklad/features-cwd))

The new feature behavior specifically addressed the second point.

It is unclear what we could do with this... One option, instead of flatly erroring out, as the revreted commit does, is to print a warning, but change the set of activated features. It will still be a breaking change, but it at least has  a chance of working by accident.

r? @alexcrichton

bors added a commit that referenced this issue Apr 28, 2018

Auto merge of #5430 - matklad:bring-old-features-back, r=alexcrichton
Revert "Enable new behavior of `--feature`"

This reverts commit 038eec5.

As discussed at #5364, the new behavior unfortunately causes real-life breakage, so we have to revert it.

This is kinda sad, this is a part of the larger issue with feature selection, which, at the moment, has a behavior which I would classify (loosely speaking) as unsound:

* `cargo build -p foo` and `cargo build -p foo -p bar` might produce different artifacts for `foo` ([repro](https://github.com/matklad/workspace-vs-feaures))
* `cargo build -p foo` might produce different artifacts, depending on cwd ([repro](https://github.com/matklad/features-cwd))

The new feature behavior specifically addressed the second point.

It is unclear what we could do with this... One option, instead of flatly erroring out, as the revreted commit does, is to print a warning, but change the set of activated features. It will still be a breaking change, but it at least has  a chance of working by accident.

r? @alexcrichton
@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

Breaking changes to stable functionality are not acceptable. Please revert and find a way to make this opt-in.

However weird and subjectively broken the old behavior is, it has been around for more than long enough that many large projects have built work-arounds that came to rely on it. It’s easy to say this in hindsight, but I think it shouldn’t be surprising that a change like this is definitely gonna break some builds.

#5390, the PR that enable this change by default, says:

So far, the feedback on https://internals.rust-lang.org/t/help-us-test-the-breaking-bug-fix-to-cargo-features/7317 has been positive

At the time of that writing, that forum thread was 2 days old and had responses from three individuals, one of them saying their build is broken. How is this "positive" enough?

But even if that discussion had received community-wide attention, breakage like this without opt-in is still unacceptable IMO. The projects most likely to be affected are those with complex build system that involve many crates. They may or may not be involved in the day-to-day development of Rust and Cargo.


Now if we are going to make a change around feature selection, in addition to being opt-in (possibly through edition = "2018" in the root crate or workspace?) I think we should spent a lot more time and involve more people to consider what the new behavior should be. In particular, fixing #4463. And possibly separating the notions of “Choosing which of the several 'root' artifacts in a workspace (each with its dependency graph and feature selection)” and “Choosing a subset of one dependency graph”.

@gnzlbg

This comment has been minimized.

Copy link

commented Sep 13, 2018

Because of this cargo bug stdsimd has been silently not testing some feature combinations (rust-lang/stdarch#569). That this produces no warnings whatsoever worries me.

@gnzlbg

This comment has been minimized.

Copy link

commented Sep 13, 2018

Breaking changes to stable functionality are not acceptable.

This is incorrect. Breaking changes to stable functionality are acceptable if the functionality is broken, otherwise we couldn't ever fix any bugs if the behavior of the fix could be observed by current crates.

The question that we have to resolve here is whether this functionality is broken or not. If it isn't broken, you are correct in that we cannot fix it. But if it is broken, we can fix it. Depending on the impact of the fix, we might have to warn first for a sufficiently long time before applying the fix, but fixing it is possible and does not require a new edition, although we could use one to implement the fix.

Currently, when I have a workspace with a package foo with a feature bar, executing:

cargo test --features=bar -p foo

compiles foo without the feature bar. I personally think this behavior is broken, and that those wanting this behavior should just write cargo test -p foo without --features=bar.

@dhardy

This comment has been minimized.

Copy link

commented Oct 1, 2018

I don’t think that’s good enough. A change of this importance needs to be opt-in

I don't think the status quo is good enough either; this bug should have been fixed a long time ago. But I agree that the #5353 may not be the ideal solution.

error: cannot specify features for more than one package

--feature flag affects the selected package. It is an error to
specify --feature with more than a single -p, or with -p outside
workspace (the latter could work in theory, but would be hard to
implement).

This is obviously a problem, because people have used expressions like cargo test --all --features foo repeatedly. But that in itself is a problem since the expression doesn't do what many expect.

And expectation was "Enable the specified set of futures for crates that support it".

Not that this expectation is wrong. What actually happens is that ...

We could implement this expectation. On the positive side, it should only break tests if those tests were in fact testing broken code. On the negative side, a typo or forgetting to add a feature to a sub-package could go unnoticed.

But what might be a reasonable compromise would be to enable features specified by --feature for all selected packages, but also add an --allow-missing-feature option which silently ignores features on packages without them.


Summary: a proper fix probably will cause some breakage to existing build systems. But it should correct mis-conceptions and should be very easy for users to fix.

IMHO it is worth suffering some breakage here. Note also that the breakage will mostly only affect continuous integration testing, and not users building/testing packages manually and depending on them transitively.

@dhardy dhardy referenced this issue Oct 1, 2018

Merged

Add rand_pcg crate #4

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

a proper fix probably will cause some breakage […] IMHO it is worth suffering some breakage here.

Breakage and fix v.s. neither are not the only options! We can have a configuration in the workspace’s root Cargo.toml to make the behavior change opt-in.

Note also that the breakage will mostly only affect continuous integration testing, and not users building/testing packages manually and depending on them transitively.

IIRC this was not the case with Servo, we could build at all. I’m not sure why you make this distinction by the way, CI is designed to be as close as possible to other usage.

@gnzlbg

This comment has been minimized.

Copy link

commented Oct 1, 2018

We are at an impasse here.

If we make this change opt-in, we don't break any code in the wild. This is good because it does not break code in the wild that's correctly working as intended. At the same time, this is bad because there is code in the wild that's silently broken, and this change does neither fix it nor alert the maintainers of this code that it needs fixing. If we make this opt-out, we break the broken code, which is good, but we also break the correct code, which is bad.

Honestly, we are here because the current behavior is too implicit, different people expected it to behave differently, and things just silently happened to work for everybody, resulting both in correct and incorrect code.

A compromise solution to this problem might be to just fix the implicitness. We could just add two flags, that enable the different behaviors, and just start warning that all projects should explicitly specify which behavior they want somehow (how exactly is something we can decide later). That will be better than what we currently have, it does not break any code, and it does let people with broken code know that their code might be broken.

Whether we ever turn this warning into an error or not, and how we would do that, is the most controversial part of this issue, and something that we don't really have to discuss right now.

@dhardy

This comment has been minimized.

Copy link

commented Oct 1, 2018

Then I assumed wrong about Servo... why are you using --package with multiple packages for any other reason than testing? Is this easy to change to mitigate the breakage caused by #5353? I tried following the links above but didn't find much information.

We can have a configuration in the workspace’s root Cargo.toml to make the behavior change opt-in.

IMO this is an ugly solution — many users will forget or not know about this, then be surprised when down the road they find thing doesn't work with feature foo despite cargo test --package thing --feature foo being in their CI.

Cargo's configuration and usage is already complex with significant documentation users need to read to understand various features. Adding further configuration options should not be a preferred option IMO.

A compromise solution ... We could just add two flags

At least this doesn't implicitly do the wrong thing for anyone, and if it only shows up when using --feature with --package / --all then it doesn't affect everyone else. Acceptable, but I'd still prefer that this is just "fixed" for everyone (with easy solutions for those affected).

azriel91 added a commit to azriel91/amethyst that referenced this issue Oct 20, 2018

Default to not running renderer tests in `amethyst_test`.
This saves us another `amethyst` compilation. Ideally we can turn off
the `graphics` feature from the workspace root, but this is blocked
pending <rust-lang/cargo#5364>.

Issue amethyst#819

azriel91 added a commit to azriel91/amethyst that referenced this issue Oct 24, 2018

Default to not running renderer tests in `amethyst_test`.
This saves us another `amethyst` compilation. Ideally we can turn off
the `graphics` feature from the workspace root, but this is blocked
pending <rust-lang/cargo#5364>.

Issue amethyst#819

@ehuss ehuss added the A-features label Nov 18, 2018

@arcnmx

This comment has been minimized.

Copy link

commented Dec 24, 2018

Okay it's incredibly frustrating that combining --package with --features is completely broken and does not produce any warnings or errors at all. Anyone assuming that cargo implements the reasonable expected behaviour here will see CI feature tests pass without issue, having no indication that crates are simply being built without the features enabled.

It's frustrating to see that there was a fix made and then suddenly reverted, with no other mitigation or progress since. This seems to be a pretty obvious footgun where most cases seem to be people using --all or -p X and not realising the interactions/implications, with parts of code silently just not being built or tested. It happens to work when a root crate has feature = ["child_crate/feature"]. With no root crate, using --features should probably error to indicate it's broken (#5849) or otherwise just use the fixed behaviour.

Breaking the build in a way unrelated to unstable features is negative impact. I don’t remember the details at this point, but we had found work-arounds for the current behavior’s weirdness, and in applying those work-arounds had come to rely on the current behavior.

Relying on obviously broken and weird behaviour, considering it to be a workaround in build scripts, not using other workarounds that rely on more established reasonable behaviour (like changing CWD rather than using -p/--all), and never expecting it to be fixed seems kind of unreasonable? There are solutions that could be employed to cause less breakage or even be partially or fully compatible with the old behaviour, but it seems unclear from this issue and the surrounding discussion why or how the old behaviour was relied on in the first place.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

@arcnmx breaking changes, whether they're fixing bugs or not, take care when approaching. We can't apply this change and then say "y'all deal with the fallout, it's a change for the better anyway". Our policy is that we would slowly roll out this change, providing warnings and such along the way. For example we would need to:

  • Add a configuration switch to enable the new beahvior
  • Add warnings today when the new behavior and the current behavior differ
  • Push to the ecosystem, evaluate fallout
  • Eventually, when ready, change the defaults.

This takes a lot of work and no one has been sufficiently motivated to drive it to completion.

@sfackler

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Apologies if this has already been covered upthread, but would it be possible to make this change for virtual workspaces specifically? In that case, there is no current package, so it seems less likely to break the world.

@arcnmx

This comment has been minimized.

Copy link

commented Jan 4, 2019

Yeah, my intention is to revive the discussion here and determine what intermediate steps and migration plans are possible, it's just quite frustrating that the proper behaviour is inaccessbily stuck gated behind an unstable/nightly flag. My main points of concern are:

  1. Considering that virtual workspaces are completely broken to my knowledge when combined with feature flags, straight-up enabling the expected/fixed behaviour there would be a good way to address #5849. Otherwise it really should error outright to prevent confusion by the current false assurance that everything is just fine.
    • as long as there's agreement that the new behaviour is correct (considering the original PR was approved, presumably is)
  2. Identifying why the change broke things in the first place, as I'm unable to find explanations of how and why the broken behaviour is currently relied on by existing projects. Most of the outcry doesn't seem to articulate why the revert was necessary, and...
    • whether there's a more conservative or backwards-compatible approach that could be used here?
    • otherwise / irrespective of the above going ahead with such a config/flag warning cycle plan is quite reasonable, with some agreement on a rough approach to follow
@dwijnand

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Perhaps switching the defaults (i.e hard fail rather than just warn) could also have an "opt-back" option (mentioned in the error message) for builds that need a second before migrating. Then we can drop the opt-back option.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Given the fallout we previously experienced I'd be wary of changing this even for virtual workspaces myself. I'd personally prefer that we take a conservative route of "warn if things are different, allow configuration for new, 'correct', behavior"

@ririsoft

This comment has been minimized.

Copy link

commented Jan 14, 2019

Hi, basic developer using rust and getting into this issue here. I agree with the proposal to change the behavior and I understand the concerns regarding stability of the current yet buggy behavior.

Given the fallout we previously experienced I'd be wary of changing this even for virtual workspaces myself. I'd personally prefer that we take a conservative route of "warn if things are different, allow configuration for new, 'correct', behavior"

This proposal would be perfectly fine for me: When doing cargo build -p foo --feature bar at workspace level I should get a warning telling me that I am using a feature at workspace level which is not what I might expect. It should suggest to use a .cargo/config or command line option to opt-in into the natural "fixed" behavior.

Cheers.

@dhardy

This comment has been minimized.

Copy link

commented Jan 14, 2019

Alternative solution: deprecate the --package and --all options. To be clear: I don't consider this a good solution, but if the combination of --feature and --package is not fixable, this is an alternative.

We already have another way of testing sub-packages:

cargo test --manifest-path rand_core/Cargo.toml --no-default-features --features=alloc
@cbeck88

This comment has been minimized.

Copy link

commented Jan 23, 2019

@dhardy Thanks very much for pointing out this effective and non-obvious workaround for this issue!

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Because of #5932 that workaround won't work in all cases, but it should work for any workspace that doesn't use default-members.

@ExpHP

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

Is @dhardy 's workaround able to reuse the workspace target directory, and resolve to the same versions of dependencies as it would when building the root crate?


...to be honest, neither the current nor proposed semantics sound right to me. Here's what I would want --all --no-default-features --features blah,blip to do:

  • Apply all feature options (in this case, --all --no-default-features --features blah,blip) to the root package.
  • Resolve the set of features to be enabled for each subcrate as if we were building the root package.

After all, crates in a workspace already have the responsibility of controlling each others' features (e.g. almost all dependencies on internal or "implementation detail" crates should be using default-features = false). And this way, cargo test --all would build every crate exactly the same way as cargo test would, except it would also build and run test cases for workspace members.

@jethrogb

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@ExpHP we're talking about workspaces here. What do you mean by "root package"?

@ExpHP

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I mean the one whose Cargo.toml contains the workspace table, and whose path dependencies define the workspace members.

For workspaces with no root package that rely on an explicit [members] table, I'm not sure if --all with features even makes sense. I guess the proposed behavior may make sense here (though it feels like duck-typing)... but I also feel that such workspaces would benefit from having a root package to act as an authority on what features mean when cargo is used in the workspace root directory.

@ExpHP

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Clarification: when I say "proposed behavior" I refer to the idea where feature flags are applied to all crates (and ignored on ones that don't have it) for --all. I think I was thinking of another issue and got confused about where I was after following a linkback X_X

@jethrogb

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I'm not sure if --all with features even makes sense.

I think you should be able to specify package/feature as a feature in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.