Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable dependencies based on dep: features #252

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

duxovni
Copy link
Contributor

@duxovni duxovni commented Aug 17, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

duxovni commented Aug 21, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants