Add a way not to enable features in workspace crates

[jyn514]

I have a workspace that looks like this, where my-crate: feature1 => x/feature2 means "my-crate has a feature named feature1 which enables feature2 in the dependency x":

bin/my-app: fips => transitive-dep/fips: => boring-sys/fips

When I run cargo hakari generate, it adds a line to workspace-hack/Cargo.toml that looks like

transitive-dep = { git = "...", branch = "...", default-features = false, features = ["fips"] }

I think haraki is running cargo tree --all-features or something like that. I want it not to enable that feature; it only builds on linux machines and some of the people on my team use Mac. I don't see an existing way to do this:

$ cargo hakari generate -h
Generate or update the contents of the workspace-hack crate

USAGE:
    cargo-hakari generate [OPTIONS]

OPTIONS:
        --color <COLOR>    Produce color output [default: auto] [possible values: auto, always,
                           never]
        --diff             Print a diff of contents instead of writing them out. Can be combined
                           with `--quiet`
    -h, --help             Print help information
    -q, --quiet            Suppress output
    -v, --verbose          Produce extra output

Would it be possible to add one?

[sunshowers]

Thanks for the report! hakari has the ability to define exclusions in the config: traversal-excludes and final-excludes.

Another option is to specify your feature as a platform-specific one if possible, e.g. with:

[dependencies]
transitive-dep = "0.1.0"

[target.'cfg(target_os = "linux")'.dependencies]
transitive-dep = { version = "0.1.0", features = ["fips"] }

In that case hakari will automatically exclude it from consideration. If you specify Linux as a platform, it'll then be included for Linux.

[jyn514]

Another option is to specify your feature as a platform-specific one if possible, e.g. with:

That's clever! I didn't realize cargo would unify the features across target-specific tables like that :)

hakari has the ability to define exclusions in the config: traversal-excludes and final-excludes.

hmm, I tried this but got an error I didn't understand:

Error: 
   0: error resolving Hakari config at /home/jnelson/work/redacted/.config/hakari.toml
   1: unknown elements: resolving hakari final-excludes
   1: * unknown third-party:
   1:   - { name = "transitive-dep", version = "*", source = { git = "..." } }

I am not sure it would solve my problem even if I got it working, though - I do want to pull in transitive-dep, just not enable its features unconditionally.

[sunshowers]

Could you paste the contents of the relevant sections of .config/hakari.toml?

I do want to pull in transitive-dep, just not enable its features unconditionally.

You'll be able to pull in transitive-dep while building, it just won't be part of the workspace-hack.

[jyn514]

[final-excludes]
third-party = [
    { name = "transitive-dep", git = "ssh://git@redacted/transitive-dep.git" }
]

[jyn514]

If I try transitive-excludes instead of final-excludes, I get info: no changes detected

[sunshowers]

Ahh I did some experimentation and it looks like you have to specify both the URL and the branch:

[final-excludes]
third-party = [
    { name = "transitive-dep", git = "ssh://git@redacted/transitive-dep.git", branch = "my-branch" }
]

I think this is a bug (not specifying a branch should mean "include all branches") but hopefully this workaround will work for you.

If I try transitive-excludes instead of final-excludes, I get info: no changes detected

Note that it's traversal-excludes -- we should add a warning for an unrecognized key. Thanks for trying it out!

[jyn514]

I think this is a bug (not specifying a branch should mean "include all branches") but hopefully this workaround will work for you.

Thanks! With traversal-excludes that worked, but too well - it's ignoring all features in transitive-dep, but I only want it to ignore the fips feature. So I end up with many crates being recompiled when it should only be boring-sys and crates that depend on it.

I think final-excludes might work, but it requires me to list every transitive dep that has a fips feature, which is a lot of maintenance (transitive-dep is itself in a workspace with a ton of crates).

It would be nice to have a way to configure excludes individual features, instead of whole crates. Alternatively, if there's a way to tell hakari not to enable features defined by crates in the current workspace, I think that would work as well.

[sunshowers]

Yeah, I think that would be a pretty good addition! This is honestly the first time excluding just a single feature (rather than an entire package) has come up like this.

Question: does the platform-specific feature via the [target.'cfg()'] table not work for you?

[jyn514]

Yeah, I think that would be a pretty good addition! This is honestly the first time excluding just a single feature (rather than an entire package) has come up like this.

Awesome, glad to hear! I'm happy to take a stab at implementing it if you can point me in the right direction :)

Question: does the platform-specific feature via the [target.'cfg()'] table not work for you?

No, unfortunately - I do want to build this workspace in multiple modes, even on Linux. Enabling the fips feature requires additional dependencies and setup that are annoying to get working locally, so we normally only enable it in CI.

[sunshowers]

Awesome, glad to hear! I'm happy to take a stab at implementing it if you can point me in the right direction :)

A little warning: it's maybe a full day of work.

Configuration

We probably want to expose this via an optional features key in the excludes config. For example:

[traversal-excludes]
workspace-members = [
    "my-crate",
    { name = "my-other-crate", features = ["foo", "bar"] }
]
third-party = [
    { package = "foo", version = "1", features = ["feature1", "feature2"] }
]

