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

PEP 639: Implement License-Expression and License-File #828

Merged
merged 36 commits into from
Oct 24, 2024

Conversation

ewdurbin
Copy link
Member

@ewdurbin ewdurbin commented Sep 3, 2024

Work in progress, current open questions:

  • Is the dependency on license-expression OK? Should vendoring or a new parser be considered?
    • went the vendoring route per feedback from @abravalheri
    • migrated to a pure python parser based on pypa/hatchling's implementation
  • How much validation to apply to License-File, as-is as far as metadata goes the only clear spec is "Path delimiters MUST be the forward slash character (/), and parent directory indicators (..) MUST NOT be used.". It is unclear how to check for other delimiters, as paths may have no delimiters whatsoever
    • validations with path lib seem to fit the bill!
  • How to handle deprecations/conflicts. License and License-Expression are mutually exclusive, but it doesn't appear that such a conflict has existed before.
    • Based on the current behavior of the library, this kind of concern appears to be left to the caller. The metadata module appears to be focused on correctly parsing individual fields.

pyproject.toml Outdated Show resolved Hide resolved
@cdce8p
Copy link

cdce8p commented Sep 4, 2024

IIRC one of the concerns raised in the Discourse thread was regarding the total file size if license_expression would be added as a dependency or fully vendored. Just as example, the scancode-licensedb-index.json file is over 800kB.

I believe that's one of the reasons @ofek went a slightly different route with #799.

@ewdurbin
Copy link
Member Author

ewdurbin commented Sep 4, 2024

See a828bc7, which is a variant of #799 to build/use a minimal compatible json blob from the upstream for use with the vendored license-expression. Also excluding the data files from the vendored package from the resulting sdist/wheel to save space.

Overall resulting size difference for sdist and wheel

file 24.1 before after
sdist 148 kB 276 kB 212 kB
whl 54 kB 180 kB 112 kB

uncompressed:

file 24.1 before after
sdist 2539 kB* 1690 kB 829 kB
whl 176 kB 1281 kB 426 kB

* this appears to be due to the inclusion of tests/.pytest_cache/v/cache/lastfailed and tests/.pytest_cache/v/cache/nodeids files in the sdist, without it it is 608 kB.

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

  1. Can you please save the data as Python so deserialization is not required?
  2. I think we shouldn't merge this until we satisfy the parsing needs of backends (i.e. user defined metadata) rather than just consumers of packages like PyPI.
  3. Although I'm not a maintainer, I would be quite weary of packaging beginning to vendor dependencies.

@ewdurbin
Copy link
Member Author

ewdurbin commented Sep 4, 2024

  1. Can you please save the data as Python so deserialization is not required?

Looked closer and saw that hatchling's implementation is more complete than I initially understood, so I've migrated this PR to use an adapted version of that, and moved storage of the license data back to using the python based representation

  1. I think we shouldn't merge this until we satisfy the parsing needs of backends (i.e. user defined metadata) rather than just consumers of packages like PyPI.

Which needs are those? Can you delineate them? I'm interested in seeing this merged so PyPI's implementation can progress.

  1. Although I'm not a maintainer, I would be quite weary of packaging beginning to vendor dependencies.

See response to 1. :)

@ofek
Copy link
Contributor

ofek commented Sep 4, 2024

  1. I think we shouldn't merge this until we satisfy the parsing needs of backends (i.e. user defined metadata) rather than just consumers of packages like PyPI.

Which needs are those? Can you delineate them? I'm interested in seeing this merged so PyPI's implementation can progress.

Now that you have normalize_license_expression which also raises an exception for invalid stuff, every backend would be satisfied. Thanks!

@ewdurbin ewdurbin mentioned this pull request Sep 4, 2024
Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Looks fantastic to my non-maintainer eyes!

@befeleme
Copy link

befeleme commented Sep 5, 2024

Just a tiny nitpick: The commit message contains a typo: "PEP 693" instead of 639 - this will make digging in the git history harder in the future.

Copy link
Contributor

@hroncok hroncok left a comment

Choose a reason for hiding this comment

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

How much validation to apply to License-File, as-is as far as metadata goes the only clear spec is "Path delimiters MUST be the forward slash character (/), and parent directory indicators (..) MUST NOT be used.". It is unclear how to check for other delimiters, as paths may have no delimiters whatsoever

I think the spirit of the rule is that there are no backlash-delimeters in the path. However, I am not sure if this is enforceable if a filename can contain a backlash.

