From a5f0d5ee5011e1945e52b184d9e62e9773210d93 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 16 Oct 2020 12:36:56 -0700 Subject: [PATCH 1/5] Allow using compatible release versions (`~=`) with `python_distribution` first-party dependencies # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/backend/python/goals/setup_py.py | 40 ++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/src/python/pants/backend/python/goals/setup_py.py b/src/python/pants/backend/python/goals/setup_py.py index 4480719cd15..22907c4743c 100644 --- a/src/python/pants/backend/python/goals/setup_py.py +++ b/src/python/pants/backend/python/goals/setup_py.py @@ -72,6 +72,7 @@ ) from pants.engine.unions import UnionMembership, UnionRule, union from pants.option.custom_types import shell_str +from pants.option.subsystem import Subsystem from pants.python.python_setup import PythonSetup from pants.util.logging import LogLevel from pants.util.memo import memoized_property @@ -308,6 +309,31 @@ class RunSetupPyResult: output: Digest # The state of the chroot after running setup.py. +class PythonDistributionSubsystem(Subsystem): + """Options for packaging wheels/sdists from a `python_distribution` target.""" + + options_scope = "python-distribution" + + @classmethod + def register_options(cls, register): + super().register_options(register) + register( + "--exact-first-party-dependency-requirements", + type=bool, + default=False, + help=( + "If True, dependencies on other `python_distribution`s will use exact requirements " + "with `==`, rather than compatible releases with `~=`. See " + "https://www.python.org/dev/peps/pep-0440/#version-specifiers." + ), + ) + + @property + def first_party_dependencies_version_specifier(self) -> str: + """The version specifier to use for first-party dependencies (either `==` or `~=`).""" + return "==" if self.options.exact_first_party_dependency_requirements else "~=" + + class SetupPySubsystem(GoalSubsystem): """Deprecated in favor of the `package` goal.""" @@ -714,7 +740,9 @@ async def get_sources(request: SetupPySourcesRequest) -> SetupPySources: @rule(desc="Compute distribution's 3rd party requirements") async def get_requirements( - dep_owner: DependencyOwner, union_membership: UnionMembership + dep_owner: DependencyOwner, + union_membership: UnionMembership, + python_distribution_subsystem: PythonDistributionSubsystem, ) -> ExportedTargetRequirements: transitive_targets = await Get( TransitiveTargets, TransitiveTargetsRequest([dep_owner.exported_target.target.address]) @@ -736,13 +764,6 @@ async def get_requirements( # if U is in the owned deps then we'll pick up R through U. And if U is not in the owned deps # then it's owned by an exported target ET, and so R will be in the requirements for ET, and we # will require ET. - # - # TODO: Note that this logic doesn't account for indirection via dep aggregator targets, of type - # `target`. But we don't have those in v2 (yet) anyway. Plus, as we move towards buildgen and/or - # stricter build graph hygiene, it makes sense to require that targets directly declare their - # true dependencies. Plus, in the specific realm of setup-py, since we must exclude indirect - # deps across exported target boundaries, it's not a big stretch to just insist that - # requirements must be direct deps. direct_deps_tgts = await MultiGet( Get(Targets, DependenciesRequest(tgt.get(Dependencies))) for tgt in owned_by_us ) @@ -754,11 +775,12 @@ async def get_requirements( req_strs = list(reqs) # Add the requirements on any exported targets on which we depend. + version_specifier = python_distribution_subsystem.first_party_dependencies_version_specifier kwargs_for_exported_targets_we_depend_on = await MultiGet( Get(SetupKwargs, OwnedDependency(tgt)) for tgt in owned_by_others ) req_strs.extend( - f"{kwargs.name}=={kwargs.version}" + f"{kwargs.name}{version_specifier}{kwargs.version}" for kwargs in set(kwargs_for_exported_targets_we_depend_on) ) From e519b12fc237e8162a2b975524a7f2b985b74c1a Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 16 Oct 2020 12:52:53 -0700 Subject: [PATCH 2/5] Fix tests # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../backend/python/goals/setup_py_test.py | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/python/pants/backend/python/goals/setup_py_test.py b/src/python/pants/backend/python/goals/setup_py_test.py index eee0f8538db..374337fc671 100644 --- a/src/python/pants/backend/python/goals/setup_py_test.py +++ b/src/python/pants/backend/python/goals/setup_py_test.py @@ -16,6 +16,7 @@ NoOwnerError, OwnedDependencies, OwnedDependency, + PythonDistributionSubsystem, SetupKwargs, SetupKwargsRequest, SetupPyChroot, @@ -44,7 +45,7 @@ from pants.engine.addresses import Address from pants.engine.fs import Snapshot from pants.engine.internals.scheduler import ExecutionError -from pants.engine.rules import rule +from pants.engine.rules import SubsystemRule, rule from pants.engine.target import Targets from pants.engine.unions import UnionRule from pants.testutil.rule_runner import QueryRule, RuleRunner @@ -93,6 +94,7 @@ def chroot_rule_runner() -> RuleRunner: get_exporting_owner, *python_sources.rules(), setup_kwargs_plugin, + SubsystemRule(PythonDistributionSubsystem), UnionRule(SetupKwargsRequest, PluginSetupKwargsRequest), QueryRule(SetupPyChroot, (SetupPyChrootRequest,)), ] @@ -213,7 +215,7 @@ def test_generate_chroot(chroot_rule_runner: RuleRunner) -> None: "packages": ("foo", "foo.qux"), "namespace_packages": ("foo",), "package_data": {"foo": ("resources/js/code.js",)}, - "install_requires": ("baz==1.1.1",), + "install_requires": ("baz~=1.1.1",), "entry_points": {"console_scripts": ["foo_main=foo.qux.bin"]}, }, "src/python/foo:foo-dist", @@ -366,6 +368,7 @@ def test_get_requirements() -> None: get_requirements, get_owned_dependencies, get_exporting_owner, + SubsystemRule(PythonDistributionSubsystem), QueryRule(ExportedTargetRequirements, (DependencyOwner,)), ] ) @@ -432,7 +435,11 @@ def test_get_requirements() -> None: ), ) - def assert_requirements(expected_req_strs, addr): + def assert_requirements(expected_req_strs, addr, *, exact_first_party_deps: bool = False): + if exact_first_party_deps: + rule_runner.set_options( + ["--python-distribution-exact-first-party-dependency-requirements"] + ) tgt = rule_runner.get_target(Address.parse(addr)) reqs = rule_runner.request( ExportedTargetRequirements, @@ -441,7 +448,13 @@ def assert_requirements(expected_req_strs, addr): assert sorted(expected_req_strs) == list(reqs) assert_requirements(["ext1==1.22.333", "ext2==4.5.6"], "src/python/foo/bar:bar-dist") - assert_requirements(["ext3==0.0.1", "bar==9.8.7"], "src/python/foo/corge:corge-dist") + assert_requirements(["ext3==0.0.1", "bar~=9.8.7"], "src/python/foo/corge:corge-dist") + + assert_requirements( + ["ext3==0.0.1", "bar==9.8.7"], + "src/python/foo/corge:corge-dist", + exact_first_party_deps=True, + ) def test_owned_dependencies() -> None: From 36637deb1d40ca874c90cb91e5f33f82c001e183 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 16 Oct 2020 13:38:32 -0700 Subject: [PATCH 3/5] Use an enum for a cleaner interface # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/backend/python/goals/setup_py.py | 35 +++++++++++++------ .../backend/python/goals/setup_py_test.py | 26 +++++++++----- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/python/pants/backend/python/goals/setup_py.py b/src/python/pants/backend/python/goals/setup_py.py index 22907c4743c..e9eb3a27f9a 100644 --- a/src/python/pants/backend/python/goals/setup_py.py +++ b/src/python/pants/backend/python/goals/setup_py.py @@ -9,6 +9,7 @@ from abc import ABC, abstractmethod from collections import abc, defaultdict from dataclasses import dataclass +from enum import Enum from typing import Any, Dict, List, Mapping, Set, Tuple, cast from pants.backend.python.macros.python_artifact import PythonArtifact @@ -309,6 +310,12 @@ class RunSetupPyResult: output: Digest # The state of the chroot after running setup.py. +class FirstPartyDependencyVersionScheme(Enum): + EXACT = "exact" # i.e., == + COMPATIBLE = "compatible" # i.e., ~= + ANY = "any" # i.e., no specifier + + class PythonDistributionSubsystem(Subsystem): """Options for packaging wheels/sdists from a `python_distribution` target.""" @@ -318,20 +325,27 @@ class PythonDistributionSubsystem(Subsystem): def register_options(cls, register): super().register_options(register) register( - "--exact-first-party-dependency-requirements", - type=bool, - default=False, + "--first-party-dependency-version-scheme", + type=FirstPartyDependencyVersionScheme, + default=FirstPartyDependencyVersionScheme.COMPATIBLE, help=( - "If True, dependencies on other `python_distribution`s will use exact requirements " - "with `==`, rather than compatible releases with `~=`. See " + "What version to set in `install_requires` when a `python_distribution` depends on " + "other `python_distribution`s. If `exact`, will use `==`. If `compatible`, will " + "use `~=`. If `any`, will leave off the version. See " "https://www.python.org/dev/peps/pep-0440/#version-specifiers." ), ) - @property - def first_party_dependencies_version_specifier(self) -> str: - """The version specifier to use for first-party dependencies (either `==` or `~=`).""" - return "==" if self.options.exact_first_party_dependency_requirements else "~=" + def first_party_dependency_version(self, version: str) -> str: + """Return the version string (e.g. '~=4.0') for a first-party dependency. + + If the user specified to use "any" version, then this will return an empty string. + """ + scheme = self.options.first_party_dependency_version_scheme + if scheme == FirstPartyDependencyVersionScheme.ANY: + return "" + specifier = "==" if scheme == FirstPartyDependencyVersionScheme.EXACT else "~=" + return f"{specifier}{version}" class SetupPySubsystem(GoalSubsystem): @@ -775,12 +789,11 @@ async def get_requirements( req_strs = list(reqs) # Add the requirements on any exported targets on which we depend. - version_specifier = python_distribution_subsystem.first_party_dependencies_version_specifier kwargs_for_exported_targets_we_depend_on = await MultiGet( Get(SetupKwargs, OwnedDependency(tgt)) for tgt in owned_by_others ) req_strs.extend( - f"{kwargs.name}{version_specifier}{kwargs.version}" + f"{kwargs.name}{python_distribution_subsystem.first_party_dependency_version(kwargs.version)}" for kwargs in set(kwargs_for_exported_targets_we_depend_on) ) diff --git a/src/python/pants/backend/python/goals/setup_py_test.py b/src/python/pants/backend/python/goals/setup_py_test.py index 374337fc671..518656bc6ee 100644 --- a/src/python/pants/backend/python/goals/setup_py_test.py +++ b/src/python/pants/backend/python/goals/setup_py_test.py @@ -11,6 +11,7 @@ DependencyOwner, ExportedTarget, ExportedTargetRequirements, + FirstPartyDependencyVersionScheme, InvalidEntryPoint, InvalidSetupPyArgs, NoOwnerError, @@ -435,11 +436,15 @@ def test_get_requirements() -> None: ), ) - def assert_requirements(expected_req_strs, addr, *, exact_first_party_deps: bool = False): - if exact_first_party_deps: - rule_runner.set_options( - ["--python-distribution-exact-first-party-dependency-requirements"] - ) + def assert_requirements( + expected_req_strs, + addr, + *, + version_scheme: FirstPartyDependencyVersionScheme = FirstPartyDependencyVersionScheme.EXACT + ): + rule_runner.set_options( + [f"--python-distribution-first-party-dependency-version-scheme={version_scheme.value}"] + ) tgt = rule_runner.get_target(Address.parse(addr)) reqs = rule_runner.request( ExportedTargetRequirements, @@ -448,12 +453,17 @@ def assert_requirements(expected_req_strs, addr, *, exact_first_party_deps: bool assert sorted(expected_req_strs) == list(reqs) assert_requirements(["ext1==1.22.333", "ext2==4.5.6"], "src/python/foo/bar:bar-dist") - assert_requirements(["ext3==0.0.1", "bar~=9.8.7"], "src/python/foo/corge:corge-dist") + assert_requirements(["ext3==0.0.1", "bar==9.8.7"], "src/python/foo/corge:corge-dist") assert_requirements( - ["ext3==0.0.1", "bar==9.8.7"], + ["ext3==0.0.1", "bar~=9.8.7"], + "src/python/foo/corge:corge-dist", + version_scheme=FirstPartyDependencyVersionScheme.COMPATIBLE, + ) + assert_requirements( + ["ext3==0.0.1", "bar"], "src/python/foo/corge:corge-dist", - exact_first_party_deps=True, + version_scheme=FirstPartyDependencyVersionScheme.ANY, ) From 407e1c8a8d457c078fb14b0afe412e1748160c43 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 16 Oct 2020 13:51:31 -0700 Subject: [PATCH 4/5] Use @enum.unique # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/backend/python/goals/setup_py.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/python/pants/backend/python/goals/setup_py.py b/src/python/pants/backend/python/goals/setup_py.py index e9eb3a27f9a..fd0e964a0bf 100644 --- a/src/python/pants/backend/python/goals/setup_py.py +++ b/src/python/pants/backend/python/goals/setup_py.py @@ -1,6 +1,7 @@ # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). +import enum import io import itertools import logging @@ -9,7 +10,6 @@ from abc import ABC, abstractmethod from collections import abc, defaultdict from dataclasses import dataclass -from enum import Enum from typing import Any, Dict, List, Mapping, Set, Tuple, cast from pants.backend.python.macros.python_artifact import PythonArtifact @@ -310,7 +310,8 @@ class RunSetupPyResult: output: Digest # The state of the chroot after running setup.py. -class FirstPartyDependencyVersionScheme(Enum): +@enum.unique +class FirstPartyDependencyVersionScheme(enum.Enum): EXACT = "exact" # i.e., == COMPATIBLE = "compatible" # i.e., ~= ANY = "any" # i.e., no specifier From 1fa0e480971d9ba9e766f9708069a549256b06b0 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 16 Oct 2020 13:52:32 -0700 Subject: [PATCH 5/5] Go back to defaulting to exact We should probably default to the safest behavior, given that the purpose of a tool like Pants is correctness. This also avoids making a breaking API change. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/backend/python/goals/setup_py.py | 2 +- src/python/pants/backend/python/goals/setup_py_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/python/pants/backend/python/goals/setup_py.py b/src/python/pants/backend/python/goals/setup_py.py index fd0e964a0bf..56ea0fea44a 100644 --- a/src/python/pants/backend/python/goals/setup_py.py +++ b/src/python/pants/backend/python/goals/setup_py.py @@ -328,7 +328,7 @@ def register_options(cls, register): register( "--first-party-dependency-version-scheme", type=FirstPartyDependencyVersionScheme, - default=FirstPartyDependencyVersionScheme.COMPATIBLE, + default=FirstPartyDependencyVersionScheme.EXACT, help=( "What version to set in `install_requires` when a `python_distribution` depends on " "other `python_distribution`s. If `exact`, will use `==`. If `compatible`, will " diff --git a/src/python/pants/backend/python/goals/setup_py_test.py b/src/python/pants/backend/python/goals/setup_py_test.py index 518656bc6ee..53888665bb8 100644 --- a/src/python/pants/backend/python/goals/setup_py_test.py +++ b/src/python/pants/backend/python/goals/setup_py_test.py @@ -216,7 +216,7 @@ def test_generate_chroot(chroot_rule_runner: RuleRunner) -> None: "packages": ("foo", "foo.qux"), "namespace_packages": ("foo",), "package_data": {"foo": ("resources/js/code.js",)}, - "install_requires": ("baz~=1.1.1",), + "install_requires": ("baz==1.1.1",), "entry_points": {"console_scripts": ["foo_main=foo.qux.bin"]}, }, "src/python/foo:foo-dist",