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

Remove cargo-deny exception for intel-mkl-src #386

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Tastaturtaste
Copy link
Contributor

As conversed about in PR #369, this PR removes the cargo-deny exclusion for intel-mkl-src introduced due to EmbarkStudios/krates#60.

This PR should be merged as soon as EmbarkStudios/krates#61 lands in cargo-deny-action.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.92%. Comparing base (c7673ef) to head (748787d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #386   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files         177      177           
  Lines       23724    23724           
=======================================
  Hits        21808    21808           
  Misses       1916     1916           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@jedbrown
Copy link

It seems this is all complete. Should this be merged now?

@Tastaturtaste
Copy link
Contributor Author

It seems this is all complete. Should this be merged now?

The CI build should be rerun to confirm. If that is green it should be good to merge.

@stefan-k
Copy link
Member

Would you mind rebasing this PR or allowing me to push to it? Thanks! :)

@Tastaturtaste
Copy link
Contributor Author

I can rebase in the next few days. Also, permission for maintainer to edit this PR should already be given.

@Tastaturtaste
Copy link
Contributor Author

Seems like cargo-deny picks up intel-mkl-src through the "examples" crates, even though those crates have "publish = false" set in their manifest and "deny.toml" has "licenses.private.ignore = true" set. If the "examples" folder is just removed from the workspace manifest members there are no license errors. I think this might not be the intended behavior for the "licenses.private.ignore" option of cargo deny. Maybe this is another bug in their implementation? I don't have the time to look closer into this right now. Maybe in the next few days, but no promises.

@stefan-k
Copy link
Member

stefan-k commented Sep 1, 2024

Thanks for the quick rebase, and thanks for digging into this. I've also had a look, and I suspect the path based dependencies are treated differently. I tried to replace them by workspace dependencies but to no avail. If you do look into this, please do not waste too much time on this annoying problem.

Also, permission for maintainer to edit this PR should already be given.

Interestingly I still cannot push to this branch.

… avoid license checks for dependencies that are not used in the published crates
@Tastaturtaste
Copy link
Contributor Author

I get no errors with cargo deny --all-features --log-level warn check licenses bans sources in a local checkout of my branch, both on windows and linux (wsl) with [email protected]. Seems to me there is something wrong with the cargo-deny-action? Seems the right version of cargo-deny is used in the workflow, but the necessary keys graph.exclude-unpublished and graph.exclude-dev are not used in the call to cargo-deny. I opened EmbarkStudios/cargo-deny-action#81 to investigate.

@Tastaturtaste
Copy link
Contributor Author

Tastaturtaste commented Nov 21, 2024

Turns out I was wrong to suspect the cargo-deny-action. I just didn't see errors locally since I did run cargo update and thus my cargo.lock was different from the one generated with a fresh checkout of the branch in the workflow. It appears with updates to some crates the "Unicode-3.0" license got into the dependency graph.

According to https://www.unicode.org/policies/licensing_policy.html the license is based on the MIT license, with the difference that the Unicode-3.0 license is targeted explicitly at data and data files:

The Consortium’s software and data files are generally licensed under the OSI-approved Unicode License v3, a free, open source, highly permissive license based on the MIT License. The primary difference between the MIT License and the Unicode License is that the Unicode License expressly covers data and data files. The Unicode License v3 and its predecessors have been approved by the Open Source Initiative (OSI) since 2015.

It seems the license is very permissive. However to be sure I would follow suite with the most restrictive use of the license in all of the dependencies, which appears to be https://crates.io/crates/unicode-ident, and change the license of argmin to "(MIT OR Apache-2.0) AND Unicode-3.0". Or track down why dependencies with this license got into the dependency graph in the first place and try to prune them.

Edit: It seems even https://crates.io/crates/proc-macro2/1.0.91 depends on unicode-ident without mentioning the Unicode-3.0 license. It also uses "MIT OR Apache-2.0" as the license terms. So maybe it really is just fine to allow this license without a problem. I will prepare a commit under this assumption, if you want something else let me know.

@Tastaturtaste
Copy link
Contributor Author

The failed check for advisories is due to unmaintained crates in the dependency graph. EmbarkStudios/cargo-deny#611 removed the ability to set the severity of different advisories, so the previously set unmaintained = warn is now deny. As noted in EmbarkStudios/cargo-deny#611 the offending advisories have to be handled via an ignore.

… through the cargo-deny config key �dvisories.unmaintained = warn, which was deprecated in EmbarkStudios/cargo-deny#611.
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.

4 participants