Add initial support for license expression (PEP 639)#4706
Conversation
4c8ccfb to
1237a7d
Compare
1237a7d to
93d0c67
Compare
|
Hi @cdce8p thank you very much for having a look on this. We probably have to organise so that PEP 639 lands after PEP 685 implementation (ideally we want a couple of days/weeks between the two so that users have time to report potential bugs). |
93d0c67 to
390b95f
Compare
Maybe I'm missing something obvious but I'm not quite sure how PEP 685 is related. Would you mind explaining it a bit more? The way I've designed the PR, it shouldn't depend on anything. It just allows users to specify |
|
Opened #4734 with just the |
The problem is that the metadata version of the core metadata is incremental:
So we cannot implement PEP 639 before 685, otherwise metadata validation may fail. Even if the PR is independent we cannot merge it now because of the nature of the releases of setuptools (we cannot risk PEP 639 support being released before PEP 685 support). |
The validation will only happen once we update the metadata version. If not, a lot of wheel uploads would probably fail as we currently include What I'm proposing here is simply to add the basic support to setuptools. It won't get validated by PyPI until we're ready to move to |
|
For sure the existence of the By accepting only some aspects of PEP 639, we add other kinds of spec violations that some tools may not be prepared to deal with. So for the sake of reducing the unknown risks, I propose we pipeline the changes you kindly contributed after the implementation of 685. Unfortunately, it means that people cannot start modifying their |
|
Sorry, I have been using the wrong PEP number, sorry for the confusion. PEP 685 is important, but the really big one for us to implement before 639 is PEP 643 (the one with |
I understand your reasoning. One last suggestion. The last days I've been looking into how My suggestion
What do you think? |
390b95f to
97e8f94
Compare
|
@abravalheri With the last updates, I removed the |
97e8f94 to
f3a1fdd
Compare
| if "file" in val: | ||
| if isinstance(val, str): | ||
| _set_config(dist, "license_expression", _static.Str(val)) | ||
| elif "file" in val: |
There was a problem hiding this comment.
@cdce8p I updated this part of the code to solve the merge conflict. I hope that is OK.
There was a problem hiding this comment.
Thanks. Feel free to ping me, especially if one of my PRs need attention. I'm usually fairly quick to respond.
Small side note. If I review PRs from other people, I only update them with merge commits. The rebase one makes it unnecessarily difficult for the OP to follow and understand what's changed on their own PR. I myself only do force pushes if my PR hasn't been reviewed yet, afterwards only merge commits.
If that's ok with you, I'll force push the rebase once #4796 is done. Just so I'm aware what has changed.
There was a problem hiding this comment.
If that's ok with you, I'll force push the rebase once #4796 is done. Just so I'm aware what has changed.
Thanks @cdce8p. That would be helpful.
I usually try to avoid merging the full main branch back to a PR, because it messes up the graph visualisation (which becomes difficult to follow), that is why I went with a rebase, sorry if that disrupted your workflow.
There was a problem hiding this comment.
I usually try to avoid merging the full main branch back to a PR, because it messes up the graph visualisation (which becomes difficult to follow), that is why I went with a rebase, sorry if that disrupted your workflow.
Don't worry about it. Guess everyone has their own preferred workflow. Just wrote that as it took me a bit to actually see what you've changed during the rebase.
Thanks @cdce8p. That would be helpful.
👍🏻
f3a1fdd to
e42c5c4
Compare
e42c5c4 to
c825f3e
Compare
| write_field(field, attr_val) | ||
|
|
||
| license = self.get_license() | ||
| license = self.license_expression or self.get_license() |
There was a problem hiding this comment.
So this mean that we need a follow-up PR for the License-Expression, right?
There was a problem hiding this comment.
Correct. This PR was designed so that it itself didn't require a metadata_version bump. That could / should happen at a later point once all pieces are in place. I.e. setuptools has adopted version 2.3, the license files are stored in the correct folder and license expression parsing is handled correctly.
|
Thank you very much @cdce8p. How do you feel about rebasing this on the |
2b6ae9f to
0d8f1f2
Compare
Done. As expected it just removed the first commit here. Before the rebase I pushed 0d8f1f2 to change the error to the Let me know if there is anything else I should address here. |
|
Problem with cygwin in the CI possibly solved by #4832 |
| license_classifiers_found = False | ||
| for cl in self.metadata.get_classifiers(): | ||
| if not cl.startswith("License :: "): | ||
| classifiers.append(cl) | ||
| continue | ||
| license_classifiers_found = True | ||
| SetuptoolsDeprecationWarning.emit( | ||
| "License classifier are deprecated in favor of the license expression.", | ||
| f"Please remove the '{cl}' classifier.", | ||
| see_url="https://peps.python.org/pep-0639/", | ||
| due_date=(2027, 2, 17), # Introduced 2025-02-17 | ||
| ) | ||
| if license_classifiers_found: | ||
| self.metadata.set_classifiers(classifiers) |
There was a problem hiding this comment.
So if the user does not use a license expression in pyproject.toml, we are still going to emit license classifiers in the core metadata right? (because the for-loop is nested inside the if license_expr).
Should we *always skip license classifiers (with the warning) even if the project does not explicitly use a license expression (e.g. instead it relies on the license files)?
(It might also be the case your intention is to introduce a more timid version of the skip/warning in this PR, and later un-nest it and make it more prominent in a follow up. Please let me know if that is the case).
There was a problem hiding this comment.
Tbh I don't think setuptools should modify the classifiers in the first place. We should either emit an error or a warning to the user to do so themselves. Everything else would just be surprising in the end. Pushed a new commit to revert that part.
(It might also be the case your intention is to introduce a more timid version of the skip/warning in this PR, and later un-nest it and make it more prominent in a follow up. Please let me know if that is the case).
At some point it might make sense to add another warning to nudge users to use license expressions. However, I do not plan to add it at this time.
* PEP 639: use SPDX-expression for license * remove license classifier * require setuptools >= 77, see pypa/setuptools#4706
Summary of changes
validate_pyprojectto0.23https://github.com/abravalheri/validate-pyproject/blob/v0.23/CHANGELOG.rst
https://github.com/abravalheri/validate-pyproject/releases/tag/v0.23
If present, the normalized license expression is written to the
Licensefield until metadata version2.4is supported.Refs: #4629