Skip to content

Commit

Permalink
Warn about deprecation of behaviour that considers modules/packages a…
Browse files Browse the repository at this point in the history
…s data when include_package_data=True (#3308)
  • Loading branch information
abravalheri committed May 16, 2022
2 parents aec2215 + 2b21892 commit e66ee62
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 3 deletions.
8 changes: 8 additions & 0 deletions changelog.d/3308.deprecation.rst
@@ -0,0 +1,8 @@
Relying on ``include_package_data`` to ensure sub-packages are automatically
added to the build wheel distribution (as "data") is now considered a
deprecated practice.

This behaviour was controversial and caused inconsistencies (#3260).

Instead, projects are encouraged to properly configure ``packages`` or use
discovery tools. General information can be found in :doc:`userguide/package_discovery`.
2 changes: 1 addition & 1 deletion docs/userguide/quickstart.rst
Expand Up @@ -131,7 +131,7 @@ the packages in your project directory:
[options]
packages = find: # OR `find_namespaces:` if you want to use namespaces
[options.packages.find] (always `find` even if `find_namespaces:` was used before)
[options.packages.find] # (always `find` even if `find_namespaces:` was used before)
# This section is optional
# Each entry in this section is optional, and if not specified, the default values are:
# `where=.`, `include=*` and `exclude=` (empty).
Expand Down
54 changes: 52 additions & 2 deletions setuptools/command/build_py.py
Expand Up @@ -9,6 +9,9 @@
import distutils.errors
import itertools
import stat
import warnings
from pathlib import Path
from setuptools._deprecation_warning import SetuptoolsDeprecationWarning
from setuptools.extern.more_itertools import unique_everseen


Expand Down Expand Up @@ -130,6 +133,7 @@ def analyze_manifest(self):
src_dirs[assert_relative(self.get_package_dir(package))] = package

self.run_command('egg_info')
check = _IncludePackageDataAbuse()
ei_cmd = self.get_finalized_command('egg_info')
for path in ei_cmd.filelist.files:
d, f = os.path.split(assert_relative(path))
Expand All @@ -140,8 +144,13 @@ def analyze_manifest(self):
d, df = os.path.split(d)
f = os.path.join(df, f)
if d in src_dirs:
if path.endswith('.py') and f == oldf:
continue # it's a module, not data
if f == oldf:
if check.is_module(f):
continue # it's a module, not data
else:
importable = check.importable_subpackage(src_dirs[d], f)
if importable:
check.warn(importable)
mf.setdefault(src_dirs[d], []).append(path)

def get_data_files(self):
Expand Down Expand Up @@ -241,3 +250,44 @@ def assert_relative(path):
% path
)
raise DistutilsSetupError(msg)


class _IncludePackageDataAbuse:
"""Inform users that package or module is included as 'data file'"""

MESSAGE = """\
!!\n\n

This comment has been minimized.

Copy link
@mgorny

mgorny May 18, 2022

Contributor

Could you please add a one-line short summary here? @gentoo is trying to capture warning messages from setuptools and right now we just get !! here. I don't think we can cleanly capture multi-line warnings, so at least grabbing a meaningful summary would be good.

This comment has been minimized.

Copy link
@abravalheri

abravalheri May 18, 2022

Author Contributor

Hi @mgorny thanks for the suggestion.

That is a tricky one. I would like to keep the highlight this kind of formatting provides. So far I received a lot of feedback, so it means that people are actually noticing these warnings instead of ignoring setuptools (which I believe is the case for most of the existing messages).

Let's see what we can do. Do you have a suggestion for the content of this one-liner?

This comment has been minimized.

Copy link
@mgorny

mgorny May 18, 2022

Contributor

How about something like:

Installing {importable!r} as data subdirectory is deprecated. Please ensure that it is explicitly recognized as a package instead.

That said:

  1. Do I understand correctly that with implicit namespaces every directory is "an importable package", or are there some exceptions? As you've seen I had some trouble understanding what the long message is asking me to do, so perhaps we could make it a bit clearer too.
  2. As for making warnings recognized more often, one trick we've found very helpful in Gentoo is to repeat them at the very end of the build process — so they're not interspersed with the verbose build output. We're also prefixing them with colored asterisks. It looks like this:
    qanotice

This comment has been minimized.

Copy link
@abravalheri

abravalheri May 18, 2022

Author Contributor

Thank you very much @mgorny.

The suggestion is a bit too long and will likely break linters and the display on some users terminal.

This task of coming out of a one liner for this warning is definitely not trivial 😅. I will leave it in the backburner for a while, until we can think of a nice/concise message. (I gave it a try in #3328)

The suggestion for the warnings is a nice one. We probably need to wait until distutils is fully absorbed, to overhaul the issuing of warnings.

This comment has been minimized.

Copy link
@abravalheri

abravalheri May 18, 2022

Author Contributor

Do I understand correctly that with implicit namespaces every directory is "an importable package", or are there some exceptions? As you've seen I had some trouble understanding what the long message is asking me to do, so perhaps we could make it a bit clearer too.

All directories whose name is a valid identifier can be used in a import ... or from ... import ... statement in Python. This might be relevant: #3260 (comment)

############################
# Package would be ignored #
############################
Python recognizes {importable!r} as an importable package, however it is
included in the distribution as "data".
This behavior is likely to change in future versions of setuptools (and
therefore is considered deprecated).
Please make sure that {importable!r} is included as a package by using
setuptools' `packages` configuration field or the proper discovery methods.
You can read more about "package discovery" and "data files" on setuptools
documentation page.
\n\n!!
"""

def __init__(self):
self._already_warned = set()

def is_module(self, file):
return file.endswith(".py") and file[:-len(".py")].isidentifier()

def importable_subpackage(self, parent, file):
pkg = Path(file).parent
parts = list(itertools.takewhile(str.isidentifier, pkg.parts))
if parts:
return ".".join([parent, *parts])
return None

def warn(self, importable):
if importable not in self._already_warned:
msg = textwrap.dedent(self.MESSAGE).format(importable=importable)
warnings.warn(msg, SetuptoolsDeprecationWarning, stacklevel=2)
self._already_warned.add(importable)
71 changes: 71 additions & 0 deletions setuptools/tests/test_build_py.py
Expand Up @@ -3,9 +3,14 @@
import shutil

import pytest
import jaraco.path
from path import Path

from setuptools import SetuptoolsDeprecationWarning
from setuptools.dist import Distribution

from .textwrap import DALS


def test_directories_in_package_data_glob(tmpdir_cwd):
"""
Expand Down Expand Up @@ -102,3 +107,69 @@ 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_subpackages(tmp_path):
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()

build_py = dist.get_command_obj("build_py")
msg = r"Python recognizes 'mypkg\.tests' as an importable package"
with pytest.warns(SetuptoolsDeprecationWarning, match=msg):
# TODO: To fix #3260 we need some transition period to deprecate the
# existing behavior of `include_package_data`. After the transition, we
# should remove the warning and fix the behaviour.
build_py.finalize_options()
build_py.run()

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

# Setuptools is configured to ignore `mypkg.tests`, therefore the following
# files/dirs should not be included in the distribution.
for f in [
"mypkg/tests/__init__.py",
"mypkg/tests/test_mypkg.py",
"mypkg/tests/test_file.txt",
"mypkg/tests",
]:
with pytest.raises(AssertionError):
# TODO: Enforce the following assertion once #3260 is fixed
# (remove context manager and the following xfail).
assert not (build_dir / f).exists()

pytest.xfail("#3260")

0 comments on commit e66ee62

Please sign in to comment.