cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken

[ghost]

Recent changes to cargo-auditable-cargo-wrapper and librsvg caused it to ignore the user's decision to opt out of cargo-audit functionality, partially because librsvg does not use buildRustPackage.

This commit restores the single-point-of-opt-out from this mis-named functionality: cargo-audit.meta.broken.

[figsoda]

cargo's auditable option is only about whether cargo itself is auditable, not whether it produces auditable binaries. The option is exposed to boostrap cargo-auditable.

[ghost]

cargo's auditable option is only about whether cargo itself is auditable

So where is the master switch to disable "audit" everywhere, including packages like librsvg that don't use buildRustPackage?

[DieracDelta]

This change appears to break overlays that override cargo (such as fenix).

[figsoda]

cargo's auditable option is only about whether cargo itself is auditable

So where is the master switch to disable "audit" everywhere, including packages like librsvg that don't use buildRustPackage?

You can overlay cargo-auditable-cargo-wrapper to just cargo, and maybe also buildRustPackage

[ghost]

This change appears to break overlays that override cargo (such as fenix).

Yeah, well, cargo-auditable-cargo-wrapper broke overlays that disable auditable in non-buildRustPackage packages (like librsvg), which I think is more obnoxious. Also, this doesn't break overlays that pass through the auditable flag; I'd say that's now part of what you need to do in order to override cargo.

You can overlay cargo-auditable-cargo-wrapper to just cargo, and maybe also buildRustPackage

Until recently you only had to do one of those two things.

In the future will people have to do three things?

There needs to be one single place where users can opt out of this, instead of having to constantly track the development of a feature simply to not use it.

I welcome suggestions on alternative locations for that single point-of-opt-out.

[figsoda]

Can I ask why you would need to globally turn off cargo-auditable?

[ghost]

Can I ask why you would need to globally turn of cargo-auditable?

Because I don't want to use it.

Also, like so many things related to cargo, it duplicates functionality that nix already provides. And does so quite poorly.

[figsoda]

Are there drawback to using cargo-auditable?

[ghost]

Are there drawback to using cargo-auditable?

Please don't derail the discussion.

[figsoda]

Are there drawback to using cargo-auditable?

Please don't derail the discussion.

I'm just asking because I would prefer to fix the issue you are experiencing instead of having to create a workaround, if that's possible.

[ghost]

I'm just asking because I would prefer to fix the issue you are experiencing

The issue I am experiencing is that I do not wish to use cargo-audit, yet it keeps re-enabling itself when I rebase my development branch.

This PR fixes the issue I am experiencing. You can fix the issue by merging it.

[figsoda]

I would appreciate it if you weren't being passive-aggressive in the replies.

Anyway, I don't think we should introduce a passthru for this, instead we can just mark cargo-auditable as broken in the overlay.

With this patch

--- a/pkgs/development/compilers/rust/1_71.nix
+++ b/pkgs/development/compilers/rust/1_71.nix
@@ -52,7 +52,7 @@ import ./default.nix {
     mips64el-unknown-linux-gnuabi64 = "de5fd0b249fbb95b9b67928ba08d7ec49f18f0ae25cbe1b0ede3c02390d7b93a";
   };
 
-  selectRustPackage = pkgs: pkgs.rust_1_71;
+  selectRustPackage = pkgs: pkgs.rust;
 
   rustcPatches = [ ];
 }
--- a/pkgs/development/compilers/rust/cargo-auditable-cargo-wrapper.nix
+++ b/pkgs/development/compilers/rust/cargo-auditable-cargo-wrapper.nix
@@ -1,13 +1,16 @@
 { lib, runCommand, makeBinaryWrapper, cargo, cargo-auditable }:
 
-runCommand "auditable-${cargo.name}" {
-  nativeBuildInputs = [ makeBinaryWrapper ];
-  meta = cargo-auditable.meta // {
-    mainProgram = "cargo";
-  };
-} ''
-  mkdir -p $out/bin
-  makeWrapper ${cargo}/bin/cargo $out/bin/cargo \
-    --set CARGO_AUDITABLE_IGNORE_UNSUPPORTED 1 \
-    --prefix PATH : ${lib.makeBinPath [ cargo cargo-auditable ]}
-''
+if cargo-auditable.meta.broken then
+  cargo
+else
+  runCommand "auditable-${cargo.name}" {
+    nativeBuildInputs = [ makeBinaryWrapper ];
+    meta = cargo-auditable.meta // {
+      mainProgram = "cargo";
+    };
+  } ''
+    mkdir -p $out/bin
+    makeWrapper ${cargo}/bin/cargo $out/bin/cargo \
+      --set CARGO_AUDITABLE_IGNORE_UNSUPPORTED 1 \
+      --prefix PATH : ${lib.makeBinPath [ cargo cargo-auditable ]}
+  ''

The following overlay works for me

