From 29a00ee65cc27cfc8a7956c3bc585c141a047950 Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 19:16:11 +0100 Subject: [PATCH 01/24] Add regression tests for sdist --- setuptools/tests/test_sdist.py | 115 +++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index c297cea3a7..fd3a890cea 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -6,10 +6,13 @@ import unicodedata import contextlib import io +import tarfile +from inspect import cleandoc from unittest import mock import pytest +from distutils.core import run_setup from setuptools import Command from setuptools._importlib import metadata from setuptools import SetuptoolsDeprecationWarning @@ -19,6 +22,8 @@ from setuptools.tests import fail_on_ascii from .text import Filenames +import jaraco.path + SETUP_ATTRS = { 'name': 'sdist_test', @@ -645,3 +650,113 @@ def test_default_revctrl(): ) res = ep.load() assert hasattr(res, '__iter__') + + +class TestRegressions: + """ + Can be removed/changed if the project decides to change how it handles symlinks + or external files. + """ + + @staticmethod + def files_for_symlink_in_extension_depends(tmp_path, dep_path): + return { + "external": { + "dir": {"file.h": ""}, + }, + "project": { + "setup.py": cleandoc( + f""" + from setuptools import Extension, setup + setup( + name="myproj", + version="42", + ext_modules=[ + Extension( + "hello", sources=["hello.pyx"], + depends=[{dep_path!r}] + ) + ], + ) + """ + ), + "hello.pyx": "", + "MANIFEST.in": "global-include *.h", + }, + } + + @pytest.mark.parametrize( + "dep_path", ("myheaders/dir/file.h", "myheaders/dir/../dir/file.h") + ) + def test_symlink_in_extension_depends(self, monkeypatch, tmp_path, dep_path): + # Given a project with a symlinked dir and a "depends" targeting that dir + files = self.files_for_symlink_in_extension_depends(tmp_path, dep_path) + jaraco.path.build(files, prefix=str(tmp_path)) + + try: + os.symlink(tmp_path / "external", tmp_path / "project/myheaders") + except (OSError, NotImplementedError): + pytest.skip("symlink not supported in OS") + + # When `sdist` runs, there should be no error + members = run_sdist(monkeypatch, tmp_path / "project") + # and the sdist should contain the symlinked files + for expected in ( + "myproj-42/hello.pyx", + "myproj-42/myheaders/dir/file.h", + ): + assert expected in members + + @staticmethod + def files_for_external_path_in_extension_depends(tmp_path, dep_path): + head, _, tail = dep_path.partition("$tmp_path$/") + dep_path = tmp_path / tail if tail else head + + return { + "external": { + "dir": {"file.h": ""}, + }, + "project": { + "setup.py": cleandoc( + f""" + from setuptools import Extension, setup + setup( + name="myproj", + version="42", + ext_modules=[ + Extension( + "hello", sources=["hello.pyx"], + depends=[{str(dep_path)!r}] + ) + ], + ) + """ + ), + "hello.pyx": "", + "MANIFEST.in": "global-include *.h", + }, + } + + @pytest.mark.parametrize( + "dep_path", ("$tmp_path$/external/dir/file.h", "../external/dir/file.h") + ) + def test_external_path_in_extension_depends(self, monkeypatch, tmp_path, dep_path): + # Given a project with a "depends" targeting an external dir + files = self.files_for_external_path_in_extension_depends(tmp_path, dep_path) + jaraco.path.build(files, prefix=str(tmp_path)) + # When `sdist` runs, there should be no error + members = run_sdist(monkeypatch, tmp_path / "project") + # and the sdist should not contain the external file + for name in members: + assert "file.h" not in name + + +def run_sdist(monkeypatch, project): + """Given a project directory, run the sdist and return its contents""" + monkeypatch.chdir(project) + with quiet(): + run_setup("setup.py", ["sdist"]) + + archive = next((project / "dist").glob("*.tar.gz")) + with tarfile.open(str(archive)) as tar: + return set(tar.getnames()) From 23f82af290d1cb1dab28297d7a5230de1e58e244 Mon Sep 17 00:00:00 2001 From: ruro Date: Fri, 4 Aug 2023 02:45:52 +0300 Subject: [PATCH 02/24] add Extension.depends to build_ext.get_source_files --- setuptools/command/build_ext.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 574fa8e8ce..fb7afc59ab 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -261,6 +261,12 @@ def links_to_dynamic(self, ext): pkg = '.'.join(ext._full_name.split('.')[:-1] + ['']) return any(pkg + libname in libnames for libname in ext.libraries) + def get_source_files(self): + filenames = _build_ext.get_source_files(self) + for ext in self.extensions: + filenames.extend(ext.depends) + return filenames + def get_outputs(self) -> List[str]: if self.inplace: return list(self.get_output_mapping().keys()) From 553bb76199528bc7fb43d5ec79c1a9777293afb4 Mon Sep 17 00:00:00 2001 From: ruro Date: Fri, 4 Aug 2023 03:30:54 +0300 Subject: [PATCH 03/24] add tests for Extension sdist handling --- setuptools/tests/test_sdist.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index fd3a890cea..09a5e7156a 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -19,6 +19,7 @@ from setuptools.command.sdist import sdist from setuptools.command.egg_info import manifest_maker from setuptools.dist import Distribution +from setuptools.extension import Extension from setuptools.tests import fail_on_ascii from .text import Filenames @@ -42,6 +43,12 @@ % SETUP_ATTRS ) +EXTENSION = Extension( + name="sdist_test.f", + sources=[os.path.join("sdist_test", "f.c")], + depends=[os.path.join("sdist_test", "f.h")], +) + @contextlib.contextmanager def quiet(): @@ -119,6 +126,10 @@ def source_dir(self, tmpdir): for fname in ['__init__.py', 'a.txt', 'b.txt', 'c.rst']: touch(test_pkg / fname) touch(data_folder / 'e.dat') + # C sources are not included by default, but they will be, + # if an extension module uses them as sources or depends + for fname in ['f.c', 'f.h']: + touch(test_pkg / fname) with tmpdir.as_cwd(): yield @@ -130,6 +141,11 @@ def assert_package_data_in_manifest(self, cmd): assert os.path.join('sdist_test', 'c.rst') not in manifest assert os.path.join('d', 'e.dat') in manifest + def assert_extension_sources_in_manifest(self, cmd): + manifest = cmd.filelist.files + assert os.path.join('sdist_test', 'f.c') in manifest + assert os.path.join('sdist_test', 'f.h') in manifest + def test_package_data_in_sdist(self): """Regression test for pull request #4: ensures that files listed in package_data are included in the manifest even if they're not added to @@ -164,6 +180,24 @@ def test_package_data_and_include_package_data_in_sdist(self): self.assert_package_data_in_manifest(cmd) + def test_extension_sources_in_sdist(self): + """ + Ensure that the files listed in Extension.sources and Extension.depends + are automatically included in the manifest. + """ + setup_attrs = {**SETUP_ATTRS, 'ext_modules': [EXTENSION]} + + dist = Distribution(setup_attrs) + dist.script_name = 'setup.py' + cmd = sdist(dist) + cmd.ensure_finalized() + + with quiet(): + cmd.run() + + self.assert_package_data_in_manifest(cmd) + self.assert_extension_sources_in_manifest(cmd) + def test_custom_build_py(self): """ Ensure projects defining custom build_py don't break From d2ed9a247d0fa89f550a4cc982a2e4dfbeb89c3f Mon Sep 17 00:00:00 2001 From: ruro Date: Fri, 4 Aug 2023 03:56:36 +0300 Subject: [PATCH 04/24] add news fragment for PR --- newsfragments/4000.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4000.bugfix.rst diff --git a/newsfragments/4000.bugfix.rst b/newsfragments/4000.bugfix.rst new file mode 100644 index 0000000000..f43169e5b4 --- /dev/null +++ b/newsfragments/4000.bugfix.rst @@ -0,0 +1 @@ +Fixed an issue where the files listed in ``Extension.depends`` were not included in manifests and sdists -- by :user:`RuRo` From 43d241bc51c35ec847cd07ff9cf294bfa620419a Mon Sep 17 00:00:00 2001 From: ruro Date: Tue, 8 Aug 2023 21:23:44 +0300 Subject: [PATCH 05/24] improve backwards compatibility and warning messages --- setuptools/command/build_ext.py | 54 ++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index fb7afc59ab..75a550ce3c 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -263,8 +263,60 @@ def links_to_dynamic(self, ext): def get_source_files(self): filenames = _build_ext.get_source_files(self) + root = os.path.abspath(self.distribution.src_root or os.curdir) + + def coerce_dep_path(d): + try: + # Normalize the path + # Note: relative paths are relative to the project root! + d = os.path.normpath(d) + is_absolute = os.path.isabs(d) + if is_absolute: + d_abs = d + d_rel = os.path.relpath(d, start=root) + else: + d_abs = os.path.abspath(os.path.join(root, d)) + d_rel = d + + # Check that the path is contained inside the project root + # If not, exclude it from the manifest/source distribution + is_excluded = os.path.commonpath([d_abs, root]) != root + return is_absolute, is_excluded, d_rel + + except ValueError: + # Most likely the root and the dependency path + # refer to different drives (on Windows) + return False, True, d + for ext in self.extensions: - filenames.extend(ext.depends) + for dep in ext.depends: + is_abs, is_exc, dep_rel = coerce_dep_path(dep) + if is_abs: + log.warn( + "An absolute path was specified in Extension.depends:\n" + "\n" + " %s\n" + "\n" + "Absolute paths in Extension.depends are allowed only " + "for backwards compatibility and should be avoided.\n", + dep, + ) + if is_exc: + log.warn( + "A path specified in Extension.depends refers to a " + "file located outside of the project root:\n" + "\n" + " %s\n" + "\n" + "This path will not be automatically included in the " + "source distribution of this project. " + "Extension.depends paths outside the project root are " + "only allowed for backwards compatibility and should " + "be avoided.\n", + dep, + ) + continue + filenames.append(dep_rel) return filenames def get_outputs(self) -> List[str]: From bf0dd49f81afa132a002a314925223fe0d93c4ac Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 10:05:02 +0100 Subject: [PATCH 06/24] Simplify handling of relative ext.depends --- setuptools/command/build_ext.py | 67 +++++++-------------------------- 1 file changed, 13 insertions(+), 54 deletions(-) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 75a550ce3c..07280ed884 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -4,6 +4,7 @@ from importlib.machinery import EXTENSION_SUFFIXES from importlib.util import cache_from_source as _compiled_file_name from typing import Dict, Iterator, List, Tuple +from pathlib import Path from distutils.command.build_ext import build_ext as _du_build_ext from distutils.ccompiler import new_compiler @@ -261,63 +262,21 @@ def links_to_dynamic(self, ext): pkg = '.'.join(ext._full_name.split('.')[:-1] + ['']) return any(pkg + libname in libnames for libname in ext.libraries) - def get_source_files(self): - filenames = _build_ext.get_source_files(self) - root = os.path.abspath(self.distribution.src_root or os.curdir) + def get_source_files(self) -> List[str]: + return [*_build_ext.get_source_files(self), *self._get_internal_depends()] - def coerce_dep_path(d): + def _get_internal_depends(self) -> Iterator[str]: + """Yield ``ext.depends`` that are contained by the project directory""" + project_root = os.path.abspath(self.distribution.src_root or os.curdir) + depends = (dep for ext in self.extensions for dep in ext.depends) + for dep in depends: try: - # Normalize the path - # Note: relative paths are relative to the project root! - d = os.path.normpath(d) - is_absolute = os.path.isabs(d) - if is_absolute: - d_abs = d - d_rel = os.path.relpath(d, start=root) - else: - d_abs = os.path.abspath(os.path.join(root, d)) - d_rel = d - - # Check that the path is contained inside the project root - # If not, exclude it from the manifest/source distribution - is_excluded = os.path.commonpath([d_abs, root]) != root - return is_absolute, is_excluded, d_rel - + path = os.path.abspath(dep) + rel_path = str(Path(path).relative_to(project_root)) + assert ".." not in rel_path # abspath should have taken care of that + yield rel_path.replace(os.sep, "/") # POSIX-style relative paths except ValueError: - # Most likely the root and the dependency path - # refer to different drives (on Windows) - return False, True, d - - for ext in self.extensions: - for dep in ext.depends: - is_abs, is_exc, dep_rel = coerce_dep_path(dep) - if is_abs: - log.warn( - "An absolute path was specified in Extension.depends:\n" - "\n" - " %s\n" - "\n" - "Absolute paths in Extension.depends are allowed only " - "for backwards compatibility and should be avoided.\n", - dep, - ) - if is_exc: - log.warn( - "A path specified in Extension.depends refers to a " - "file located outside of the project root:\n" - "\n" - " %s\n" - "\n" - "This path will not be automatically included in the " - "source distribution of this project. " - "Extension.depends paths outside the project root are " - "only allowed for backwards compatibility and should " - "be avoided.\n", - dep, - ) - continue - filenames.append(dep_rel) - return filenames + log.warn(f"ignoring {dep} for distribution: outside of project dir") def get_outputs(self) -> List[str]: if self.inplace: From bd777f298689696263d52b6319ee67008bec5f16 Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 10:27:26 +0100 Subject: [PATCH 07/24] Update changelog entry --- newsfragments/4000.bugfix.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/newsfragments/4000.bugfix.rst b/newsfragments/4000.bugfix.rst index f43169e5b4..6c34a663b2 100644 --- a/newsfragments/4000.bugfix.rst +++ b/newsfragments/4000.bugfix.rst @@ -1 +1,2 @@ -Fixed an issue where the files listed in ``Extension.depends`` were not included in manifests and sdists -- by :user:`RuRo` +Automatically add files listed in ``Extension.depends`` to sdists, +as long as they are contained in the project directory -- by :user:`RuRo` From bfc84d31babe4746eb9e7e238d9917c5a9cd8252 Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 10:28:38 +0100 Subject: [PATCH 08/24] Change classification of PR from bugfix to feature --- newsfragments/{4000.bugfix.rst => 4000.feature.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename newsfragments/{4000.bugfix.rst => 4000.feature.rst} (100%) diff --git a/newsfragments/4000.bugfix.rst b/newsfragments/4000.feature.rst similarity index 100% rename from newsfragments/4000.bugfix.rst rename to newsfragments/4000.feature.rst From 973f7f45b347efd21b17d0923a43a442da4448c1 Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 16:03:31 +0100 Subject: [PATCH 09/24] Improve logic operations on path handling --- setuptools/command/build_ext.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 07280ed884..7d72202a8c 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -271,10 +271,11 @@ def _get_internal_depends(self) -> Iterator[str]: depends = (dep for ext in self.extensions for dep in ext.depends) for dep in depends: try: - path = os.path.abspath(dep) - rel_path = str(Path(path).relative_to(project_root)) - assert ".." not in rel_path # abspath should have taken care of that - yield rel_path.replace(os.sep, "/") # POSIX-style relative paths + path = os.path.abspath(os.path.join(project_root, dep)) + # if dep is absolute, os.path.join will ignore project_root + rel_path = Path(path).relative_to(project_root) + assert ".." not in rel_path.parts # abspath should take care of that + yield rel_path.as_posix() # POSIX-style relative paths except ValueError: log.warn(f"ignoring {dep} for distribution: outside of project dir") From 87d5e9c51ad8ab0f79e91372cd66b0d36459f98d Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 19:36:58 +0100 Subject: [PATCH 10/24] Use 'resolve' instead of 'abspath' --- setuptools/command/build_ext.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 7d72202a8c..75722120b7 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -267,15 +267,13 @@ def get_source_files(self) -> List[str]: def _get_internal_depends(self) -> Iterator[str]: """Yield ``ext.depends`` that are contained by the project directory""" - project_root = os.path.abspath(self.distribution.src_root or os.curdir) + project_root = Path(self.distribution.src_root or os.curdir).resolve() depends = (dep for ext in self.extensions for dep in ext.depends) for dep in depends: try: - path = os.path.abspath(os.path.join(project_root, dep)) - # if dep is absolute, os.path.join will ignore project_root - rel_path = Path(path).relative_to(project_root) - assert ".." not in rel_path.parts # abspath should take care of that - yield rel_path.as_posix() # POSIX-style relative paths + abs_path = (project_root / dep).resolve() + # if dep is absolute, Path will ignore project_root + yield abs_path.relative_to(project_root).as_posix() except ValueError: log.warn(f"ignoring {dep} for distribution: outside of project dir") From 77a12951c581f9f2e3c5e13ee673a258e7f2299a Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 19:54:45 +0100 Subject: [PATCH 11/24] Add test for auto inclusion of depends in the case of symlinks --- setuptools/tests/test_sdist.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 09a5e7156a..c83d790dbc 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -794,3 +794,29 @@ def run_sdist(monkeypatch, project): archive = next((project / "dist").glob("*.tar.gz")) with tarfile.open(str(archive)) as tar: return set(tar.getnames()) + + +@pytest.mark.parametrize( + "dep_path", ("myheaders/dir/file.h", "myheaders/dir/../dir/file.h") +) +def test_auto_include_symlinked_depends(monkeypatch, tmp_path, dep_path): + """Similar to TestRegressions.test_relative_to_symlink_in_extension_depends + but does not contain a ``MANIFEST.in``. + """ + files = TestRegressions.files_for_symlink_in_extension_depends(tmp_path, dep_path) + files["project"].pop("MANIFEST.in") + jaraco.path.build(files, prefix=str(tmp_path)) + + try: + os.symlink(tmp_path / "external", tmp_path / "project/myheaders") + except (OSError, NotImplementedError): + pytest.skip("symlink not supported in OS") + + # When `sdist` runs, there should be no error + members = run_sdist(monkeypatch, tmp_path / "project") + # and the sdist should contain the symlinked files + for expected in ( + "myproj-42/hello.pyx", + "myproj-42/myheaders/dir/file.h", + ): + assert expected in members From cf2bda6b1992c6662dbd35be16cc8234ec56d91f Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 20:07:26 +0100 Subject: [PATCH 12/24] Allow symlinked Extension.depends to be auto-included by sdist --- setuptools/_path.py | 27 +++++++++++++++++++++++++++ setuptools/command/build_ext.py | 5 ++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/setuptools/_path.py b/setuptools/_path.py index b99d9dadcf..3a737ea8e1 100644 --- a/setuptools/_path.py +++ b/setuptools/_path.py @@ -1,6 +1,7 @@ import os import sys from typing import Union +from pathlib import Path _Path = Union[str, os.PathLike] @@ -11,6 +12,14 @@ def ensure_directory(path): os.makedirs(dirname, exist_ok=True) +def safe_samefile(path1: _Path, path2: _Path) -> bool: + """Similar to os.path.samefile but returns False instead of raising exception""" + try: + return os.path.samefile(path1, path2) + except OSError: + return False + + def same_path(p1: _Path, p2: _Path) -> bool: """Differs from os.path.samefile because it does not require paths to exist. Purely string based (no comparison between i-nodes). @@ -35,3 +44,21 @@ def normpath(filename: _Path) -> str: # See pkg_resources.normalize_path for notes about cygwin file = os.path.abspath(filename) if sys.platform == 'cygwin' else filename return os.path.normcase(os.path.realpath(os.path.normpath(file))) + + +def besteffort_internal_path(root: _Path, file: _Path) -> str: + """Process ``file`` and return an equivalent relative path contained into root + (POSIX style, with no ``..`` segments). + It may raise an ``ValueError`` if that is not possible. + If ``file`` is not absolute, it will be assumed to be relative to ``root``. + """ + path = os.path.join(root, file) # will ignore root if file is absolute + resolved = Path(path).resolve() + logical = Path(os.path.abspath(path)) + + # Prefer logical paths, since a parent directory can be symlinked inside root + if same_path(resolved, logical) or safe_samefile(resolved, logical): + return logical.relative_to(root).as_posix() + + # Since ``resolved`` is used, it makes sense to resolve root too. + return resolved.relative_to(Path(root).resolve()).as_posix() diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 75722120b7..5307d7abb8 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -11,6 +11,7 @@ from distutils.sysconfig import customize_compiler, get_config_var from distutils import log +from setuptools import _path from setuptools.errors import BaseError from setuptools.extension import Extension, Library @@ -271,9 +272,7 @@ def _get_internal_depends(self) -> Iterator[str]: depends = (dep for ext in self.extensions for dep in ext.depends) for dep in depends: try: - abs_path = (project_root / dep).resolve() - # if dep is absolute, Path will ignore project_root - yield abs_path.relative_to(project_root).as_posix() + yield _path.besteffort_internal_path(project_root, dep) except ValueError: log.warn(f"ignoring {dep} for distribution: outside of project dir") From 379cbe32577a1f068f22ddada6aa335ff37484fe Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Thu, 10 Aug 2023 20:35:34 +0100 Subject: [PATCH 13/24] Add test case for when dependency files don't exit --- setuptools/tests/test_sdist.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index c83d790dbc..171aacb544 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -198,6 +198,27 @@ def test_extension_sources_in_sdist(self): self.assert_package_data_in_manifest(cmd) self.assert_extension_sources_in_manifest(cmd) + def test_missing_extension_sources(self): + """ + Similar to test_extension_sources_in_sdist but the referenced files don't exist. + Missing files should not be included in distribution (with no error raised). + """ + os.remove(os.path.join("sdist_test", "f.h")) + setup_attrs = {**SETUP_ATTRS, 'ext_modules': [EXTENSION]} + + dist = Distribution(setup_attrs) + dist.script_name = 'setup.py' + cmd = sdist(dist) + cmd.ensure_finalized() + + with quiet(): + cmd.run() + + self.assert_package_data_in_manifest(cmd) + manifest = cmd.filelist.files + assert os.path.join('sdist_test', 'f.c') in manifest + assert os.path.join('sdist_test', 'f.h') not in manifest + def test_custom_build_py(self): """ Ensure projects defining custom build_py don't break From 2d5670ab0f80e55b0abfcf36e6b4d898d3219ff5 Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 17:55:04 +0300 Subject: [PATCH 14/24] remove test_auto_include_symlinked_depends --- setuptools/tests/test_sdist.py | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 171aacb544..0a6af1e7d1 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -815,29 +815,3 @@ def run_sdist(monkeypatch, project): archive = next((project / "dist").glob("*.tar.gz")) with tarfile.open(str(archive)) as tar: return set(tar.getnames()) - - -@pytest.mark.parametrize( - "dep_path", ("myheaders/dir/file.h", "myheaders/dir/../dir/file.h") -) -def test_auto_include_symlinked_depends(monkeypatch, tmp_path, dep_path): - """Similar to TestRegressions.test_relative_to_symlink_in_extension_depends - but does not contain a ``MANIFEST.in``. - """ - files = TestRegressions.files_for_symlink_in_extension_depends(tmp_path, dep_path) - files["project"].pop("MANIFEST.in") - jaraco.path.build(files, prefix=str(tmp_path)) - - try: - os.symlink(tmp_path / "external", tmp_path / "project/myheaders") - except (OSError, NotImplementedError): - pytest.skip("symlink not supported in OS") - - # When `sdist` runs, there should be no error - members = run_sdist(monkeypatch, tmp_path / "project") - # and the sdist should contain the symlinked files - for expected in ( - "myproj-42/hello.pyx", - "myproj-42/myheaders/dir/file.h", - ): - assert expected in members From 32a776ebabd9ca416886562e35c27015c0a2fd74 Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 17:57:42 +0300 Subject: [PATCH 15/24] reimplement _get_internal_depends according to the new logic --- setuptools/command/build_ext.py | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 5307d7abb8..9b661a9374 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -11,7 +11,6 @@ from distutils.sysconfig import customize_compiler, get_config_var from distutils import log -from setuptools import _path from setuptools.errors import BaseError from setuptools.extension import Extension, Library @@ -270,11 +269,37 @@ def _get_internal_depends(self) -> Iterator[str]: """Yield ``ext.depends`` that are contained by the project directory""" project_root = Path(self.distribution.src_root or os.curdir).resolve() depends = (dep for ext in self.extensions for dep in ext.depends) + + def skip(orig_path: str, reason: str) -> None: + log.info( + "dependency %s won't be automatically " + "included in the manifest: the path %s", + orig_path, + reason, + ) + for dep in depends: + path = Path(dep) + + if path.is_absolute(): + skip(dep, "must be relative") + continue + + if ".." in path.parts: + skip(dep, "can't have `..` segments") + continue + try: - yield _path.besteffort_internal_path(project_root, dep) - except ValueError: - log.warn(f"ignoring {dep} for distribution: outside of project dir") + resolved = (project_root / path).resolve(strict=True) + except OSError: + skip(dep, "doesn't exist") + continue + + if not resolved.is_relative_to(project_root): + skip(dep, "must be inside the project root") + continue + + yield path.as_posix() def get_outputs(self) -> List[str]: if self.inplace: From 32cf0322e0e69c6ea19d64571f1ace235808143c Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 17:55:31 +0300 Subject: [PATCH 16/24] remove no longer used functions in _path --- setuptools/_path.py | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/setuptools/_path.py b/setuptools/_path.py index 3a737ea8e1..b99d9dadcf 100644 --- a/setuptools/_path.py +++ b/setuptools/_path.py @@ -1,7 +1,6 @@ import os import sys from typing import Union -from pathlib import Path _Path = Union[str, os.PathLike] @@ -12,14 +11,6 @@ def ensure_directory(path): os.makedirs(dirname, exist_ok=True) -def safe_samefile(path1: _Path, path2: _Path) -> bool: - """Similar to os.path.samefile but returns False instead of raising exception""" - try: - return os.path.samefile(path1, path2) - except OSError: - return False - - def same_path(p1: _Path, p2: _Path) -> bool: """Differs from os.path.samefile because it does not require paths to exist. Purely string based (no comparison between i-nodes). @@ -44,21 +35,3 @@ def normpath(filename: _Path) -> str: # See pkg_resources.normalize_path for notes about cygwin file = os.path.abspath(filename) if sys.platform == 'cygwin' else filename return os.path.normcase(os.path.realpath(os.path.normpath(file))) - - -def besteffort_internal_path(root: _Path, file: _Path) -> str: - """Process ``file`` and return an equivalent relative path contained into root - (POSIX style, with no ``..`` segments). - It may raise an ``ValueError`` if that is not possible. - If ``file`` is not absolute, it will be assumed to be relative to ``root``. - """ - path = os.path.join(root, file) # will ignore root if file is absolute - resolved = Path(path).resolve() - logical = Path(os.path.abspath(path)) - - # Prefer logical paths, since a parent directory can be symlinked inside root - if same_path(resolved, logical) or safe_samefile(resolved, logical): - return logical.relative_to(root).as_posix() - - # Since ``resolved`` is used, it makes sense to resolve root too. - return resolved.relative_to(Path(root).resolve()).as_posix() From 476a86f8271eb4126a6858be4d13083c2e7dfca4 Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 16:08:06 +0300 Subject: [PATCH 17/24] refactor the tests a bit --- setuptools/tests/test_sdist.py | 53 +++++++++++++++------------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 0a6af1e7d1..694dd2633c 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -48,6 +48,7 @@ sources=[os.path.join("sdist_test", "f.c")], depends=[os.path.join("sdist_test", "f.h")], ) +EXTENSION_SOURCES = EXTENSION.sources + EXTENSION.depends @contextlib.contextmanager @@ -128,8 +129,8 @@ def source_dir(self, tmpdir): touch(data_folder / 'e.dat') # C sources are not included by default, but they will be, # if an extension module uses them as sources or depends - for fname in ['f.c', 'f.h']: - touch(test_pkg / fname) + for fname in EXTENSION_SOURCES: + touch(tmpdir / fname) with tmpdir.as_cwd(): yield @@ -141,10 +142,18 @@ def assert_package_data_in_manifest(self, cmd): assert os.path.join('sdist_test', 'c.rst') not in manifest assert os.path.join('d', 'e.dat') in manifest - def assert_extension_sources_in_manifest(self, cmd): - manifest = cmd.filelist.files - assert os.path.join('sdist_test', 'f.c') in manifest - assert os.path.join('sdist_test', 'f.h') in manifest + def setup_with_extension(self): + setup_attrs = {**SETUP_ATTRS, 'ext_modules': [EXTENSION]} + + dist = Distribution(setup_attrs) + dist.script_name = 'setup.py' + cmd = sdist(dist) + cmd.ensure_finalized() + + with quiet(): + cmd.run() + + return cmd def test_package_data_in_sdist(self): """Regression test for pull request #4: ensures that files listed in @@ -185,39 +194,25 @@ def test_extension_sources_in_sdist(self): Ensure that the files listed in Extension.sources and Extension.depends are automatically included in the manifest. """ - setup_attrs = {**SETUP_ATTRS, 'ext_modules': [EXTENSION]} - - dist = Distribution(setup_attrs) - dist.script_name = 'setup.py' - cmd = sdist(dist) - cmd.ensure_finalized() - - with quiet(): - cmd.run() - + cmd = self.setup_with_extension() self.assert_package_data_in_manifest(cmd) - self.assert_extension_sources_in_manifest(cmd) + manifest = cmd.filelist.files + for path in EXTENSION_SOURCES: + assert path in manifest def test_missing_extension_sources(self): """ Similar to test_extension_sources_in_sdist but the referenced files don't exist. Missing files should not be included in distribution (with no error raised). """ - os.remove(os.path.join("sdist_test", "f.h")) - setup_attrs = {**SETUP_ATTRS, 'ext_modules': [EXTENSION]} - - dist = Distribution(setup_attrs) - dist.script_name = 'setup.py' - cmd = sdist(dist) - cmd.ensure_finalized() - - with quiet(): - cmd.run() + for path in EXTENSION_SOURCES: + os.remove(path) + cmd = self.setup_with_extension() self.assert_package_data_in_manifest(cmd) manifest = cmd.filelist.files - assert os.path.join('sdist_test', 'f.c') in manifest - assert os.path.join('sdist_test', 'f.h') not in manifest + for path in EXTENSION_SOURCES: + assert path not in manifest def test_custom_build_py(self): """ From 16244b1137621a1999e202b4b2e1819b44fc12eb Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 17:30:46 +0300 Subject: [PATCH 18/24] move test project root to subdir, use source_dir fixture --- setuptools/tests/test_sdist.py | 37 ++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 694dd2633c..05672c200a 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -115,6 +115,9 @@ def touch(path): class TestSdistTest: @pytest.fixture(autouse=True) def source_dir(self, tmpdir): + tmpdir = tmpdir / "project_root" + tmpdir.mkdir() + (tmpdir / 'setup.py').write_text(SETUP_PY, encoding='utf-8') # Set up the rest of the test package @@ -133,7 +136,7 @@ def source_dir(self, tmpdir): touch(tmpdir / fname) with tmpdir.as_cwd(): - yield + yield tmpdir def assert_package_data_in_manifest(self, cmd): manifest = cmd.filelist.files @@ -293,14 +296,14 @@ def test_setup_py_excluded(self): manifest = cmd.filelist.files assert 'setup.py' not in manifest - def test_defaults_case_sensitivity(self, tmpdir): + def test_defaults_case_sensitivity(self, source_dir): """ Make sure default files (README.*, etc.) are added in a case-sensitive way to avoid problems with packages built on Windows. """ - touch(tmpdir / 'readme.rst') - touch(tmpdir / 'SETUP.cfg') + touch(source_dir / 'readme.rst') + touch(source_dir / 'SETUP.cfg') dist = Distribution(SETUP_ATTRS) # the extension deliberately capitalized for this test @@ -582,15 +585,15 @@ def test_sdist_with_latin1_encoded_filename(self): } @pytest.mark.parametrize("config", _EXAMPLE_DIRECTIVES.keys()) - def test_add_files_referenced_by_config_directives(self, tmp_path, config): + def test_add_files_referenced_by_config_directives(self, source_dir, config): config_file, _, _ = config.partition(" - ") config_text = self._EXAMPLE_DIRECTIVES[config] - (tmp_path / 'src').mkdir() - (tmp_path / 'src/VERSION.txt').write_text("0.42", encoding="utf-8") - (tmp_path / 'README.rst').write_text("hello world!", encoding="utf-8") - (tmp_path / 'USAGE.rst').write_text("hello world!", encoding="utf-8") - (tmp_path / 'DOWHATYOUWANT').write_text("hello world!", encoding="utf-8") - (tmp_path / config_file).write_text(config_text, encoding="utf-8") + (source_dir / 'src').mkdir() + (source_dir / 'src/VERSION.txt').write_text("0.42", encoding="utf-8") + (source_dir / 'README.rst').write_text("hello world!", encoding="utf-8") + (source_dir / 'USAGE.rst').write_text("hello world!", encoding="utf-8") + (source_dir / 'DOWHATYOUWANT').write_text("hello world!", encoding="utf-8") + (source_dir / config_file).write_text(config_text, encoding="utf-8") dist = Distribution({"packages": []}) dist.script_name = 'setup.py' @@ -610,11 +613,11 @@ def test_add_files_referenced_by_config_directives(self, tmp_path, config): assert '/' not in cmd.filelist.files assert '\\' not in cmd.filelist.files - def test_pyproject_toml_in_sdist(self, tmpdir): + def test_pyproject_toml_in_sdist(self, source_dir): """ Check if pyproject.toml is included in source distribution if present """ - touch(tmpdir / 'pyproject.toml') + touch(source_dir / 'pyproject.toml') dist = Distribution(SETUP_ATTRS) dist.script_name = 'setup.py' cmd = sdist(dist) @@ -624,11 +627,11 @@ def test_pyproject_toml_in_sdist(self, tmpdir): manifest = cmd.filelist.files assert 'pyproject.toml' in manifest - def test_pyproject_toml_excluded(self, tmpdir): + def test_pyproject_toml_excluded(self, source_dir): """ Check that pyproject.toml can excluded even if present """ - touch(tmpdir / 'pyproject.toml') + touch(source_dir / 'pyproject.toml') with open('MANIFEST.in', 'w') as mts: print('exclude pyproject.toml', file=mts) dist = Distribution(SETUP_ATTRS) @@ -640,8 +643,8 @@ def test_pyproject_toml_excluded(self, tmpdir): manifest = cmd.filelist.files assert 'pyproject.toml' not in manifest - def test_build_subcommand_source_files(self, tmpdir): - touch(tmpdir / '.myfile~') + def test_build_subcommand_source_files(self, source_dir): + touch(source_dir / '.myfile~') # Sanity check: without custom commands file list should not be affected dist = Distribution({**SETUP_ATTRS, "script_name": "setup.py"}) From 4129ed7242663eb92f756c13e0158438668eafa6 Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 17:31:10 +0300 Subject: [PATCH 19/24] improve the touch helper function --- setuptools/tests/test_sdist.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 05672c200a..c3245e8c69 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -8,6 +8,7 @@ import io import tarfile from inspect import cleandoc +from pathlib import Path from unittest import mock import pytest @@ -109,7 +110,10 @@ def latin1_fail(): def touch(path): + if isinstance(path, str): + path = Path(path) path.write_text('', encoding='utf-8') + return path class TestSdistTest: From 7414cc93182a0a3a8aa8a85da7205a368fde8a70 Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 16:09:07 +0300 Subject: [PATCH 20/24] add test_symlinked_extension_sources --- setuptools/tests/test_sdist.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index c3245e8c69..586c667f31 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -116,6 +116,14 @@ def touch(path): return path +def symlink_or_skip_test(src, dst): + try: + os.symlink(src, dst) + return dst + except (OSError, NotImplementedError): + pytest.skip("symlink not supported in OS") + + class TestSdistTest: @pytest.fixture(autouse=True) def source_dir(self, tmpdir): @@ -221,6 +229,29 @@ def test_missing_extension_sources(self): for path in EXTENSION_SOURCES: assert path not in manifest + def test_symlinked_extension_sources(self): + """ + Similar to test_extension_sources_in_sdist but the referenced files are + instead symbolic links to project-local files. Referenced file paths + should be included. Symlink targets themselves should NOT be included. + """ + symlinked = [] + for path in EXTENSION_SOURCES: + base, ext = os.path.splitext(path) + target = base + "_target." + ext + + os.rename(path, target) + symlink_or_skip_test(os.path.basename(target), path) + symlinked.append(target) + + cmd = self.setup_with_extension() + self.assert_package_data_in_manifest(cmd) + manifest = cmd.filelist.files + for path in EXTENSION_SOURCES: + assert path in manifest + for path in symlinked: + assert path not in manifest + def test_custom_build_py(self): """ Ensure projects defining custom build_py don't break From 80b03053f2aa564cd9e43d4208df6c8715b32bb8 Mon Sep 17 00:00:00 2001 From: ruro Date: Sat, 12 Aug 2023 16:10:01 +0300 Subject: [PATCH 21/24] add test_invalid_extension_depends --- setuptools/tests/test_sdist.py | 63 ++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 586c667f31..807183701f 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -7,6 +7,7 @@ import contextlib import io import tarfile +import logging from inspect import cleandoc from pathlib import Path from unittest import mock @@ -252,6 +253,68 @@ def test_symlinked_extension_sources(self): for path in symlinked: assert path not in manifest + _INVALID_PATHS = { + "must be relative": lambda: ( + os.path.abspath(os.path.join("sdist_test", "f.h")) + ), + "can't have `..` segments": lambda: ( + os.path.join("sdist_test", "..", "sdist_test", "f.h") + ), + "doesn't exist": lambda: ( + os.path.join("sdist_test", "this_file_does_not_exist.h") + ), + "must be inside the project root": lambda: ( + symlink_or_skip_test( + touch(os.path.join("..", "outside_of_project_root.h")), + "symlink.h", + ) + ), + } + + @pytest.mark.parametrize("reason", _INVALID_PATHS.keys()) + def test_invalid_extension_depends(self, reason, caplog): + """ + Due to backwards compatibility reasons, `Extension.depends` should accept + invalid/weird paths, but then ignore them when building a sdist. + + This test verifies that the source distribution is still built + successfully with such paths, but that instead of adding these paths to + the manifest, we emit an informational message, notifying the user that + the invalid path won't be automatically included. + """ + invalid_path = self._INVALID_PATHS[reason]() + extension = Extension( + name="sdist_test.f", + sources=[], + depends=[invalid_path], + ) + setup_attrs = {**SETUP_ATTRS, 'ext_modules': [extension]} + + dist = Distribution(setup_attrs) + dist.script_name = 'setup.py' + cmd = sdist(dist) + cmd.ensure_finalized() + + with quiet(), caplog.at_level(logging.INFO): + cmd.run() + + self.assert_package_data_in_manifest(cmd) + manifest = cmd.filelist.files + assert invalid_path not in manifest + + expected_message = [ + message + for (logger, level, message) in caplog.record_tuples + if ( + logger == "root" # + and level == logging.INFO # + and invalid_path in message # + ) + ] + assert len(expected_message) == 1 + (expected_message,) = expected_message + assert reason in expected_message + def test_custom_build_py(self): """ Ensure projects defining custom build_py don't break From 05f7205f227d3e2244c48a53423376c4dd11cf71 Mon Sep 17 00:00:00 2001 From: ruro Date: Mon, 14 Aug 2023 16:51:15 +0300 Subject: [PATCH 22/24] inline is_relative_to (it's not available in Python 3.8) Co-authored-by: Anderson Bravalheri --- setuptools/command/build_ext.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/setuptools/command/build_ext.py b/setuptools/command/build_ext.py index 9b661a9374..9a80781cf4 100644 --- a/setuptools/command/build_ext.py +++ b/setuptools/command/build_ext.py @@ -295,7 +295,9 @@ def skip(orig_path: str, reason: str) -> None: skip(dep, "doesn't exist") continue - if not resolved.is_relative_to(project_root): + try: + resolved.relative_to(project_root) + except ValueError: skip(dep, "must be inside the project root") continue From 3463e0f3500e081343a8bdab8c510edbe6b98fbf Mon Sep 17 00:00:00 2001 From: ruro Date: Mon, 14 Aug 2023 18:50:04 +0300 Subject: [PATCH 23/24] don't run logging verification tests under stdlib distutils --- setuptools/tests/test_sdist.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 807183701f..380afca69a 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -8,6 +8,7 @@ import io import tarfile import logging +import distutils from inspect import cleandoc from pathlib import Path from unittest import mock @@ -108,6 +109,10 @@ def latin1_fail(): "os.environ.get('PYTEST_XDIST_WORKER')", reason="pytest-dev/pytest-xdist#843", ) +skip_under_stdlib_distutils = pytest.mark.skipif( + not distutils.__package__.startswith('setuptools'), + reason="the test is not supported with stdlib distutils", +) def touch(path): @@ -271,6 +276,7 @@ def test_symlinked_extension_sources(self): ), } + @skip_under_stdlib_distutils @pytest.mark.parametrize("reason", _INVALID_PATHS.keys()) def test_invalid_extension_depends(self, reason, caplog): """ From 5c453aa45d8457d8e1a6da767777ae97f4110688 Mon Sep 17 00:00:00 2001 From: Anderson Bravalheri Date: Tue, 15 Aug 2023 16:27:17 +0100 Subject: [PATCH 24/24] Refactor TestRegressions to use symlink_or_skip_test --- setuptools/tests/test_sdist.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/setuptools/tests/test_sdist.py b/setuptools/tests/test_sdist.py index 380afca69a..2cd7482792 100644 --- a/setuptools/tests/test_sdist.py +++ b/setuptools/tests/test_sdist.py @@ -849,11 +849,7 @@ def test_symlink_in_extension_depends(self, monkeypatch, tmp_path, dep_path): # Given a project with a symlinked dir and a "depends" targeting that dir files = self.files_for_symlink_in_extension_depends(tmp_path, dep_path) jaraco.path.build(files, prefix=str(tmp_path)) - - try: - os.symlink(tmp_path / "external", tmp_path / "project/myheaders") - except (OSError, NotImplementedError): - pytest.skip("symlink not supported in OS") + symlink_or_skip_test(tmp_path / "external", tmp_path / "project/myheaders") # When `sdist` runs, there should be no error members = run_sdist(monkeypatch, tmp_path / "project")