Skip to content

Setup cargo-deny #358

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

Merged
merged 2 commits into from
Aug 26, 2021
Merged

Setup cargo-deny #358

merged 2 commits into from
Aug 26, 2021

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Aug 26, 2021

Part of #294


This change is Reviewable


- uses: EmbarkStudios/cargo-deny-action@v1
with:
command: check ${{ matrix.checks }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +73 to +74
"MIT",
"Apache-2.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have set these licenses.

#"Nokia",
]
# Lint level for licenses considered copyleft
copyleft = "deny"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And denied copyleft licenses.

# Certain crates/versions that will be skipped when doing duplicate detection.
skip = [
#{ name = "ansi_term", version = "=0.11.0" },
{ name = "cfg-if", version = "=0.1.10" },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cfg-if exists both in version 0.1 and 1.0 in the dependency tree.


[sources.allow-org]
# 1 or more github.com organizations to allow git sources for
github = ["GraphiteEditor"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All git dependencies from this organization should be fine.

@@ -3,6 +3,7 @@ name = "graphite-proc-macros"
version = "0.1.0"
authors = ["Graphite Authors <[email protected]>"]
edition = "2018"
license = "Apache-2.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo-deny complained about the missing license here.

Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Keavon
Copy link
Member

Keavon commented Aug 26, 2021

Actually one thing I noticed:

Is it possible to alter this so we only get 1 entry in there? Or even 0 entries (include it as a step in build). And I also don't see Cloudflare, do you have any idea why that might be missing?

@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 26, 2021

Is it possible to alter this so we only get 1 entry in there?

This two entry scheme suggested at https://github.com/EmbarkStudios/cargo-deny-action#recommended-pipeline-to-avoid-sudden-breakages to allow CI to still pass when there are new advisories, while still preventing it from passing when there are other problems and to still show if there are new advisories.

And I also don't see Cloudflare, do you have any idea why that might be missing?

What do you mean? What does Cloudflare have to do with this?

Comment on lines 49 to 50
- advisories
- bans licenses sources
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if it would be possible to rename the actions (as they are displayed in GitHub) to "security advisories" and "banned licenses"? I didn't understand what "advisories" meant (security, not license compatibility) and "bans licenses sources" is verbose and grammatically awkward. If that is not possible to change since it is calling the action directly by that name, no worries!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! I feel bad to nitpick but I'll bring it up, however I don't feel strongly about this:

Do you think the "and crates" part of "banned licenses and crates" is redundant? Besides crates, what else would the banned licenses apply to? It makes it kind of long. Perhaps "banned crate licenses" would be a good middle ground, as it's both descriptive and shorter, and distinguishes from the node.js ecosystem licenses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can explicitly ban crates even if they have a compatible license.

@Keavon Keavon merged commit 82a1fac into GraphiteEditor:master Aug 26, 2021
@bjorn3 bjorn3 deleted the cargo-deny branch August 26, 2021 09:03
@Keavon Keavon linked an issue Aug 26, 2021 that may be closed by this pull request
2 tasks
Keavon pushed a commit that referenced this pull request Jun 16, 2022
* Setup cargo-deny

* Nicer job names
Keavon pushed a commit that referenced this pull request Jul 30, 2023
* Setup cargo-deny

* Nicer job names
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.

Set up cargo-deny (Rust) and license-checker-webpack-plugin (Node)
2 participants