search

nix-community / crate2nix

rebuild only changed crates in CI with crate2nix and nix
https://nix-community.github.io/crate2nix/
Apache License 2.0
354 stars 83 forks source link

Enable dependencies based on dep: features #252

Closed duxovni closed 2 years ago

duxovni commented 2 years ago

Now that Rust 1.60 includes the explicit dep: syntax for dependency features, we need to look for those as well when figuring out which dependencies are enabled. This fixes the broken build of serde_with 2.0.0.

Ericson2314 commented 2 years ago

It it looks like right now you have features and deps living in the "same namespace"? I am not sure that is a good.

Can you make it separate?

Similarly, can you make the test have a dependency and feature that are completely unrelated but share the same name?

duxovni commented 2 years ago

What do you mean by "same namespace"? Are you talking about dep: features being a type of feature even though they represent dependencies?

And yes, as written, if we do use dep: features and also have an unrelated feature with the same name as the dependency, enabling that feature will lead to crate2nix including that dependency. It seems like a weird edge case that there's no good reason anyone would ever do, but I guess I could perform a much larger overhaul to fix that; my main focus here was just erring on the side of including more dependencies to prevent build errors.

Ericson2314 commented 2 years ago

It is more, work, but I don't think it will be too hard.

The entire reason they added dep: upstream was to fix this ambiguity, so supporting dep: without supporting the motivation feature for why it exists doesn't seem good to me.

Ericson2314 commented 2 years ago

Heh, I need this now, so I'll merge after all, but making a issue to fix the semantics.