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

compatibility with cargo auditable #962

Closed
Lunarequest opened this issue Aug 3, 2023 · 8 comments
Closed

compatibility with cargo auditable #962

Lunarequest opened this issue Aug 3, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@Lunarequest
Copy link

Summary 💡

cargo_audtiable aims to make it possible to know the versions of packages compiled in a rust executable there by making it possible to identify vulnerabilities in production binaries currently when building with cargo auditable you get this error

'main' panicked at 'cargo metadata failure: error: Package `gix-features v0.32.1 (/home/Luna/gitoxide/src/gix-features)` does not have feature `prodash`. It has an optional dependency with that name, but that dependency uses the "dep:" syntax in the features table, so it does not have an implicit feature with that name.
cargo-auditable/src/collect_audit_data.rs:77:9

Motivation 🔦

No response

@Lunarequest Lunarequest added the enhancement New feature or request label Aug 3, 2023
@Byron
Copy link
Owner

Byron commented Aug 3, 2023

This seems to be tracked here and turned out to be a cargo metadata problem.

@Byron Byron closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2023
@jalil-salame
Copy link
Contributor

In order to build gitoxide in nixpkgs, I patched the v0.29.0 gitoxide repo, the patchset is very minimal: Two lines modified each in a different Cargo.toml, no functional changes, would you be interested in upstreaming it? @Byron

See NixOS/nixpkgs#254087

It changes this line https://github.com/Byron/gitoxide/blob/main/gitoxide-core/Cargo.toml#L26 dep:tracing -> tracing

And this line https://github.com/Byron/gitoxide/blob/main/gix-features/Cargo.toml#L20 dep:prodash -> prodash.

This is the full patch:
fix-cargo-auditable.txt

@Byron
Copy link
Owner

Byron commented Sep 14, 2023

Thanks for the patch. I couldn't figure out why this would fix anything though, so I fear it will break again. When using older versions of cargo I noticed that it tends to interpret dep: (or the lack thereof) differently, maybe that's what's happening here? Here is the PR for it.

Besides, I had more trouble than I anticipated applying the patch provided here. git am < patch was supposed to work, but failed with an error message about the author email being empty (clearly not the case). So I ended up applying the patch only, and adjusting the author by hand which fortunately was easy with git commit -a 'name <email>' --amend.

If you have any tips on how applying the patch should have worked I'd be grateful.

@jalil-salame
Copy link
Contributor

Thanks for the patch. I couldn't figure out why this would fix anything though, so I fear it will break again. When using older versions of cargo I noticed that it tends to interpret dep: (or the lack thereof) differently, maybe that's what's happening here? Here is the PR for it.

It's a cargo metadata bug that happens on an edge case of cargo feature parsing:

If you define mydep = {version="*", optional=true} cargo will create a mydep feature, unless a dep:myfeature feature is present, this is to allow a mydep = ["dep:mydep", "feat", ...] feature to have no conflicts.

To prevent it there are 2 options:

  1. Only use dep:mydep when you have a feature named mydep
  2. Build with cargo auditable and rename dep:mydep when there is a build error.

Option 1 is easier for you to apply, but I will make PRs if cargo auditable breaks as I can notice with a ~24h delay after a new version of gitoxide comes to nixpkgs-unstable.

Besides, I had more trouble than I anticipated applying the patch provided here. git am < patch was supposed to work, but failed with an error message about the author email being empty (clearly not the case). So I ended up applying the patch only, and adjusting the author by hand which fortunately was easy with git commit -a 'name <email>' --amend.

If you have any tips on how applying the patch should have worked I'd be grateful.

I think git apply should work? Nix uses patch directly as it only patches the source code and doesn't actually create a commit.

I messed this up as it should've been a PR directly 😅, I even have a fork of gitoxide with the commit applied locally.

@Byron
Copy link
Owner

Byron commented Sep 14, 2023

I think git apply should work? Nix uses patch directly as it only patches the source code and doesn't actually create a commit.

It does, but it skips the commit message and the author as well. I assumed you might want to keep this information in the commit, since you provided it. In theory, git am can do the trick, but it fails as well. Time for gix apply I suppose ;).

I messed this up as it should've been a PR directly 😅, I even have a fork of gitoxide with the commit applied locally.

PRs are definitely preferred, even though I am generally open to other channels as long as I find a way to apply these file-based commits correctly :D.

@jalil-salame
Copy link
Contributor

I made the patch with git format patch to get it into nixpkgs, I didn't know if removing the commit information would mess it up so I left it there.

@Byron
Copy link
Owner

Byron commented Sep 14, 2023

git apply can apply it well, and so can patch, but both drop the commit information. git am is supposed to be able to deal with such a patch, but it fails. I have a feeling it's a parsing bug in git am - git am your-patch can't detect the format, and git am < your-patch applies the patch, but fails to parse the commit portion with fatal: empty ident name (for <>) not allowed. It's quite a desaster.

It seems nixpkgs has a way to apply these nonetheless.

@jalil-salame
Copy link
Contributor

It seems nixpkgs has a way to apply these nonetheless.

nixpkgs doesn't apply patches to a git repo so it ignores the commit info c:

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

No branches or pull requests

3 participants