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

crate2nix stumbles over arbitrary #[cfg] conditionals #310

Closed flokli closed 10 months ago

flokli commented 11 months ago

https://github.com/dalek-cryptography/curve25519-dalek/blob/72761ca6b4772af985f969db53faf7accbad9b36/curve25519-dalek/Cargo.toml#L73 seems to use

#[cfg(curve25519_dalek_backend = "fiat")]

and

std::env::var("CARGO_CFG_CURVE25519_DALEK_BACKEND")

in various places in the code.

It looks like crate2nix has some logic to turn these expressions from Cargo.toml into nix expressions, but it seems to look for these keys in a target function argument, and that specific key seems to not exits, causing evaluation of such a Crate.nix to fail:

      "curve25519-dalek" = rec {
        crateName = "curve25519-dalek";
        version = "4.0.0";
        edition = "2021";
        sha256 = "1hibd9z81w9z41v3zf6jxc3qjgbyjk2q23wim588jd6x2ziss4gp";
        authors = [
          "Isis Lovecruft <isis@patternsinthevoid.net>"
          "Henry de Valence <hdevalence@hdevalence.ca>"
        ];
        dependencies = [
          {
            name = "cfg-if";
            packageId = "cfg-if";
          }
          {
            name = "cpufeatures";
            packageId = "cpufeatures";
            target = { target, features }: ("x86_64" == target."arch");
          }
          {
            name = "curve25519-dalek-derive";
            packageId = "curve25519-dalek-derive";
            target = { target, features }: ((!("fiat" == target."curve25519_dalek_backend")) && (!("serial" == target."curve25519_dalek_backend")) && ("x86_64" == target."arch"));
          }
          {
            name = "digest";
            packageId = "digest";
            optional = true;
            usesDefaultFeatures = false;
          }
          {
            name = "fiat-crypto";
            packageId = "fiat-crypto";
            target = { target, features }: ("fiat" == target."curve25519_dalek_backend");
          }
          {
            name = "subtle";
            packageId = "subtle";
            usesDefaultFeatures = false;
          }
          {
            name = "zeroize";
            packageId = "zeroize";
            optional = true;
            usesDefaultFeatures = false;
          }
        ];
        buildDependencies = [
          {
            name = "platforms";
            packageId = "platforms";
          }
          {
            name = "rustc_version";
            packageId = "rustc_version";
          }
        ];
        features = {
          "alloc" = [ "zeroize?/alloc" ];
          "default" = [ "alloc" "precomputed-tables" "zeroize" ];
          "digest" = [ "dep:digest" ];
          "rand_core" = [ "dep:rand_core" ];
          "serde" = [ "dep:serde" ];
          "zeroize" = [ "dep:zeroize" ];
        };
        resolvedDefaultFeatures = [ "alloc" "digest" "precomputed-tables" "zeroize" ];
      };
       error: attribute 'curve25519_dalek_backend' missing

       at /tmp/test/Cargo.nix:1334:58:

         1333|             packageId = "curve25519-dalek-derive";
         1334|             target = { target, features }: ((!("fiat" == target."curve25519_dalek_backend")) && (!("serial" == target."curve25519_dalek_backend")) && ("x86_64" == target."arch"));
             |                                                          ^
         1335|           }
ghost commented 10 months ago

Looks like this was fixed by

https://github.com/dalek-cryptography

Be careful. Performance is not the reason why signing and decryption primitives are written in assembler. Constant-time execution is the reason.

https://github.com/dalek-cryptography/subtle/blob/6b6a81ad9a6a00c0b42c327eaf4b2f785774377e/README.md?plain=1#L13-L15

https://github.com/dalek-cryptography/curve25519-dalek/blob/72761ca6b4772af985f969db53faf7accbad9b36/curve25519-dalek/README.md?plain=1#L215-L216

kolloch commented 10 months ago

@amjoseph-nixpkgs true.

I knew that supporting all target expressions will be tricky, so I thought the approach to avoid errors and be fine if it compiles is a good one. But I was not thinking about security implications...

Probably, this should be opt-in but then again it will break already "working" crates again...

ghost commented 10 months ago

But I was not thinking about security implications...

Oh, my comment about constant time refers to @flokli's reproducer crate, dalek-cryptography.

I was not criticizing crate2nix!

In any event I think this issue can be closed as resolved by #307. I'm able to crate2nix generate && nom build -f Cargo.nix the reproducer crate.

flokli commented 10 months ago

This indeed does work now, thanks.

Any chance for a new release to be tagged? There's quite a bunch of things I cherry-pick since the last release…