Add support for `license_files` specified in `setup.py` #466

abravalheri · 2022-09-23T11:49:08Z

As discussed in #465, bdist_wheel will behave differently when license_files is specified in setup.py or in setup.cfg.

The main idea of this PR is to make the behaviour consistent across these different possibilities.

I am supposing that the original intention of the wheel authors was to also support distributions using from distutils.core import setup in addition to from setuptools import setup.
In my mind this justifies the choice for distribution.get_option_dict("metadata") instead of simply looking up in distribution.metadata.
Therefore, I am attempting to preserve this backward compatibility in the PR.

If this is not required the implementation. can be further simplified.

Closes #444.
Closes #465.

codecov · 2022-09-23T11:50:43Z

Codecov Report

Base: 67.57% // Head: 66.77% // Decreases project coverage by -0.79% ⚠️

Coverage data is based on head (16c90f4) compared to base (da67526).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
- Coverage   67.57%   66.77%   -0.80%     
==========================================
  Files          12       12              
  Lines         916      900      -16     
==========================================
- Hits          619      601      -18     
- Misses        297      299       +2

Impacted Files	Coverage Δ
src/wheel/bdist_wheel.py	`47.28% <100.00%> (-2.35%)`	⬇️
src/wheel/metadata.py	`91.93% <0.00%> (-6.46%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

It does not seem to be possible to run `bdist_wheel` directly with distutils.

abravalheri · 2022-09-26T17:19:14Z

The errors in the CI seem to be related to the fact that earlier versions of setuptools (between 45.2.0 and 57.0.0) would process license_file/license_files differently from what wheel expects in its test suite.

When the version of setuptools is updated, the tests seem to run without problems. For example:

> docker run --rm -it ubuntu:20.04 /bin/bash

apt update -y
apt install -y software-properties-common
add-apt-repository -y ppa:deadsnakes/ppa
apt update -y
apt install -y curl gcc git python3.7 python3.7-dev python3.7-venv
python3.7 -m ensurepip
python3.7 -c 'import setuptools; print(setuptools.__version__)'  # ==> 47.1.0

apt install -y --reinstall python3.7-distutils
curl -sSL https://bootstrap.pypa.io/get-pip.py | python3.7 -

cd /tmp
git clone -q https://github.com/abravalheri/wheel.git -b license_files
cd /tmp/wheel

python3.7 -m pip install --no-binary=:all: .
python3.7 -m pip install .[test] coverage[toml]

python3.7 -c 'import wheel; print(wheel.__version__)'  # ==> 0.37.1
pytest -W always  # ==> Fails: old setuptools behaviour doesn't match wheel's expectations

python3.7 -m pip install 'setuptools==57.0.0'
pytest -W always  # ==> succeds

python3.7 -m pip install -U setuptools
pytest -W always  # ==> succeds

It seems to be hard to reconcile the behaviour with old versions of setuptools... I don't know if the complexity is worthy.

Please let me know if you would like me to:

Increase complexity to ensure old versions of setuptools maintain the same behaviour
Update the CI to run with newer versions of setuptools
Simplify the implementation and always use distribution.metadata instead of distribution.get_option_dict("metadata").

agronholm · 2022-10-17T20:04:04Z

I think I would prefer the third option. Do you see any downsides for that?

abravalheri · 2022-10-19T13:02:59Z

Hi @agronholm, I submitted a few commits simplifying the code with some of the assumptions, just to see how it would work. If necessary I can revert the commits to get the PR to an earlier stage.

The main changes are the following:

8055d1c - Here I am always relying on distribution.metadata instead of distribution.get_option_dict("metadata").
- I still keep some of the code for backward compatibility
- I could see in some versions of setuptools distribution.metadata.license_files = [] even when the user does not explicitly specify it. This effectively disables the default files and break the test suite expectations.
9fb0b76 - Here I remove all code for backward compatibility with old versions of setuptools and rely 100% on it already processing license_files.
I added a few xfail to the test suite for old versions of setuptool (I also added another xfail in 48fe0d6)

(The other changes mainly target the CI error)

Do you see any downsides for that?

The main downside here is that developers using python setup.py bdist_wheel instead of python -m build AND that run the build in machines with old versions of setuptools installed may have a different set of license files included in the wheel.

This may also happen for people pinning versions of setuptools older than 57.0.0 in their pyproject.toml [build-system] requires.

Version 57.0.0 was released last march.

abravalheri · 2022-10-19T13:03:44Z

If you would like, I can also bump install_requires to setuptools >= 57.

agronholm · 2022-10-19T13:37:05Z

If you would like, I can also bump install_requires to setuptools >= 57.

If this change indeed drops compatibility for earlier versions, then yes. Also, please add a note to the changelog.

agronholm

These suggestions fix a typo.

tests/test_bdist_wheel.py

Co-authored-by: Alex Grönholm <alex.gronholm@nextday.fi>

abravalheri · 2022-10-19T15:02:34Z

Thank you very much for the review @agronholm.

If this change indeed drops compatibility for earlier versions, then yes. Also, please add a note to the changelog.

While you can still run bdist_wheel with an older version of setuptools, the resulting .whl may not contain all licenses.

In 81c0bf6 I updated the dependencies (and removed the now obsolete xfails).

In 67d20a4, I added some notes to the changelog.

agronholm · 2022-10-19T15:20:19Z

src/wheel/bdist_wheel.py

-        return files
+            files.add(license_file)
+
+        license_files = getattr(metadata, "license_files", None) or []


Is the attribute entirely absent if the option hasn't been provided?

I believe the attribute is always present if setuptools is used.
Is there any chance bdist_wheel runs with distutils?

I believe the attribute is always present if setuptools is used. Is there any chance bdist_wheel runs with distutils?

bdist_wheel is a setuptools extension so I don't think this could ever happen.

Ok, so I will remove the getattr and use the attribute directly. Thanks Alex.

Done in 7545b8a.

agronholm · 2022-10-19T16:06:50Z

src/wheel/bdist_wheel.py


-        if "license_file" in metadata:
+        license_file = getattr(metadata, "license_file", None)
+        if license_file:


Is this code path ever triggered on any version of setuptools?

Yes, the user can specify license_file instead of license_files (it will already result in a warning from setuptools).

I can think about a unit test to add here... (I haven't done that before because the code path was basically inherited from the existing implementation).

I'm asking because I looked and did not find this attribute on 45.2.0 or 57.0.0. So it would be nice to check if this gets triggered at all or if setuptools just lumps it into license_files on its own.

We can see here that the metadata object has the attribute in the latest version (I am not sure about 45 or 57).

I will add some unit tests with the intention to activate this path, so we can have more certainty about it.

BTW, we can see in the same function that setuptools is already adding license_file to license_files, so maybe wheel don't have to analyse it again...

In 1682602, I added a check in the test suite to ensure the code path with the warning is triggered.

In 5b09d96, I removed that code path completely since it seems to already be handled by setuptools (as observed in https://github.com/pypa/setuptools/blob/v57.0.0/setuptools/dist.py#L583-L599 and https://github.com/pypa/setuptools/blob/2451deee140b043dcb18796eabda370806881035/setuptools/config.py#L523-L527). Setuptools uses a different warning, and depending on the version, a different warning class.

BTW, we can see in the same function that setuptools is already adding license_file to license_files, so maybe wheel don't have to analyse it again...

I concur. I was just looking at the coverage report and I think the license_files property could be eliminated entirely.

Perfect.

I removed this code path.

Please let me know if you prefer to remove entirely the test_licenses_deprecated (this way the wheel test suite is more independent of setuptools deprecation schedule). Tomorrow morning I can have a look at it.

Yeah, we should not be testing setuptools code in the wheel test suite. Chuck it out :)

Done in 16c90f4

agronholm · 2022-10-20T12:53:07Z

Thanks!

abravalheri added 6 commits September 23, 2022 10:31

Test issue 444 (comma-separated license files)

16a4997

Add support for license_files oneliners

c6fd92f

Test license_files for setup.py

999fb16

Add support for license_files in setup.py

bef75a5

Fix lint errors

d6cb528

Add test for license_files using distutils

5ac2aaa

abravalheri added 2 commits September 23, 2022 12:52

Remove tests with distutils

a411503

It does not seem to be possible to run `bdist_wheel` directly with distutils.

Avoid license patterns for test with setup.py

acc2173

abravalheri marked this pull request as ready for review September 26, 2022 17:19

abravalheri added 4 commits October 19, 2022 13:24

Remove fallback to get_option_dict in bdist_wheel

8055d1c

Completely rely on setuptools expanding license_files

9fb0b76

Remove unused imports

819ed28

Add xfail mark related to old version of setuptools

48fe0d6

agronholm requested changes Oct 19, 2022

View reviewed changes

abravalheri and others added 3 commits October 19, 2022 15:38

Apply suggestions from code review

8f6b817

Co-authored-by: Alex Grönholm <alex.gronholm@nextday.fi>

Update dependency on setuptools>57

81c0bf6

Update changelog

67d20a4

agronholm reviewed Oct 19, 2022

View reviewed changes

agronholm approved these changes Oct 19, 2022

View reviewed changes

agronholm reviewed Oct 19, 2022

View reviewed changes

abravalheri added 3 commits October 19, 2022 17:50

Remove unnecessary getattr

7545b8a

Rely on setuptools to handle license_path

da30500

Ensure code path for license_file is triggered

1682602

abravalheri added 2 commits October 19, 2022 18:09

Rely on setuptools for warning about license_files

5b09d96

Remove test dependent on Setuptools deprecation

16c90f4

agronholm merged commit eaa09fc into pypa:main Oct 20, 2022

abravalheri deleted the license_files branch October 20, 2022 13:08

abravalheri mentioned this pull request Oct 27, 2022

Add a public API #472

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `license_files` specified in `setup.py` #466

Add support for `license_files` specified in `setup.py` #466

abravalheri commented Sep 23, 2022

codecov bot commented Sep 23, 2022 •

edited

abravalheri commented Sep 26, 2022 •

edited

agronholm commented Oct 17, 2022

abravalheri commented Oct 19, 2022 •

edited

abravalheri commented Oct 19, 2022

agronholm commented Oct 19, 2022

agronholm left a comment

abravalheri commented Oct 19, 2022

agronholm Oct 19, 2022

abravalheri Oct 19, 2022

agronholm Oct 19, 2022

abravalheri Oct 19, 2022

abravalheri Oct 19, 2022

agronholm Oct 19, 2022

abravalheri Oct 19, 2022

agronholm Oct 19, 2022

abravalheri Oct 19, 2022

abravalheri Oct 19, 2022 •

edited

abravalheri Oct 19, 2022 •

edited

agronholm Oct 19, 2022

abravalheri Oct 19, 2022

agronholm Oct 19, 2022

abravalheri Oct 20, 2022

agronholm commented Oct 20, 2022

Add support for license_files specified in setup.py #466

Add support for license_files specified in setup.py #466

Conversation

abravalheri commented Sep 23, 2022

codecov bot commented Sep 23, 2022 • edited

Codecov Report

abravalheri commented Sep 26, 2022 • edited

agronholm commented Oct 17, 2022

abravalheri commented Oct 19, 2022 • edited

abravalheri commented Oct 19, 2022

agronholm commented Oct 19, 2022

agronholm left a comment

Choose a reason for hiding this comment

abravalheri commented Oct 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri Oct 19, 2022 • edited

Choose a reason for hiding this comment

abravalheri Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agronholm commented Oct 20, 2022

Add support for `license_files` specified in `setup.py` #466

Add support for `license_files` specified in `setup.py` #466

codecov bot commented Sep 23, 2022 •

edited

abravalheri commented Sep 26, 2022 •

edited

abravalheri commented Oct 19, 2022 •

edited

abravalheri Oct 19, 2022 •

edited

abravalheri Oct 19, 2022 •

edited