final: prev: {
  rustPackages = prev.rustPackages.overrideScope (_: prevRust: {
    cargo-auditable = prevRust.cargo-auditable.overrideAttrs (old: {
      meta = old.meta // {
        broken = true;
      };
    });
  });
  rust = prev.rust // {
    packages = prev.rust.packages // {
      stable = final.rustPackages;
    };
  };
}

It is a little more code than I would like, but one could possibly deduplicate the top-level rust attributes to simplify the overlay. Though that would probably go into a separate PR.

[figsoda]

For the commit message, I don't think cargo-auditable: respect user's choices is descriptive enough. For my suggested solution, it should probably be something like cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is broken.

And for the rest of the commit message, it should be cargo-auditable instead of cargo-audit, and I assume you mean buildRustPackage instead of buildRustCrate?

[ghost]

I would appreciate it if you weren't being passive-aggressive in the replies.

I don't see anything aggressive about my replies.

Nixpkgs is policy-free and I want to keep it that way. My own policy choices are not relevant to that goal. Every time somebody tries to impose their own policy choices on nixpkgs, the debate takes a predictable path: they try to steer the conversation towards my policy choices, and explain why their policy choices are better. It's tedious and distracting, so I refuse to be drawn into it. Aggression has nothing to do with it.

Anyway, I don't think we should introduce a passthru for this, instead we can just mark cargo-auditable as broken

Yes, I've been doing that, which is how I discovered that cargo-auditable-cargo-wrapper ignores cargo-auditable.meta.broken.

With this patch

(which causes cargo-auditable-cargo-wrapper to check meta.broken)

The following overlay works for me

I'm not excited about further-overloading meta.broken (which is already used for too many things), but it's a compromise I'm okay with if it gets you on board.

I assume you mean buildRustPackage instead of buildRustCrate?

Yes, thanks for noticing that.

[ghost]

Rebuilding to verify this; will undraftify when built.

[tjni]

I do think we need better ways to control global policy decisions, but I don't have a design in mind. This change is great, it's the right thing to do to fall back to cargo in situations when cargo-auditable is broken, and if it works to solve the policy problem (with a little bit creativity and shielding your eyes), then even better.

[RaitoBezarius]

Are there drawback to using cargo-auditable?

I would say that drawbacks are listed here: rust-lang/rfcs#2801 — either case, I find cargo-auditable to be extremely cheap and would pursue enabling it by default on a large scale beyond Nixpkgs, obviously, I also agree that it's important to let anyone override this default and opt out as they deem it fit, including, beyond Nixpkgs.

It will make things easier when this feature lands upstream hopefully.

[figsoda]

I wasn't able to make it work without this patch:

--- a/pkgs/development/compilers/rust/1_71.nix
+++ b/pkgs/development/compilers/rust/1_71.nix
@@ -52,7 +52,7 @@ import ./default.nix {
     mips64el-unknown-linux-gnuabi64 = "de5fd0b249fbb95b9b67928ba08d7ec49f18f0ae25cbe1b0ede3c02390d7b93a";
   };
 
-  selectRustPackage = pkgs: pkgs.rust_1_71;
+  selectRustPackage = pkgs: pkgs.rust;
 
   rustcPatches = [ ];
 }

but lgtm if you managed to make this work

one nitpick in the commit message: cargo-audit -> cargo-auditable, though it can be edited with a squash merge so we don't have to wait for ofborg again

[tjni]

@figsoda did something break for you without that patch? I don't see a clear connection from the code.

[figsoda]

@figsoda did something break for you without that patch? I don't see a clear connection from the code.

This overlay wasn't working correctly for me without that patch, you would probably need to overlay rust_1_71 instead of rust for it to work, which wouldn't be very ideal

final: prev: {
  rustPackages = prev.rustPackages.overrideScope (_: prevRust: {
    cargo-auditable = prevRust.cargo-auditable.overrideAttrs (old: {
      meta = old.meta // {
        broken = true;
      };
    });
  });
  rust = prev.rust // {
    packages = prev.rust.packages // {
      stable = final.rustPackages;
    };
  };
}

[tjni]

Oh, that makes sense now. I'm ok with this being hard to overlay since the scope is reduced to handling builds gracefully on systems that cargo-auditable doesn't support. There's still room for future work targeted at making usage of cargo-auditable a configurable option.

@amjoseph-nixpkgs can you address the typo that was pointed out in the commit message (i.e. cargo-audit -> cargo-auditable)? I think we can merge then.

[figsoda]

@amjoseph-nixpkgs can you address the typo that was pointed out in the commit message (i.e. cargo-audit -> cargo-auditable)? I think we can merge then.

If you want, you can edit the commit message with a squash merge. Not sure if this change alone fixes @amjoseph-nixpkgs's issues, but this PR lgtm.

[tjni]

Cool, it worked!