Also, as the path MUST be relative, we could assert it does not start with /.


I also noticed a license file is never checked for existence. Is that intentional? The PEP states it MUST be present (and the file content MUST be UTF-8 encoded text).

tests/test_metadata.py Outdated Show resolved Hide resolved
although inconsistent (see Requires-External, Provides-Extra, Provides-Dist, Obsoletes-Dist...) pluralize the result for multiple use of `License-File`

https://packaging.pypa.io/en/stable/metadata.html#packaging.metadata.RawMetadata

> Any core metadata field that can be specified multiple times or can hold multiple values in a single field have a key with a plural name.
Per feedback, integrate a variant of pypa#799 that builds a minimal JSON dataset to feed vendored license-expression

32K	src/packaging/_spdx.json

vs

848K	src/packaging/_vendor/license_expression/data/scancode-licensedb-index.json
- back away from vendoring
- adapt hatchling's license expression parser to accept well-formed LicenseRef-
- back to python storage of license data
@ewdurbin ewdurbin changed the title PEP 693: Implement License-Expression and License-File PEP 639: Implement License-Expression and License-File Sep 5, 2024
@ewdurbin
Copy link
Member Author

ewdurbin commented Sep 5, 2024

Just a tiny nitpick: The commit message contains a typo: "PEP 693" instead of 639 - this will make digging in the git history harder in the future.

🤦🏼 resolved with a rebase/force-push

@brettcannon
Copy link
Member

Thanks for your patience with this, @ewdurbin !

@pradyunsg did you want to review this or are you okay to merge? Once we do I will add the new label for issues.

@ewdurbin
Copy link
Member Author

Given that this has approval from at least one maintainer... I put the effort into a draft PR for implementation in warehouse is now up at pypi/warehouse#16949

@pradyunsg
Copy link
Member

@pradyunsg did you want to review this or are you okay to merge?

Okay to merge.

@henryiii henryiii mentioned this pull request Oct 24, 2024
6 tasks
@brettcannon brettcannon merged commit 029f415 into pypa:main Oct 24, 2024
33 checks passed
@brettcannon
Copy link
Member

Merged! Thanks, everyone!

Off to create a new label for our latest module!

@henryiii
Copy link
Contributor

Of to go rebasing my PRs on this now! ("Now" meaning after teaching today, probably)

@ewdurbin
Copy link
Member Author

Thanks everyone! Eagerly awaiting a release so that PyPI's implementation can be deployed :)

@ewdurbin ewdurbin deleted the pep_639 branch October 24, 2024 17:57
@henryiii
Copy link
Contributor

I'd like to get a PR for extra validation in before a release, as it's easier to add validation before a release than afterwords. I should know if it's needed and have it added by tomorrow.

@henryiii
Copy link
Contributor

The validation errors I was looking for are included, so good to go from me!

Question, though: would adding a warning if a License :: classifier is present and metadata 2.4+ is encountered be a good idea? Likewise, if 2.4 is set and a classic license is used. I don't see another similar warning in packaging.metadata, though.

Basically, this: https://github.com/pypa/pyproject-metadata/blob/cb7450073acecefc714cc5de82816799841777b9/pyproject_metadata/__init__.py#L505-L517

Warning can always be added after a release, unlike errors, so not a blocker at all.

@ewdurbin
Copy link
Member Author

From the PEP:

If the License-Expression field is present, build tools MAY raise an error if one or more license classifiers is included in a Classifier field, and MUST NOT add such classifiers themselves.

and

If only the License field is present, tools MAY issue a warning informing users it is deprecated and recommending License-Expression instead.

So both of those warnings seem to be inline with the PEP.

@henryiii
Copy link
Contributor

Yes, that’s where I got the warnings, but they are MAY, I assume we get to pick.

@brettcannon
Copy link
Member

would adding a warning if a License :: classifier is present and metadata 2.4+ is encountered be a good idea?

It's a hard call for in 'packaging' itself. If we put it in it will definitely come up more, but for a "MAY" it feels like we might be going beyond just being a library.

@ofek
Copy link
Contributor

ofek commented Oct 25, 2024

Please do not emit a warning in this foundational library.

@henryiii
Copy link
Contributor

Okay, sounds good. Users can add the MAY themselves, then.

@ewdurbin
Copy link
Member Author

ewdurbin commented Nov 8, 2024

With the release of packaging 24.2, the PyPI implementation at pypi/warehouse#16949 is ready for final review.

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.