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

Allow changing the versioning scheme for python_distribution first-party dependencies #10977

Merged
merged 6 commits into from
Oct 19, 2020
Merged
Changes from 1 commit
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
40 changes: 31 additions & 9 deletions src/python/pants/backend/python/goals/setup_py.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Should this be python_distribution to match the target type name?

Related: I want to make a change that normalizes dashes in scope names to underscores, so that they behave similarly to the option name itself. (i.e., --sco-pe-na-me and [sco_pe].na_me both work, as well as, [sco-pe].na_me for backwards compat).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it as python_distribution before, but changed it because every single scope I've seen using -, so it seemed confusing to be inconsistent.

That flexibility would be nice. We will want to decide the canonical format used in documentation.

I don't think this is worth changing to python_distribution until we add the new flexibility you describe.


@classmethod
def register_options(cls, register):
super().register_options(register)
register(
"--exact-first-party-dependency-requirements",
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
type=bool,
default=False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should default to ~=, but curious what you all think.

Note that this is an API change. If this weren't 2.0, we would need to set the default to using == and use a deprecation cycle to be able to change the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back to using the prior default of ==, because imo the default should favor correctness over ergonomics. Users can loosen it if they are okay with the risk of less correctness.

Also, we avoid a deprecation cycle.

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."""

Expand Down Expand Up @@ -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])
Expand All @@ -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
)
Expand All @@ -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)
)

Expand Down