Skip to content

Support multiple cargo_metadata feature options#83

Merged
bors[bot] merged 1 commit intorust-embedded:masterfrom
mciantyre:cargo-metadata-features
Jul 28, 2020
Merged

Support multiple cargo_metadata feature options#83
bors[bot] merged 1 commit intorust-embedded:masterfrom
mciantyre:cargo-metadata-features

Conversation

@mciantyre
Copy link
Copy Markdown
Contributor

The PR follows #78, which did not correct cargo_metadata feature exclusivity. oli-obk/cargo_metadata#118 lets us combine feature options, like --no-default-features --features foo, so we can fix our usage here. As of this PR, the return of cargo_metadata will truly reflect whatever feature options the user passes to cargo-binutils.

The cargo_metadata change was published in 0.10.1, but this PR bumps to 0.11, which is backwards compatible with 0.10.

This tracks a patch in cargo-metadata that lets us call features()
more than once to express more complex feature configurations.
We introduced the change in cargo-metadata 0.10.1, but this commit
jumps us to 0.11.
@mciantyre mciantyre requested a review from a team as a code owner July 27, 2020 00:20
@rust-highfive
Copy link
Copy Markdown

r? @therealprof

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

@therealprof
Copy link
Copy Markdown
Contributor

Looks good to me in general. I do wonder whether features and all-features should be mutually exclusive though? Or are they not because all-features only concern the crate in hand while features allows setting any features in the global feature-set?

@mciantyre
Copy link
Copy Markdown
Contributor Author

The behaviors of cargo build and cargo metadata are that --all-features invalidates all other --features usage, no matter if --features activates the crate at hand's feature, or a feature from the global set. --all-features also invalidates --no-default-features. The behavior seems to follow

// 1
if matches.is_present("all-features") {
    metadata_command.features(CargoOpt::AllFeatures);
} else {
    if matches.is_present("no-default-features") {
        metadata_command.features(CargoOpt::NoDefaultFeatures);
    }
    if let Some(features) = matches.values_of("features") {
        metadata_command.features(CargoOpt::SomeFeatures(
            features.map(|s| s.to_owned()).collect(),
        ));
    }
}

which means that --all-features is mutually exclusive from the other two options.

But, it's nonetheless valid for me to pass in all three feature options to build, metadata, or a binutil tool:

# 2
cargo metadata --no-default-features --features foo --features my-dep/dep-feat --all-features

I suppose it doesn't matter if the checks in this crate are mutually exclusive, so long as they represent (1). I can update the PR to (1) if you'd like.

As of this writing, the PR assumes that the cargo_metadata crate behaves like the cargo metadata tool, accepting the variety of feature options shown in (2). Snippet (1) might distrust cargo_metadata, implying that we know better about how to combine feature options. I'm leaning towards mimicking what the cargo metadata reference implementation supports, and what we expect cargo_metadata to also support.

Copy link
Copy Markdown
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. LGTM then.

bors r+

@bors
Copy link
Copy Markdown
Contributor

bors bot commented Jul 28, 2020

Build succeeded:

@bors bors bot merged commit d15cdbf into rust-embedded:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants