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_files specified in setup.py #466

Merged
merged 20 commits into from Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 7 additions & 26 deletions src/wheel/bdist_wheel.py
Expand Up @@ -15,7 +15,6 @@
import warnings
from collections import OrderedDict
from email.generator import BytesGenerator, Generator
from glob import iglob
from io import BytesIO
from shutil import rmtree
from sysconfig import get_config_var
Expand Down Expand Up @@ -432,38 +431,20 @@ def _ensure_relative(self, path):

@property
def license_paths(self):
metadata = self.distribution.get_option_dict("metadata")
metadata = self.distribution.metadata
files = set()
patterns = sorted(
{option for option in metadata.get("license_files", ("", ""))[1].split()}
)

if "license_file" in metadata:
license_file = getattr(metadata, "license_file", None)
if license_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 16c90f4

warnings.warn(
'The "license_file" option is deprecated. Use '
'"license_files" instead.',
DeprecationWarning,
)
files.add(metadata["license_file"][1])

if "license_file" not in metadata and "license_files" not in metadata:
patterns = ("LICEN[CS]E*", "COPYING*", "NOTICE*", "AUTHORS*")

for pattern in patterns:
for path in iglob(pattern):
if path.endswith("~"):
log.debug(
f'ignoring license file "{path}" as it looks like a ' f"backup"
)
continue

if path not in files and os.path.isfile(path):
log.info(
f'adding license file "{path}" (matched pattern "{pattern}")'
)
files.add(path)

return files
files.add(license_file)

license_files = getattr(metadata, "license_files", None) or []
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7545b8a.

return sorted({*files, *license_files})

def egg2dist(self, egginfo_path, distinfo_path):
"""Convert an .egg-info directory into a .dist-info directory"""
Expand Down
52 changes: 40 additions & 12 deletions tests/test_bdist_wheel.py
Expand Up @@ -8,6 +8,7 @@
from zipfile import ZipFile

import pytest
from setuptools import __version__ as setuptools_version

from wheel.bdist_wheel import bdist_wheel
from wheel.wheelfile import WheelFile
Expand All @@ -34,21 +35,27 @@
"LICENSE~",
"AUTHORS~",
}


@pytest.fixture
def dummy_dist(tmpdir_factory):
basedir = tmpdir_factory.mktemp("dummy_dist")
basedir.join("setup.py").write(
"""\
SETUPPY_EXAMPLE = """\
from setuptools import setup

setup(
name='dummy_dist',
version='1.0'
version='1.0',
)
"""


def lincense_files_in_old_setuptools(old_version):
abravalheri marked this conversation as resolved.
Show resolved Hide resolved
return pytest.mark.xfail(
tuple(setuptools_version.split(".")) < tuple(old_version.split(".")),
reason=f"setuptools < {old_version} has problems with license_files",
)


@pytest.fixture
def dummy_dist(tmpdir_factory):
basedir = tmpdir_factory.mktemp("dummy_dist")
basedir.join("setup.py").write(SETUPPY_EXAMPLE)
for fname in DEFAULT_LICENSE_FILES | OTHER_IGNORED_FILES:
basedir.join(fname).write("")

Expand All @@ -71,6 +78,7 @@ def test_unicode_record(wheel_paths):
assert "åäö_日本語.py".encode() in record


@lincense_files_in_old_setuptools("57.0")
abravalheri marked this conversation as resolved.
Show resolved Hide resolved
def test_licenses_default(dummy_dist, monkeypatch, tmpdir):
monkeypatch.chdir(dummy_dist)
subprocess.check_call(
Expand All @@ -83,6 +91,7 @@ def test_licenses_default(dummy_dist, monkeypatch, tmpdir):
assert set(wf.namelist()) == DEFAULT_FILES | license_files


@lincense_files_in_old_setuptools("56.0")
abravalheri marked this conversation as resolved.
Show resolved Hide resolved
def test_licenses_deprecated(dummy_dist, monkeypatch, tmpdir):
dummy_dist.join("setup.cfg").write("[metadata]\nlicense_file=licenses/DUMMYFILE")
monkeypatch.chdir(dummy_dist)
Expand All @@ -94,10 +103,29 @@ def test_licenses_deprecated(dummy_dist, monkeypatch, tmpdir):
assert set(wf.namelist()) == DEFAULT_FILES | license_files


def test_licenses_override(dummy_dist, monkeypatch, tmpdir):
dummy_dist.join("setup.cfg").write(
"[metadata]\nlicense_files=licenses/*\n LICENSE"
)
@pytest.mark.parametrize(
"config_file, config",
[
pytest.param(
"setup.cfg",
"[metadata]\nlicense_files=licenses/*\n LICENSE",
marks=lincense_files_in_old_setuptools("57.0"),
abravalheri marked this conversation as resolved.
Show resolved Hide resolved
),
pytest.param(
"setup.cfg",
"[metadata]\nlicense_files=licenses/*, LICENSE",
marks=lincense_files_in_old_setuptools("57.0"),
abravalheri marked this conversation as resolved.
Show resolved Hide resolved
),
(
"setup.py",
SETUPPY_EXAMPLE.replace(
")", " license_files=['licenses/DUMMYFILE', 'LICENSE'])"
),
),
],
)
def test_licenses_override(dummy_dist, monkeypatch, tmpdir, config_file, config):
dummy_dist.join(config_file).write(config)
monkeypatch.chdir(dummy_dist)
subprocess.check_call(
[sys.executable, "setup.py", "bdist_wheel", "-b", str(tmpdir), "--universal"]
Expand Down