WIP: Features vs packages

[matklad]

This is intended to fix just one weird thing about feature resolution in Cargo. However, fixing it could break someone's spacebar heater, so I've decided to submit a PR early to make sure we indeed want this fix :-)

So, the problematic piece of logic is here:

cargo/src/cargo/ops/resolve.rs

Lines 247 to 255 in 4b1c1b7

	match ws.current_opt() {
	Some(current) if member_id == current.package_id() => method,
	_ => {
	if specs.iter().any(|spec| spec.matches(member_id)) {
	base
	} else {
	continue;
	}
	}

The cwd of Cargo affects the resolution logic, and that seems just plain wrong. It is a known issue that cargo build -p foo and cargo build -p foo -p bar could give different results for foo because of feature selection. However, just cargo bulid -p foo can give different results depending on Cargo's cwd, because the current package always gets activated implicitly! Another related piece of weirdness is that cargo build -p foo --features bar does not activate the bar feature of foo. It activates the feature of the package at the cwd.

This PR removes special casing of cwd, and, as a result:

• cargo build -p foo --feature bar does what it looks like it does.
• cargo test -p foo -p bar --feature baz no longer works. It is an error to specify --feature for more than one package.
• cargo build -p not-a-member is just weird. Because it is not a member, we need to resolve some member to get to it, and this is truly ambiguous. Previously, we would have looked at cwd package. Now we resolve the whole workspace instead.

@alexcrichton do you feel comfortable with these changes in behavior of features/workspaces combination?

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[djc]

Just a drive-by review: your goals mostly sound good; I definitely agree that the CWD should not affect what gets build if the command-line arguments are explicit.

However, I would suggest that cargo build -p not-a-member should just error out. It feels like cargo has a number of places where it goes to far in trying to guess what you want, which leads to either surprising results or makes it harder to provide good error messages.

The addition of RequestedPackages by itself does not look like an improvement design-wise (mostly just because it adds more arguments in a bunch of places rather than replacing something else), but that might be solve by some further refactoring down the line.

[matklad]

However, I would suggest that cargo build -p not-a-member should just error out.

I am afraid we can't do that :( cargo build --package foo worked since before workspaces were implemented (where it would build a dependency of current package, which was unambiguous at that time), so we probably need to stay backwards compatible here. And, in theory, cargo build -p not-a-member could work 100% intuitively, when we separate feature resolution process from dependency resolution process.

The addition of RequestedPackages by itself does not look like an improvement design-wise (mostly just because it adds more arguments in a bunch of places rather than replacing something else)

Yeah, I just find it impossible to refactor things by replacing them: I usually try to build a new better thing alongside the old one, keeping the code compiling and the tests green, and then remove the old thing separately (well, actually I am not that wise yet, so I typically start replacing the thing, go half-way through to an incomprehensible mess with thousands of compiler errors, and then start afresh with "first add, then remove" strategy :-) ). This PR is an intermediate state, when both new and old thing coexist. I intend to remove Packages structure, specks argument, various feature-related flags from CompileOptions and Method::Everything variant as well :-) Hopefully, it'll be a net reduction in lines of code :-)

[matklad]

Another interesting finding: cargo metadata accepts --features, and it makes exactly zero sense, because metadata is always workspace-wide...

[djc]

Oh, that looks quite a bit nicer already!

[alexcrichton]

Nice! I'm definitely on board with this idea.

I think we won't be able to turn -p foo -p bar --features baz into a hard error immediately though. I suspect that it's used as a workaround in a few locations where it otherwise wouldn't be possible to invoke. Could we start out with a warning and a tracking issue perhaps? That way if people hit it all the tiem we can figure out a way to not have a warning and otherwise we can delete it eventually.

[alexcrichton]

I'm having a tough time proving to myself that this loop is equivalent to what was there before. I think the idea is that if cli flags affect method they're forbidden earlier, right?

That is, if method has features: &["some_feature"], then we're guaranteed there's only one member here at this point, right? If so, can that be asserted here?

[matklad]

this loop is equivalent to what was there before

It is not exactly equivalent, --all-features and --no-default-features would now apply to all members of the workspace, and not just the current one. This also technically is a breaking change, and it also seems much more intuitive than the previous behavior. The reasoning about features: &["some_feature"] is correct, I think.

[alexcrichton]

Hm ok so something liek cargo build --all --all-features is now changing meaning? (just to make sure I understand)

[matklad]

Hm ok so something liek cargo build --all --all-features is now changing meaning?

Yep!

And I think even cargo build -p foo can change meaning, because we no longer activate cwd package unconditionally.

[alexcrichton]

I'm personally not a huge fan of doing this sort of verification in argument parsing, could these move into the main library?

Cargo-as-a-library I think would be easier to use if we didn't shift too much of the validation to cli argument parsing (which may cause panics/weird bugs if this data gets into cargo-as-a-library). Additionally I feel like argument parsing is primarily concerned with sort of syntactically what's passed in rather than interpreting it which happens in later layers. (also helps us keep everything centralized as much as we can)

[matklad]

I think we won't be able to turn -p foo -p bar --features baz into a hard error immediately though. I suspect that it's used as a workaround in a few locations where it otherwise wouldn't be possible to invoke. Could we start out with a warning and a tracking issue perhaps? That way if people hit it all the tiem we can figure out a way to not have a warning and otherwise we can delete it eventually.

So, what exactly do we want to preserve here? If we want to preserve the semantics exactly, that would mean that cargo build -p foo --features baz would remain highly surprising.

We can probably switch behavior when there's only one -p argument, and use the old behavior with a warning when there are more than one -p.

The third option is to just ignore --feature flags for multiple ps and give a warning instead (i.e, not outright error out and break the build, but select different features).

All in all, I think we are in a tough situation: current behavior seems to be highly surprising and wrong, but there's no clear path for opt-in for a new behavior, short for a completely new command line flags :(

[alexcrichton]

Ah yes I see, good points! It was more cut-and-dry in my head where we only had to worry about two -p arguments when combined with --features (a hard error after this PR but we could warn instead for a bit). It's true that -p foo --features bar really only can change immediately if we do anything at all.

Perhaps we could try this out and see what happens in that case? It's true that the behavior is surprising and probably wrong, but that wouldn't stop people from relying on it. It may be best to:

• First, advertise this change on internals
• Next, assuming no one says they're actively relying on it and are unwilling to change, land this PR
• Get it into nightlies
• Watch for breakage and likely revert if something comes up

[alexcrichton]

@matklad hm so how about this. What if we implement all these new semantics behind a new -Z foo flag? That way we can get this on nightly and ask users to "please pass -Z foo and see if it breaks for you". If everything goes well then we can remove the -Z flag and switch the defaults.

[matklad]

@alexcrichton sounds reasonable! I think I can implement this without disruptive refactoring. I still would want to clean up the code here a lot, but figuring out the actual semantics we want is for sure much more important!

[matklad]

Closing for now in favor of #5353