nix-community / crate2nix

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

fix: resolve missing target features to 'null' #307

Closed tilpner closed 11 months ago

tilpner commented 11 months ago

I ran into an error where the evaluation of whether a dependency is used was accessing non-existent target values, and failing:

       at /nix/store/41ra1jaz5pn2rddqbf7j5csj0vn5whl8-services-crate2nix/crate/Cargo-generated.nix:4950:58:

         4949|             packageId = "curve25519-dalek-derive";
         4950|             target = { target, features }: ((!("fiat" == target."curve25519_dalek_backend")) && (!("serial" == target."curve25519_dalek_backend")) && ("x86_64" == target."arch"));
             |                                                          ^
         4951|           }

curve25519-dalek uses --cfg (target) values to switch between different cryptographic backends.

This kind of fallback is already present, but only for checking whether a value is defined, not for comparisons:

https://github.com/nix-community/crate2nix/blob/e5372757075fc93c7624131069fa5d22ed88c4fd/crate2nix/src/render.rs#L216

I chose null only as a value to which no string will compare positively, theoretically this could be any non-string value.

The alternative is probably to continue failing loudly, instead of covering up the lack of a cfg value. If that is preferred, a way of setting them globally/per-crate should be documented (I didn't find it), but it might be tedious if many dependencies use a lot of these variables.

kolloch commented 11 months ago

For a moment there, I was a bit confused by the operator precedence there.

Looks good to me, thank you.

kolloch commented 11 months ago

Feel free to add yourself to the CHANGELOG.md in another PR!

tilpner commented 11 months ago

Thanks for the quick review (and your continued maintenance)! :)

If it were a major improvement, or a breaking change, I'd add it to the changelog, but I'm not sure a minor compatibility fix needs to be in there.