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

Add verify-pyproject-license hook #30

Merged
merged 10 commits into from
May 17, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Apr 30, 2024

Contributes to #29.

@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner April 30, 2024 17:18
vyasr
vyasr previously requested changes May 1, 2024
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Overall looks good.

I'm fine merging this with only pyproject license support since that is the more immediate need (where we get bit by wheeltamer), but we probably want to 1) add tooling to also validate our conda meta.yaml files, and 2) add logic for validating other fields. For 2, other fields we should standardize:

  1. Author name (NVIDIA Corporation)
  2. Classifiers: some packages will want more classifiers than others, but there are some base ones that we ought to be able to standardize. For instance, making sure the Python versions are consistent.

src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented May 3, 2024

Just as a survey of various Python build tools:

  • Poetry says to use license with SPDX identifiers (so Apache-2.0, not Apache 2.0)
  • Python Packaging User Guide says don't use license if your project's license is well-known, use classifiers like License :: OSI Approved :: Apache Software License instead

I don't know what our constraints are with respect to the internal wheel validation process, but using SPDX and a classifier would seem to be the optimal solution imo.

@jakirkham
Copy link
Member

It looks like all review requests were resolved. Requesting a new round of reviews

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Some clean up and deduplicating, otherwise looks good.

src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A couple comments - please resolve them as you see fit. You can merge without waiting for more review from me if you wish.

src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
src/rapids_pre_commit_hooks/pyproject_license.py Outdated Show resolved Hide resolved
@KyleFromNVIDIA KyleFromNVIDIA merged commit 6c8aa87 into rapidsai:main May 17, 2024
2 checks passed
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.

5 participants