This is likely going to mean moving away from simply deserializing the PackageSetSummary as is done today, to defining our own custom data structure that can be deserialized. This bit shouldn't be too hard. Easiest way would likely be to add another field to PackageSetSummary with #[serde(flatten)]. and put extra contents in there. Ideally the same missing check would carry over as well -- if a package is found but it did not contain a feature that was specified, error out.

Updating the core algorithms

For traversal-excludes, information about omitted packages is plumbed through to guppy's Cargo build simulations:

https://github.com/facebookincubator/cargo-guppy/blob/88681d7d78e148d3fbd8205acbd3e13f9786cab0/guppy/src/graph/cargo/build.rs#L85-L87

This needs to be updated to handle excluding individual features as well.

final-excludes is simpler (though as you said it won't solve your problem). This method needs to be updated to also handle individual features:

https://github.com/facebookincubator/cargo-guppy/blob/88681d7d78e148d3fbd8205acbd3e13f9786cab0/tools/hakari/src/hakari.rs#L1376

Note that a package with no features left should be removed from the map, like in the example shown here:

https://github.com/facebookincubator/cargo-guppy/blob/88681d7d78e148d3fbd8205acbd3e13f9786cab0/tools/hakari/src/hakari.rs#L1411-L1412

[jyn514]

Awesome! I have ~2 hours today to put into it, I'll take a stab and see what progress I can make :)

[jyn514]

We probably want to expose this via an optional features key in the excludes config. For example:

[traversal-excludes]
workspace-members = [
   "my-crate",
   { name = "my-other-crate", features = ["foo", "bar"] }
]

What should { name = "my-other-crate", features = [] } mean? "Ignore all features", same as "my-other-crate" on its own? Or "ignore no features", i.e. a no-op?

(alternatively I can just make this a hard error and require people to specify at least one feature)

[sunshowers]

What should { name = "my-other-crate", features = [] } mean? "Ignore all features", same as "my-other-crate" on its own? Or "ignore no features", i.e. a no-op?

I'd make this a hard error.

[jyn514]

@sunshowers I'm sure there's a simple way to do this, but is there a way to tell serde "deserialize this struct, then another struct" without having to manually implement Visitor? I currently have

impl<'de> Deserialize<'de> for ThirdPartySummaryWithFeatures {
    fn deserialize<D>(d: D) -> Result<Self, D::Error>
        where
            D: serde::Deserializer<'de> {
        let FeatureFields { features } = FeatureFields::deserialize(d)?;
        let krate = ThirdPartySummary::deserialize(d)?;

        Ok(ThirdPartySummaryWithFeatures { krate, features: require_non_empty(features)? })
    }
}

which gives an error that I use d in ThirdPartySummary::deserialize after moving it in FeatureFields::deserialize.

(I'm making these separate structs because ThirdPartySummary seems quite complicated and I don't want to rewrite the deserialization logic.)

[jyn514]

actually I think this is just serde(flatten) - sorry for the ping

[sunshowers]

Yeah, I think that's serde(flatten).

[jyn514]

This needs to be updated to handle excluding individual features as well.

You're right, this is trickier :grin: I've plumbed down the individual features to CargoSetBuildState (it's very hacky for now, but should be enough to test whether the logic works), but I'm not actually sure how to filter out features based on the crate -> feature mapping. I see that the core of the logic lives around CargoSetBuildState::build_set, but there's a lot to account for:

1. Are they enabled by a feature with their name?
2. Are they enabled by another feature for the package?
3. Are they excluded by an omitted feature, but enabled by some other feature?

The same also applies to optional dependencies, although hopefully :crossed_fingers: that will use the same logic.

1. is easy. I am not sure how to do 2. or 3. I found to_feature_set(filter: FeatureFilter), ~~but that seems to act on the whole graph, not on links between crates.~~ ah no I'm wrong - FeatureId gives me both the package and feature name. Ok, this should be doable, just complicated.

~~I was going to say doing this per-crate seems overcomplicated, but then I realized that features may have the same name yet be unrelated, e.g. a serde feature.~~

[sunshowers]

Where the is_omitted call happens you could also filter by feature index rather than just the package index as happens today.

[sunshowers]

Feel free to put up whatever you have, I'm happy to finish it up in the next week or so

[jyn514]

Feel free to put up whatever you have, I'm happy to finish it up in the next week or so

I am not sure you want it as-is, it's incomplete and doesn't compile :sweat_smile: but here's what I have so far: https://github.com/facebookincubator/cargo-guppy/compare/main...jyn514:specific-features?expand=1 I was also going to test it on two crates with the same feature name, to make sure only the crate you wrote was used - mint and rgb both have a serde feature you can use to test.

Thanks so much for all your help! This was a lot of fun :D

[jyn514]

Oh, also I think the commented-out code around PackageSetSummaryWithFeatures::to_package_set_registry was a bad idea, since it requires making lots of things from guppy public - I was planning to delete that and convert the PackageSetSummaryWithFeatures to PackageSetSummary temporarily, then call add_omitted_features in the appropriate places in tools/hakari/src/hakari.rs.

[sunshowers]

Hi -- I've left Meta and can no longer contribute to this repository. Please file an issue at guppy-rs/guppy. Thanks.