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 support for License-Expression parsing (PEP 639) #213

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Oct 11, 2024

PEP 639 has been (provisionally) accepted recently. hatch added support1 for it early on and so the first packages which store the license metadata inside the License-Expression field start popping up. E.g. for ftfy it looks like this

...
License-Expression: Apache-2.0
License-File: LICENSE.txt
...

This PR adds the basic parsing support without any validation similar to the existing behavior for the License metadata field or the classifier. It might make sense to add the validation at a later point though.

To keep the changes to a minimum, the behavior of --from=mixed is preserved. However:

Caution

Breaking change: The --from=all output now includes the License-Expression value.

Fixes #225

Footnotes

  1. https://hatch.pypa.io/1.9/config/metadata/#spdx-expression

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.14%. Comparing base (3fbf20f) to head (84b2837).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   99.13%   99.14%           
=======================================
  Files           1        1           
  Lines         462      466    +4     
=======================================
+ Hits          458      462    +4     
  Misses          4        4           

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

@stefan6419846
Copy link

Isn't the proposed handling against PEP 639? According to https://packaging.python.org/en/latest/specifications/core-metadata/#license, the following holds true:

As of Metadata 2.4, License and License-Expression are mutually exclusive. If both are specified, tools which parse metadata will disregard License and PyPI will reject uploads. See PEP 639.

With PyPI not allowing both keys to be set and License-Expression taking precedence otherwise, wouldn't it make more sense to retrieve license-expression as the first variant in METADATA_KEYS["license"]? This would avoid the breaking change as well.

@cdce8p
Copy link
Contributor Author

cdce8p commented Oct 30, 2024

With PyPI not allowing both keys to be set and License-Expression taking precedence otherwise, wouldn't it make more sense to retrieve license-expression as the first variant in METADATA_KEYS["license"]?

Yeah, you're correct. License-Expression should take precedence and all other values should be ignored. My initial thought was to preserve backwards compatibility as much as possible and not change the precedence order on an existing option. In practice it shouldn't make much of a difference as "normally" only one should be defined (either License-Expression or License metadata / classifier). Anyway pushed a new commit to update the logic.

This would avoid the breaking change as well.

Not quite sure about that one. The breaking change is in the --from=all output. It adds a new column, so for users who depend on the exact form it's a breaking change.

Julian added a commit to python-jsonschema/jsonschema that referenced this pull request Dec 28, 2024
The License field is deprecated, as is the trove classifier.

Note that pip show does not support properly detecting these yet,
but it will.

Note also that `pip-licenses` does not support PEP 639 (see
raimon49/pip-licenses#213) the implications of which are that we are
already broken (the `license_check` noxenv fails because of packages
already using the newer standard). This doesn't fix that yet. AFAICT no
tool exists that does this properly yet/now. So let's see... I guess we
reimplement that functionality?!

Refs: https://peps.python.org/pep-0639/
Refs: pypa/pip#13112
Refs: pypa/pip#6677
Copy link

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Thanks for this PR ! Could you update the dependency file to add a package using license expression ? I proposed attrs but any other package doing it is fine :)

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 28, 2025

Could you update the dependency file to add a package using license expression ? I proposed attrs but any other package doing it is fine :)

I'd prefer not to add an additional test dependency just for that. The last prettytable release also added License-Expression. Once #222 is merged, I can update this PR / or do a followup to enable the asserts.

@SMoraisAnsys
Copy link

Could you update the dependency file to add a package using license expression ? I proposed attrs but any other package doing it is fine :)

I'd prefer not to add an additional test dependency just for that. The last prettytable release also added License-Expression. Once #222 is merged, I can update this PR / or do a followup to enable the asserts.

Attrs is already a dependency (see the top of the dependency file)
But sure, prettiable is a good target :)

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 28, 2025

Attrs is already a dependency (see the top of the dependency file) But sure, prettiable is a good target :)

It's only a transitive dependency on an old pytest version. Will be removed with #217 it seems. Anyway, I'd like to keep dependency bumps separate. Will update the asserts here, if #217 and / or #222 ever gets merged.

@SMoraisAnsys
Copy link

@raimon49 Sorry for the direct ping but this package doesn't seem to have much activity since 6 months. Could you have a look at this PR or should we assume the repo to be dead and move with a fork ?

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.

request: support PEP 639
3 participants