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

Validate SPDX license expressions #217

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Nov 8, 2024

Followup to #210. Implement the validation for PEP 639.
Most of the test cases are directly from the packaging PR: pypa/packaging#828

@henryiii
Copy link
Collaborator

henryiii commented Nov 8, 2024

src/validate_pyproject/formats.py:386: error: Module
"setuptools._vendor.packaging" has no attribute "licenses"  [attr-defined]
            from setuptools._vendor.packaging import (  # type: ignore[no-...
            ^
src/validate_pyproject/formats.py:386: note: Error code "attr-defined" not covered by "type: ignore" comment

# let's try setuptools vendored version
from setuptools._vendor.packaging import ( # type: ignore[no-redef]
licenses as _licenses,
)
Copy link
Owner

@abravalheri abravalheri Nov 8, 2024

Choose a reason for hiding this comment

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

I don't think we need to cover this case...

  • Newer versions of setuptools use this strategy for exposing the _vendor directory:

    sys.path.extend(((vendor_path := os.path.join(os.path.dirname(os.path.dirname(__file__)), 'setuptools', '_vendor')) not in sys.path) * [vendor_path])  # fmt: skip

    So packaging will be available directly, without the need for prefixing it with setuptools._vendor.

  • Older versions of setuptools will contain older versions of packaging so importing setuptools._vendor.packaging.licenses will always fail, right?

Copy link
Owner

Choose a reason for hiding this comment

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

I am just wondering now if we should add right at the beginning of the file:

try:
    import setuptools  # Just interested on the side effect in `sys.path`
except ImportError:
    pass

... however, this may:

  1. Hurt performance
  2. Cause version conflict errors when there would be no need for modifying sys.path

So maybe the best for now is to leave the implementation as suggested in the current state of the PR. We can revisit this later if we receive bug reports.

@abravalheri
Copy link
Owner

Thank you very much for having a look on this @cdce8p.

Co-authored-by: Anderson Bravalheri <andersonbravalheri+github@gmail.com>
@abravalheri abravalheri merged commit f45606b into abravalheri:main Nov 11, 2024
21 checks passed
@abravalheri
Copy link
Owner

Thank you very much @cdce8p

@cdce8p cdce8p deleted the validate-spdx-license-expression branch November 11, 2024 14:23
@cdce8p
Copy link
Contributor Author

cdce8p commented Nov 11, 2024

@abravalheri Would you mind creating a new release for it? I'd love to use that in setuptools. See pypa/setuptools#4734

@abravalheri
Copy link
Owner

Hi @cdce8p I am preparing one.

Regarding setuptools, it might take still a while, I would prefer to pipeline the implementation of the metadata PEPs, so that we have consistency... This may mean retaining an old version of the validation while the version 2.2 is not fully merged yet.

@cdce8p
Copy link
Contributor Author

cdce8p commented Nov 11, 2024

Hi @cdce8p I am preparing one.

Thanks!

Regarding setuptools, it might take still a while, I would prefer to pipeline the implementation of the metadata PEPs, so that we have consistency... This may mean retaining an old version of the validation while the version 2.2 is not fully merged yet.

I can understand the intention, although as explained in pypa/setuptools#4706 (comment) I don't think that blocks us from added the support for License-Expression early. It's just an additional building block to eventually get to the latest metadata version. It would be unfortunate IMO if these setuptools PRs have to wait even though they are fine on their own.

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.

3 participants