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

Fix bug that prevents packages.find.exclude to take effect when include_package_data=True #3261

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 8 additions & 0 deletions changelog.d/3261.breaking.rst
@@ -0,0 +1,8 @@
Projects that were abusing ``include_package_data=True`` to automatically add
sub-packages and sub-modules will find the built wheel distribution no longer
including some files.

These projects are encouraged to properly configure ``packages`` to include all
sub-packages. More information can be found in :doc:`userguide/package_discovery`.

The previous behaviour was unintentional and caused a bug (#3260).
2 changes: 2 additions & 0 deletions changelog.d/3261.change.rst
@@ -0,0 +1,2 @@
Fixed bug (#3260) that prevented configuration for ``packages.find.exclude`` to take effect
when ``include_package_data = True``.
21 changes: 21 additions & 0 deletions setuptools/command/build_py.py
Expand Up @@ -8,6 +8,7 @@
import distutils.errors
import itertools
import stat
from pathlib import Path
from setuptools.extern.more_itertools import unique_everseen


Expand Down Expand Up @@ -134,10 +135,20 @@ def analyze_manifest(self):
d, f = os.path.split(assert_relative(path))
prev = None
oldf = f

# Climb the hierarchy to find a parent package included in the distribution
while d and d != prev and d not in src_dirs:
if has_valid_modules(d):
# The given directory contains Python modules, and therefore can be
# considered a package (or namespace) itself, not a simple container
# for data files.
# If the intention is to include it in the distribution, then it
# should be added to `packages`, and therefore appear in `src_dirs`.
break
prev = d
d, df = os.path.split(d)
f = os.path.join(df, f)

if d in src_dirs:
if path.endswith('.py') and f == oldf:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side note

This f == oldf condition is a bit odd.
According to the git history (f01c17e), the original intention of this code was to:

Allow .py files found by the include_package_data option to be automatically included.

To me that sounds a little bit as a misuse. My opinion is that modules should not be affected by include_package_data...

Unfortunately, I cannot find the original repository (I guess it was bitbucket) to check if it was associated with a PR or feature request to understand the context.

Copy link
Member

Choose a reason for hiding this comment

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

All of the issues and PRs were migrated from Bitbucket, but that commit comes from when Setuptools was hosted on Sourceforge and Subversion, where issues and merges weren't tracked as easily, so the commit history is the best we have.

continue # it's a module, not data
Expand Down Expand Up @@ -240,3 +251,13 @@ def assert_relative(path):
% path
)
raise DistutilsSetupError(msg)


def has_valid_modules(directory):
return any(
all(
part.isidentifier()
for part in potential_module.relative_to(directory).with_suffix("").parts
)
for potential_module in Path(directory).glob("**/*.py")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I thought about checking against importlib.machinery.all_suffixes, but I decided to go only with .py because I don't know what to do with .so files...

(Maybe the .so is not generated by setuptools, but instead compiled out-of-band and included by the user on purpose?)

)
51 changes: 51 additions & 0 deletions setuptools/tests/test_build_py.py
Expand Up @@ -3,9 +3,13 @@
import shutil

import pytest
import jaraco.path
from path import Path

from setuptools.dist import Distribution

from .textwrap import DALS


def test_directories_in_package_data_glob(tmpdir_cwd):
"""
Expand Down Expand Up @@ -79,3 +83,50 @@ def test_executable_data(tmpdir_cwd):

assert os.stat('build/lib/pkg/run-me').st_mode & stat.S_IEXEC, \
"Script is not executable"


def test_excluded_subpacakges(tmp_path):
abravalheri marked this conversation as resolved.
Show resolved Hide resolved
files = {
"setup.cfg": DALS("""
[metadata]
name = mypkg
version = 42

[options]
include_package_data = True
packages = find:

[options.packages.find]
exclude = *.tests*
"""),
"mypkg": {
"__init__.py": "",
"resource_file.txt": "",
"tests": {
"__init__.py": "",
"test_mypkg.py": "",
"test_file.txt": "",
}
},
"MANIFEST.in": DALS("""
global-include *.py *.txt
global-exclude *.py[cod]
prune dist
prune build
prune *.egg-info
""")
}

with Path(tmp_path):
jaraco.path.build(files)
dist = Distribution({"script_name": "%PEP 517%"})
dist.parse_config_files()
dist.run_command("build_py")
build_dir = Path(dist.get_command_obj("build_py").build_lib)

assert (build_dir / "mypkg/__init__.py").exists()
assert (build_dir / "mypkg/resource_file.txt").exists()
assert not (build_dir / "mypkg/tests/__init__.py").exists()
assert not (build_dir / "mypkg/tests/test_mypkg.py").exists()
assert not (build_dir / "mypkg/tests/test_file.txt").exists()
assert not (build_dir / "mypkg/tests").exists()