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

Provide a documented way to use cargo auditable as a drop-in replacement for cargo #89

Closed
Shnatsel opened this issue Dec 6, 2022 · 9 comments

Comments

@Shnatsel
Copy link
Member

Shnatsel commented Dec 6, 2022

This came up in the pull request about enabling cargo auditable in Nix by defaut.

There are two hurdles here:

  1. cargo auditable calls cargo internally quite a lot. We need to make sure it doesn't recurse.
  2. Some targets are currently not supported, notably WebAssembly, and cargo auditable will error out in this case. This is not an option for a drop-in replacement for cargo - we need to print a warning and keep going.
@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 6, 2022

Cargo sets only one environment variable when calling subcommands, and that's a path to itself in the CARGO env var.

Just following that convention (and letting people set it to the real cargo when using as a drop-in replacement) sounds good. I'm not going to require that variable to be set because I'm not sure if that would break anything (especially exotic things like sccache).

I think if I add another env var for ignoring unsupported platforms, we should be good. So something like this would work, if placed in $PATH and called cargo:

#!/bin/sh

export CARGO='/path/to/real/cargo'
export CARGO_AUDITABLE_IGNORE_UNSUPPORTED=true
cargo-auditable auditable "$@"

btw cargo-auditable auditable is not a typo, that's how Cargo calls subcommands and we actually take advantage of it.

@figsoda @zowoq would this work for Nix?

I could write this as a Rust executable as well, but I think a shell script is way easier to understand and debug - a compiled Rust binary would just be a black box. Transparency is key when you override default behavior.

@figsoda
Copy link
Contributor

figsoda commented Dec 6, 2022

Didn't test anything out, but this should work for nix. I think a shell script in nixpkgs is good enough, no rust needed here.

@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 6, 2022

Alright, I'll implement support for these two environment variables then!

@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 6, 2022

As of commit 00030f0 the CARGO variable is implemented, so you can use the above snippet.

The CARGO_AUDITABLE_IGNORE_UNSUPPORTED isn't implemented yet, but shouldn't be a blocker for initial testing.

@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 6, 2022

Support for CARGO_AUDITABLE_IGNORE_UNSUPPORTED has landed in 74a2d3a

@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 7, 2022

Actually you can just use alias cargo="cargo auditable" without going through all this trouble. I've documented it in the README.

@Shnatsel Shnatsel closed this as completed Dec 7, 2022
@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 7, 2022

I'm open to feedback on whether continuing the build but printing a warning when targeting an unsupported platform should just be the default.

Maybe a configuration option is overkill, since this behavior is basically never desirable.

@figsoda
Copy link
Contributor

figsoda commented Dec 7, 2022

I am not sure about the semver aspect of this, but it (warning instead of error) definitely sounds like a saner default to me

@Shnatsel
Copy link
Member Author

Shnatsel commented Dec 7, 2022

I'll ship it as 0.6 just to be on the safe side

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

No branches or pull requests

2 participants