From 4250a5fe89f2ae28bfc174de6f47e265e6e0ef4a Mon Sep 17 00:00:00 2001 From: Arun Babu Neelicattu Date: Sat, 4 Jul 2020 03:38:48 +0200 Subject: [PATCH] inspection: use pep517 metadata build This change replaces setup.py explicit execution in favour of pep517 metadata builds. In addition to improving handling of PEP 517 metadata builds, error handling when reading setup files have also been improved. --- poetry/inspection/info.py | 164 ++++++++++++------ tests/conftest.py | 8 +- .../inspection/demo_only_setup/setup.py | 23 --- tests/inspection/test_info.py | 152 +++++++++++++++- tests/puzzle/test_provider.py | 9 +- 5 files changed, 268 insertions(+), 88 deletions(-) delete mode 100644 tests/fixtures/inspection/demo_only_setup/setup.py diff --git a/poetry/inspection/info.py b/poetry/inspection/info.py index c1f37ad6b45..6c588c24a96 100644 --- a/poetry/inspection/info.py +++ b/poetry/inspection/info.py @@ -30,6 +30,17 @@ logger = logging.getLogger(__name__) +PEP517_META_BUILD = """\ +import pep517.build +import pep517.meta + +path='{source}' +system=pep517.build.compat_system(path) +pep517.meta.build(source_dir=path, dest='{dest}', system=system) +""" + +PEP517_META_BUILD_DEPS = ["pep517===0.8.2", "toml==0.10.1"] + class PackageInfoError(ValueError): def __init__(self, path): # type: (Union[Path, str]) -> None @@ -256,17 +267,27 @@ def _from_sdist_file(cls, path): # type: (Path) -> PackageInfo return info.update(new_info) + @staticmethod + def has_setup_files(path): # type: (Path) -> bool + return any((path / f).exists() for f in SetupReader.FILES) + @classmethod - def from_setup_py(cls, path): # type: (Union[str, Path]) -> PackageInfo + def from_setup_files(cls, path): # type: (Path) -> PackageInfo """ - Mechanism to parse package information from a `setup.py` file. This uses the implentation + Mechanism to parse package information from a `setup.[py|cfg]` file. This uses the implementation at `poetry.utils.setup_reader.SetupReader` in order to parse the file. This is not reliable for complex setup files and should only attempted as a fallback. :param path: Path to `setup.py` file - :return: """ - result = SetupReader.read_from_directory(Path(path)) + if not cls.has_setup_files(path): + raise PackageInfoError(path) + + try: + result = SetupReader.read_from_directory(path) + except Exception: + raise PackageInfoError(path) + python_requires = result["python_requires"] if python_requires is None: python_requires = "*" @@ -288,7 +309,7 @@ def from_setup_py(cls, path): # type: (Union[str, Path]) -> PackageInfo requirements = parse_requires(requires) - return cls( + info = cls( name=result.get("name"), version=result.get("version"), summary=result.get("description", ""), @@ -296,6 +317,12 @@ def from_setup_py(cls, path): # type: (Union[str, Path]) -> PackageInfo requires_python=python_requires, ) + if not (info.name and info.version) and not info.requires_dist: + # there is nothing useful here + raise PackageInfoError(path) + + return info + @staticmethod def _find_dist_info(path): # type: (Path) -> Iterator[Path] """ @@ -308,7 +335,7 @@ def _find_dist_info(path): # type: (Path) -> Iterator[Path] # Sometimes pathlib will fail on recursive symbolic links, so we need to workaround it # and use the glob module instead. Note that this does not happen with pathlib2 # so it's safe to use it for Python < 3.4. - directories = glob.iglob(Path(path, pattern).as_posix(), recursive=True) + directories = glob.iglob(path.joinpath(pattern).as_posix(), recursive=True) else: directories = path.glob(pattern) @@ -316,14 +343,12 @@ def _find_dist_info(path): # type: (Path) -> Iterator[Path] yield Path(d) @classmethod - def from_metadata(cls, path): # type: (Union[str, Path]) -> Optional[PackageInfo] + def from_metadata(cls, path): # type: (Path) -> Optional[PackageInfo] """ Helper method to parse package information from an unpacked metadata directory. :param path: The metadata directory to parse information from. """ - path = Path(path) - if path.suffix in {".dist-info", ".egg-info"}: directories = [path] else: @@ -392,10 +417,79 @@ def _get_poetry_package(path): # type: (Path) -> Optional[ProjectPackage] except RuntimeError: pass + @classmethod + def _pep517_metadata(cls, path): # type (Path) -> PackageInfo + """ + Helper method to use PEP-517 library to build and read package metadata. + + :param path: Path to package source to build and read metadata for. + """ + info = None + try: + info = cls.from_setup_files(path) + if info.requires_dist is not None: + return info + except PackageInfoError: + pass + + with temporary_directory() as tmp_dir: + # TODO: cache PEP 517 build environment corresponding to each project venv + venv_dir = Path(tmp_dir) / ".venv" + EnvManager.build_venv(venv_dir.as_posix()) + venv = VirtualEnv(venv_dir, venv_dir) + + dest_dir = Path(tmp_dir) / "dist" + dest_dir.mkdir() + + try: + venv.run( + "python", + "-m", + "pip", + "install", + "--disable-pip-version-check", + "--ignore-installed", + *PEP517_META_BUILD_DEPS + ) + venv.run( + "python", + "-", + input_=PEP517_META_BUILD.format( + source=path.as_posix(), dest=dest_dir.as_posix() + ), + ) + return cls.from_metadata(dest_dir) + except EnvCommandError as e: + # something went wrong while attempting pep517 metadata build + # fallback to egg_info if setup.py available + cls._log("PEP517 build failed: {}".format(e), level="debug") + setup_py = path / "setup.py" + if not setup_py.exists(): + raise PackageInfoError(path) + + cwd = Path.cwd() + os.chdir(path.as_posix()) + try: + venv.run("python", "setup.py", "egg_info") + return cls.from_metadata(path) + except EnvCommandError: + raise PackageInfoError(path) + finally: + os.chdir(cwd.as_posix()) + + if info: + cls._log( + "Falling back to parsed setup.py file for {}".format(path), "debug" + ) + return info + + # if we reach here, everything has failed and all hope is lost + raise PackageInfoError(path) + @classmethod def from_directory( cls, path, allow_build=False - ): # type: (Union[str, Path], bool) -> PackageInfo + ): # type: (Path, bool) -> PackageInfo """ Generate package information from a package source directory. When `allow_build` is enabled and introspection of all available metadata fails, the package is attempted to be build in an isolated @@ -404,57 +498,28 @@ def from_directory( :param path: Path to generate package information from. :param allow_build: If enabled, as a fallback, build the project to gather metadata. """ - path = Path(path) - - current_dir = os.getcwd() - info = cls.from_metadata(path) if info and info.requires_dist is not None: # return only if requirements are discovered return info - setup_py = path.joinpath("setup.py") - project_package = cls._get_poetry_package(path) if project_package: return cls.from_package(project_package) - if not setup_py.exists(): - if not allow_build and info: - # we discovered PkgInfo but no requirements were listed - return info - # this means we cannot do anything else here - raise PackageInfoError(path) - - if not allow_build: - return cls.from_setup_py(path=path) - try: - # TODO: replace with PEP517 - # we need to switch to the correct path in order for egg_info command to work - os.chdir(str(path)) - - # Execute egg_info - cls._execute_setup() - except EnvCommandError: - cls._log( - "Falling back to parsing setup.py file for {}".format(path), "debug" - ) - # egg_info could not be generated, we fallback to ast parser - return cls.from_setup_py(path=path) - else: - info = cls.from_metadata(path) + if not allow_build: + return cls.from_setup_files(path) + return cls._pep517_metadata(path) + except PackageInfoError as e: if info: + # we discovered PkgInfo but no requirements were listed return info - finally: - os.chdir(current_dir) - - # if we reach here, everything has failed and all hope is lost - raise PackageInfoError(path) + raise e @classmethod - def from_sdist(cls, path): # type: (Union[Path, pkginfo.SDist]) -> PackageInfo + def from_sdist(cls, path): # type: (Path) -> PackageInfo """ Gather package information from an sdist file, packed or unpacked. @@ -508,10 +573,3 @@ def from_path(cls, path): # type: (Path) -> PackageInfo return cls.from_bdist(path=path) except PackageInfoError: return cls.from_sdist(path=path) - - @classmethod - def _execute_setup(cls): - with temporary_directory() as tmp_dir: - EnvManager.build_venv(tmp_dir) - venv = VirtualEnv(Path(tmp_dir), Path(tmp_dir)) - venv.run("python", "setup.py", "egg_info") diff --git a/tests/conftest.py b/tests/conftest.py index db5f96250f0..08d4641b47c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,6 +10,7 @@ from poetry.config.config import Config as BaseConfig from poetry.config.dict_config_source import DictConfigSource +from poetry.inspection.info import PackageInfo from poetry.utils._compat import Path from poetry.utils.env import EnvManager from poetry.utils.env import VirtualEnv @@ -79,8 +80,11 @@ def download_mock(mocker): @pytest.fixture(autouse=True) -def execute_setup_mock(mocker): - mocker.patch("poetry.inspection.info.PackageInfo._execute_setup") +def pep517_metadata_mock(mocker): + mocker.patch( + "poetry.inspection.info.PackageInfo._pep517_metadata", + return_value=PackageInfo(name="demo", version="0.1.2"), + ) @pytest.fixture diff --git a/tests/fixtures/inspection/demo_only_setup/setup.py b/tests/fixtures/inspection/demo_only_setup/setup.py deleted file mode 100644 index 24bcd01c29c..00000000000 --- a/tests/fixtures/inspection/demo_only_setup/setup.py +++ /dev/null @@ -1,23 +0,0 @@ -# -*- coding: utf-8 -*- - -from setuptools import setup - - -kwargs = dict( - name="demo", - license="MIT", - version="0.1.0", - description="Demo project.", - author="Sébastien Eustace", - author_email="sebastien@eustace.io", - url="https://github.com/demo/demo", - packages=["my_package"], - install_requires=[ - 'cleo; extra == "foo"', - "pendulum (>=1.4.4)", - 'tomlkit; extra == "bar"', - ], -) - - -setup(**kwargs) diff --git a/tests/inspection/test_info.py b/tests/inspection/test_info.py index 8333397fdfe..d82526943d9 100644 --- a/tests/inspection/test_info.py +++ b/tests/inspection/test_info.py @@ -1,14 +1,26 @@ +from typing import Set + import pytest from poetry.inspection.info import PackageInfo +from poetry.inspection.info import PackageInfoError from poetry.utils._compat import PY35 +from poetry.utils._compat import CalledProcessError from poetry.utils._compat import Path +from poetry.utils._compat import decode +from poetry.utils.env import EnvCommandError +from poetry.utils.env import VirtualEnv FIXTURE_DIR_BASE = Path(__file__).parent.parent / "fixtures" FIXTURE_DIR_INSPECTIONS = FIXTURE_DIR_BASE / "inspection" +@pytest.fixture(autouse=True) +def pep517_metadata_mock(): + pass + + @pytest.fixture def demo_sdist(): # type: () -> Path return FIXTURE_DIR_BASE / "distributions" / "demo-0.1.0.tar.gz" @@ -19,14 +31,78 @@ def demo_wheel(): # type: () -> Path return FIXTURE_DIR_BASE / "distributions" / "demo-0.1.0-py2.py3-none-any.whl" -def demo_check_info(info): # type: (PackageInfo) -> None +@pytest.fixture +def source_dir(tmp_path): # type: (Path) -> Path + yield Path(tmp_path.as_posix()) + + +@pytest.fixture +def demo_setup(source_dir): # type: (Path) -> Path + setup_py = source_dir / "setup.py" + setup_py.write_text( + decode( + "from setuptools import setup; " + 'setup(name="demo", ' + 'version="0.1.0", ' + 'install_requires=["package"])' + ) + ) + yield source_dir + + +@pytest.fixture +def demo_setup_cfg(source_dir): # type: (Path) -> Path + setup_cfg = source_dir / "setup.cfg" + setup_cfg.write_text( + decode( + "\n".join( + [ + "[metadata]", + "name = demo", + "version = 0.1.0", + "[options]", + "install_requires = package", + ] + ) + ) + ) + yield source_dir + + +@pytest.fixture +def demo_setup_complex(source_dir): # type: (Path) -> Path + setup_py = source_dir / "setup.py" + setup_py.write_text( + decode( + "from setuptools import setup; " + 'setup(name="demo", ' + 'version="0.1.0", ' + 'install_requires=[i for i in ["package"]])' + ) + ) + yield source_dir + + +@pytest.fixture +def demo_setup_complex_pep517_legacy(demo_setup_complex): # type: (Path) -> Path + pyproject_toml = demo_setup_complex / "pyproject.toml" + pyproject_toml.write_text( + decode("[build-system]\n" 'requires = ["setuptools", "wheel"]') + ) + yield demo_setup_complex + + +def demo_check_info(info, requires_dist=None): # type: (PackageInfo, Set[str]) -> None assert info.name == "demo" assert info.version == "0.1.0" - assert set(info.requires_dist) == { + assert info.requires_dist + + requires_dist = requires_dist or { 'cleo; extra == "foo"', "pendulum (>=1.4.4)", 'tomlkit; extra == "bar"', } + assert set(info.requires_dist) == requires_dist def test_info_from_sdist(demo_sdist): @@ -57,9 +133,15 @@ def test_info_from_requires_txt(): @pytest.mark.skipif(not PY35, reason="Parsing of setup.py is skipped for Python < 3.5") -def test_info_from_setup_py(): - info = PackageInfo.from_setup_py(FIXTURE_DIR_INSPECTIONS / "demo_only_setup") - demo_check_info(info) +def test_info_from_setup_py(demo_setup): + info = PackageInfo.from_setup_files(demo_setup) + demo_check_info(info, requires_dist={"package"}) + + +@pytest.mark.skipif(not PY35, reason="Parsing of setup.cfg is skipped for Python < 3.5") +def test_info_from_setup_cfg(demo_setup_cfg): + info = PackageInfo.from_setup_files(demo_setup_cfg) + demo_check_info(info, requires_dist={"package"}) def test_info_no_setup_pkg_info_no_deps(): @@ -69,3 +151,63 @@ def test_info_no_setup_pkg_info_no_deps(): assert info.name == "demo" assert info.version == "0.1.0" assert info.requires_dist is None + + +@pytest.mark.skipif(not PY35, reason="Parsing of setup.py is skipped for Python < 3.5") +def test_info_setup_simple(mocker, demo_setup): + spy = mocker.spy(VirtualEnv, "run") + info = PackageInfo.from_directory(demo_setup, allow_build=True) + assert spy.call_count == 0 + demo_check_info(info, requires_dist={"package"}) + + +@pytest.mark.skipif( + PY35, + reason="For projects with setup.py using Python < 3.5 fallback to pep517 build", +) +def test_info_setup_simple_py2(mocker, demo_setup): + spy = mocker.spy(VirtualEnv, "run") + info = PackageInfo.from_directory(demo_setup, allow_build=True) + assert spy.call_count == 2 + demo_check_info(info, requires_dist={"package"}) + + +@pytest.mark.skipif(not PY35, reason="Parsing of setup.cfg is skipped for Python < 3.5") +def test_info_setup_cfg(mocker, demo_setup_cfg): + spy = mocker.spy(VirtualEnv, "run") + info = PackageInfo.from_directory(demo_setup_cfg, allow_build=True) + assert spy.call_count == 0 + demo_check_info(info, requires_dist={"package"}) + + +def test_info_setup_complex(demo_setup_complex): + info = PackageInfo.from_directory(demo_setup_complex, allow_build=True) + demo_check_info(info, requires_dist={"package"}) + + +def test_info_setup_complex_pep517_error(mocker, demo_setup_complex): + mocker.patch( + "poetry.utils.env.VirtualEnv.run", + auto_spec=True, + side_effect=EnvCommandError(CalledProcessError(1, "mock", output="mock")), + ) + + with pytest.raises(PackageInfoError): + PackageInfo.from_directory(demo_setup_complex, allow_build=True) + + +def test_info_setup_complex_pep517_legacy(demo_setup_complex_pep517_legacy): + info = PackageInfo.from_directory( + demo_setup_complex_pep517_legacy, allow_build=True + ) + demo_check_info(info, requires_dist={"package"}) + + +@pytest.mark.skipif(not PY35, reason="Parsing of setup.py is skipped for Python < 3.5") +def test_info_setup_complex_disable_build(mocker, demo_setup_complex): + spy = mocker.spy(VirtualEnv, "run") + info = PackageInfo.from_directory(demo_setup_complex, allow_build=False) + assert spy.call_count == 0 + assert info.name == "demo" + assert info.version == "0.1.0" + assert info.requires_dist is None diff --git a/tests/puzzle/test_provider.py b/tests/puzzle/test_provider.py index c5b0e3bcc07..016f40c9678 100644 --- a/tests/puzzle/test_provider.py +++ b/tests/puzzle/test_provider.py @@ -8,6 +8,7 @@ from poetry.core.packages.directory_dependency import DirectoryDependency from poetry.core.packages.file_dependency import FileDependency from poetry.core.packages.vcs_dependency import VCSDependency +from poetry.inspection.info import PackageInfo from poetry.puzzle.provider import Provider from poetry.repositories.pool import Pool from poetry.repositories.repository import Repository @@ -118,8 +119,8 @@ def test_search_for_vcs_read_setup_with_extras(provider, mocker): def test_search_for_vcs_read_setup_raises_error_if_no_version(provider, mocker): mocker.patch( - "poetry.inspection.info.PackageInfo._execute_setup", - side_effect=EnvCommandError(CalledProcessError(1, "python", output="")), + "poetry.inspection.info.PackageInfo._pep517_metadata", + return_value=PackageInfo(name="demo", version=None), ) dependency = VCSDependency("demo", "git", "https://github.com/demo/no-version.git") @@ -269,9 +270,7 @@ def test_search_for_directory_setup_read_setup_with_extras(provider, mocker): @pytest.mark.skipif(not PY35, reason="AST parsing does not work for Python <3.4") -def test_search_for_directory_setup_read_setup_with_no_dependencies(provider, mocker): - mocker.patch("poetry.utils.env.EnvManager.get", return_value=MockEnv()) - +def test_search_for_directory_setup_read_setup_with_no_dependencies(provider): dependency = DirectoryDependency( "demo", Path(__file__).parent.parent