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

napi: fix build error in cargo-auditable #713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JohnRTitor
Copy link

As reported #702
when trying to build via cargo auditable build --release --lib --bin=lightningcss --features="cli" build fails with the following error.

   Compiling lightningcss v1.0.0-alpha.55 (/home/masum/Dev-Environment/lightningcss)
    Building [=======================> ] 169/171: lightningcss                            thread 'main' panicked at cargo-auditable/src/collect_audit_data.rs:77:9:
cargo metadata failure: error: Package `lightningcss-napi v0.1.0 (/home/masum/Dev-Environment/lightningcss/napi)` does not have feature `rayon`. 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.

This patch fixes the bug and the build succeeds on cargo auditable.

@JohnRTitor
Copy link
Author

Should close #702

JohnRTitor added a commit to JohnRTitor/nixpkgs that referenced this pull request Apr 6, 2024
Additionally patch ./napi/Cargo.toml to allow building lightningcss-napi
parcel-bundler/lightningcss#713
parcel-bundler/lightningcss#702
@devongovett
Copy link
Member

Why would the dep for crossbeam-channel and rayon be different here? Not sure I understand the issue here.

@JohnRTitor
Copy link
Author

I am guessing cargo auditable does not think of "rayon" as a hard dependency thus producing this error, as you can see in the error message.

This behaviour is not observed in cargo however. We are currently using a patch on Nixpkgs to let it build by removing dep. NixOS/nixpkgs@5af750a

@devongovett
Copy link
Member

crossbeam-channel is also optional though

@JohnRTitor
Copy link
Author

I am not sure, something must have changed between the bump to 1.24.1 v1.24.0...v1.24.1

toastal added a commit to toastal/nixpkgs that referenced this pull request May 18, 2024
<system-color> panic regression still persists
parcel-bundler/lightningcss#685

patch unmerged for napi & cargo auditable
parcel-bundler/lightningcss#713
@JohnRTitor
Copy link
Author

@devongovett is there a reason to hold off this PR, or is it blocked on something?

@devongovett
Copy link
Member

Mainly that I don't understand the problem. AFAICT it is correct: https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies Why do you think this is wrong? I don't know what cargo-auditable is but perhaps that has a bug?

@JohnRTitor
Copy link
Author

cargo-auditable is a tool to store the exact versions of dependencies in rust binaries and makes it easier/possible to audit the dependency tree of rust binaries.

It is a drop-in replacement of cargo. Mainly this lets easier identification of vulnerabilities (CVEs). Nixpkgs by default uses cargo auditable to build rust binaries.

Stricter checks like these are implemented by cargo-auditable. And hence this PR. cargo auditable is compatible with cargo itself, so this change won't affect builds with cargo.

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

Successfully merging this pull request may close these issues.

2 participants