Emit Resolve.features_sorted in "cargo metadata"

[acmcarther]

This PR adds features to metadata.resolve.nodes[*] using the Resolve object's known features. This is different from the features fields that already exist elsewhere within metadata as this one is the actual features that need to be used in order to build the code.

Context: I'm currently using Cargo's internals to synthesize BUILD files for the Bazel build tool. cargo metadata currently provides almost everything I need in order to avoid using Cargo's internals -- except for this.

cc google/cargo-raze#7

[rust-highfive]

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.

[matklad]

LGTM, but let's see what @alexcrichton things here!

[alexcrichton]

Thanks for the PR @acmcarther! Unfortunately though this may not actually be the best way to do this in the sense that it's exposing an existing bug in Cargo, the fact that Cargo knows feature selection at this point!

To expand on that, it's actually a bug in Cargo that Cargo thinks it knows what features are applied just after crate graph resolution. Instead Cargo should only know feature selection once it actually goes to compile crates which ends up dealing with target-specific dependencies and such. The primary instances of this bug are:

• Incorrect dev-dependency feature resolution #1796
• Build Deps getting mixed in with dependencies #2589
• Shared build+target dependency crates conflate features #4361

I'm wary though to block integration of this based on existing bugs in Cargo which are otherwise pretty difficult to fix. It would of course always be relatively easy for Cargo to continue to support this format (even if it didn't internally have such easy access one day) so it may be worth landing this anyway.

I'm curiuos, @matklad do you have thoughts on this aspect?

[matklad]

@alexcrichton Am I correct that long-term, we'd love to split resolve into two phases:

1. in the first phase, we resolve dependencies between the packages, not looking at the features at all (i.e, pulling in all optional dependencies and such).

2. in the second phase, which is specific to a compilation of particular target and particular command-line flags, we prune the DAG from the first phase, and do feature selection based on features, requested in the manifest (of a particular workspace package / target), supplied on the command line and conditional on the target tripple.

In particular, is it true that dealing with --features command-line flag belongs to the second phase, and not to the first? If it is the case, I'd say that cargo metadata is already affected by this bug, because this command accepts the --features, --all-features and --no-default-features flags.

So, I'd r+ this PR, because it doesn't make the situation significantly worse :-)

When we fix the Cargo bug, I think we can backwards compatibly evolve cargo metadata in such a way that it can supply both "the general resolve" as well as "resolve, specific to a particular combination of package, target, requested features and target tripple".

[acmcarther]

@alexcrichton

Unfortunately though this may not actually be the best way to do this in the sense that it's exposing an existing bug in Cargo, the fact that Cargo knows feature selection at this point!

I see. Yes I was at some level aware that neither dependencies nor features as yielded by the resolve are usable directly. This explains some design decisions that I made in the past and since forgot about regarding pruning dependencies after processing the Resolve (using the platform triple / platform attrs). I couldn't remember why I ever had to do such a thing in the first place. Apparently I needed to prune features as well but didn't realize it and luckily nothing broke.

@matklad

(two step resolve description)

I'd love for this to work in this manner -- though I'd hope that I could have cargo metadata yield data from both passes (i.e. give me platform specific deps and features). Sounds like you considered it as well below.

When we fix the Cargo bug, I think we can backwards compatibly evolve cargo metadata in such a way that it can supply both "the general resolve" as well as "resolve, specific to a particular combination of package, target, requested features and target tripple".

I was actually confused that cargo metadata didn't take the platform triple in the first place. 👍 on the change.

[alexcrichton]

@matklad oh I don't think we'll ever be able to purge features from resolve because features can activate optional dependencies which need to get into the crate graph. That being said I do think the command line flags may also be a bug in the sense that the lock file (sort of what you get from cargo metadata) is created without looking at those flags at all.

In general though I think I'd agree in that we can probably get by with just adding this for now and dealing with it later (unfortunately). It seems prudent to add a --target to cargo metadata though! Although at some point it's for sure that cargo metadata won't be suitable for builds due to incorrect feature/target selection, but presumably we can cover that later down the road.

@acmcarther ok you and @matklad have convinced me this should be fine to add for now, and we can always tweak it later likely in a backwards-compatible fashion if necessary.

@bors: r+

[bors]

📌 Commit 6a2b646 has been approved by alexcrichton

[bors]

⌛ Testing commit 6a2b646 with merge e514f98...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e514f98 to master...