From 398bcac680f685d67f4277c7cbd02451e14c5946 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Sun, 12 Mar 2023 15:31:20 -0700 Subject: [PATCH 1/2] Allow a user resolve to silently shadow a tool lockfile of the same name. This is necessary for the transition to named resolves - as the tool name will be an obvious choice for many people for the user resolve. Also, change the name of the synthetic target used for the lockfile. Currently it's squatting on the resolve name, meaning you can't name your python_requirements target for your tool by that name. Now we use a less obtrusive name, that is unlkely to collide with a user-selected one. --- .../pants/backend/python/goals/lockfile.py | 8 +- .../python/macros/common_requirements_rule.py | 5 +- .../pants/core/goals/generate_lockfiles.py | 97 ++++++------------- .../core/goals/generate_lockfiles_test.py | 27 +++--- 4 files changed, 54 insertions(+), 83 deletions(-) diff --git a/src/python/pants/backend/python/goals/lockfile.py b/src/python/pants/backend/python/goals/lockfile.py index 5ad71435dbd..60d1942f6c9 100644 --- a/src/python/pants/backend/python/goals/lockfile.py +++ b/src/python/pants/backend/python/goals/lockfile.py @@ -259,6 +259,10 @@ class PythonSyntheticLockfileTargetsRequest(SyntheticTargetsRequest): path: str = SyntheticTargetsRequest.SINGLE_REQUEST_FOR_ALL_TARGETS +def synthetic_lockfile_target_name(resolve: str) -> str: + return f"_{resolve}_lockfile" + + @rule async def python_lockfile_synthetic_targets( request: PythonSyntheticLockfileTargetsRequest, @@ -277,7 +281,9 @@ async def python_lockfile_synthetic_targets( ( os.path.join(spec_path, "BUILD.python-lockfiles"), tuple( - TargetAdaptor("_lockfiles", name=name, sources=[lockfile]) + TargetAdaptor( + "_lockfiles", name=synthetic_lockfile_target_name(name), sources=[lockfile] + ) for _, lockfile, name in lockfiles ), ) diff --git a/src/python/pants/backend/python/macros/common_requirements_rule.py b/src/python/pants/backend/python/macros/common_requirements_rule.py index e514f06d3e9..1b8fb4d4941 100644 --- a/src/python/pants/backend/python/macros/common_requirements_rule.py +++ b/src/python/pants/backend/python/macros/common_requirements_rule.py @@ -10,6 +10,7 @@ from packaging.utils import canonicalize_name as canonicalize_project_name +from pants.backend.python.goals.lockfile import synthetic_lockfile_target_name from pants.backend.python.macros.common_fields import ( ModuleMappingField, TypeStubsModuleMappingField, @@ -83,7 +84,7 @@ async def _generate_requirements( if lockfile: lockfile_address = Address( os.path.dirname(lockfile), - target_name=resolve, + target_name=synthetic_lockfile_target_name(resolve), ) target_adaptor = await Get( TargetAdaptor, @@ -93,7 +94,7 @@ async def _generate_requirements( ), ) if target_adaptor.type_alias == "_lockfiles": - req_deps.append(f"{lockfile}:{resolve}") + req_deps.append(f"{lockfile}:{synthetic_lockfile_target_name(resolve)}") else: logger.warning( softwrap( diff --git a/src/python/pants/core/goals/generate_lockfiles.py b/src/python/pants/core/goals/generate_lockfiles.py index a3bf4000580..42b88895794 100644 --- a/src/python/pants/core/goals/generate_lockfiles.py +++ b/src/python/pants/core/goals/generate_lockfiles.py @@ -272,9 +272,9 @@ def output_changed(self, title: str, reqs: ChangedPackages) -> Iterator[str]: def get_bump_attrs(self, prev: PackageVersion, curr: PackageVersion) -> dict[str, str]: for key, attrs in self._BUMPS: - if key and getattr(prev, key, None) != getattr(curr, key, None): - break - return attrs + if key is None or getattr(prev, key, None) != getattr(curr, key, None): + return attrs + return {} # Should never happen, but let's be safe. DEFAULT_TOOL_LOCKFILE = "" @@ -314,77 +314,30 @@ class _ResolveProviderType(Enum): @dataclass(frozen=True, order=True) class _ResolveProvider: option_name: str - type_: _ResolveProviderType class AmbiguousResolveNamesError(Exception): def __init__(self, ambiguous_name: str, providers: set[_ResolveProvider]) -> None: - tool_providers = [] - user_providers = [] - for provider in sorted(providers): - if provider.type_ == _ResolveProviderType.TOOL: - tool_providers.append(provider.option_name) - else: - user_providers.append(provider.option_name) - - if tool_providers: - if not user_providers: - raise AssertionError( - softwrap( - f""" - {len(tool_providers)} tools have the same options_scope: {ambiguous_name}. - If you're writing a plugin, rename your `GenerateToolLockfileSentinel`s so - that there is no ambiguity. Otherwise, please open a bug at - https://github.com/pantsbuild/pants/issues/new. - """ - ) - ) - if len(user_providers) == 1: - msg = softwrap( - f""" - A resolve name from the option `{user_providers[0]}` collides with the - name of a tool resolve: {ambiguous_name} - - To fix, please update `{user_providers[0]}` to use a different resolve name. - """ - ) - else: - msg = softwrap( - f""" - Multiple options define the resolve name `{ambiguous_name}`, but it is - already claimed by a tool: {user_providers} - - To fix, please update these options so that none of them use - `{ambiguous_name}`. - """ - ) - else: - assert len(user_providers) > 1 - msg = softwrap( - f""" - The same resolve name `{ambiguous_name}` is used by multiple options, which - causes ambiguity: {user_providers} + msg = softwrap( + f""" + The same resolve name `{ambiguous_name}` is used by multiple options, which + causes ambiguity: {providers} - To fix, please update these options so that `{ambiguous_name}` is not used more - than once. - """ - ) + To fix, please update these options so that `{ambiguous_name}` is not used more + than once. + """ + ) super().__init__(msg) def _check_ambiguous_resolve_names( all_known_user_resolve_names: Iterable[KnownUserResolveNames], - all_tool_sentinels: Iterable[type[GenerateToolLockfileSentinel]], ) -> None: resolve_name_to_providers = defaultdict(set) - for sentinel in all_tool_sentinels: - resolve_name_to_providers[sentinel.resolve_name].add( - _ResolveProvider(sentinel.resolve_name, _ResolveProviderType.TOOL) - ) for known_user_resolve_names in all_known_user_resolve_names: for resolve_name in known_user_resolve_names.names: resolve_name_to_providers[resolve_name].add( - _ResolveProvider(known_user_resolve_names.option_name, _ResolveProviderType.USER) + _ResolveProvider(known_user_resolve_names.option_name) ) for resolve_name, providers in resolve_name_to_providers.items(): @@ -401,11 +354,21 @@ def determine_resolves_to_generate( Return a tuple of `(user_resolves, tool_lockfile_sentinels)`. """ - _check_ambiguous_resolve_names(all_known_user_resolve_names, all_tool_sentinels) + # Let user resolve names silently shadow tools with the same name. + # This is necessary since we now support installing a tool from a named resolve, + # and it's not reasonable to ban the name of the tool as the resolve name, when it + # is the most obvious choice for that... + # This is likely only an issue if you were going to, e.g., have a named resolve called flake8 + # but not use it as the resolve for the flake8 tool, which seems pretty unlikely. + all_known_user_resolve_name_strs = set( + itertools.chain.from_iterable(akurn.names for akurn in all_known_user_resolve_names) + ) + all_tool_sentinels = [ + ts for ts in all_tool_sentinels if ts.resolve_name not in all_known_user_resolve_name_strs + ] - resolve_names_to_sentinels = { - sentinel.resolve_name: sentinel for sentinel in all_tool_sentinels - } + # Resolve names must be globally unique, so check for ambiguity across backends. + _check_ambiguous_resolve_names(all_known_user_resolve_names) if not requested_resolve_names: return [ @@ -423,9 +386,9 @@ def determine_resolves_to_generate( ) specified_sentinels = [] - for resolve, sentinel in resolve_names_to_sentinels.items(): - if resolve in requested_resolve_names: - requested_resolve_names.discard(resolve) + for sentinel in all_tool_sentinels: + if sentinel.resolve_name in requested_resolve_names: + requested_resolve_names.discard(sentinel.resolve_name) specified_sentinels.append(sentinel) if requested_resolve_names: @@ -436,7 +399,7 @@ def determine_resolves_to_generate( known_resolve_names.names for known_resolve_names in all_known_user_resolve_names ), - *resolve_names_to_sentinels.keys(), + *(sentinel.resolve_name for sentinel in all_tool_sentinels), }, description_of_origin="the option `--generate-lockfiles-resolve`", ) diff --git a/src/python/pants/core/goals/generate_lockfiles_test.py b/src/python/pants/core/goals/generate_lockfiles_test.py index b48cfc04b29..3d77c40a156 100644 --- a/src/python/pants/core/goals/generate_lockfiles_test.py +++ b/src/python/pants/core/goals/generate_lockfiles_test.py @@ -101,22 +101,23 @@ def assert_chosen( with pytest.raises(UnrecognizedResolveNamesError): assert_chosen({"fake"}, expected_user_resolves=[], expected_tools=[]) - # Error if same resolve name used for tool lockfiles and user lockfiles. class AmbiguousTool(GenerateToolLockfileSentinel): resolve_name = "ambiguous" - with pytest.raises(AmbiguousResolveNamesError): - determine_resolves_to_generate( - [ - KnownUserResolveNames( - ("ambiguous",), - "[lang].resolves", - requested_resolve_names_cls=Lang1Requested, - ) - ], - [AmbiguousTool], - set(), - ) + # Let a user resolve shadow a tool resolve with the same name. + assert determine_resolves_to_generate( + [ + KnownUserResolveNames( + ("ambiguous",), + "[lang].resolves", + requested_resolve_names_cls=Lang1Requested, + ) + ], + [AmbiguousTool], + set(), + ) == ([Lang1Requested(["ambiguous"])], []) + + # Error if same resolve name used for multiple user lockfiles. with pytest.raises(AmbiguousResolveNamesError): determine_resolves_to_generate( [ From e7e56b0923c94ea43470a886c9ca34ffe6bb4703 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Sun, 12 Mar 2023 16:35:30 -0700 Subject: [PATCH 2/2] Fix tests --- .../backend/python/macros/pipenv_requirements_test.py | 2 +- .../backend/python/macros/poetry_requirements_test.py | 2 +- .../backend/python/macros/python_requirements_test.py | 2 +- .../pants/engine/internals/synthetic_targets_test.py | 8 +++++--- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/python/pants/backend/python/macros/pipenv_requirements_test.py b/src/python/pants/backend/python/macros/pipenv_requirements_test.py index d20bb3d2ccc..de1065ac011 100644 --- a/src/python/pants/backend/python/macros/pipenv_requirements_test.py +++ b/src/python/pants/backend/python/macros/pipenv_requirements_test.py @@ -99,7 +99,7 @@ def test_pipfile_lockfile_dependency(rule_runner: RuleRunner) -> None: rule_runner.set_options(["--python-enable-resolves"]) file_addr = Address("", target_name="reqs", relative_file_path="Pipfile.lock") lock_addr = Address( - "3rdparty/python", target_name="python-default", relative_file_path="default.lock" + "3rdparty/python", target_name="_python-default_lockfile", relative_file_path="default.lock" ) assert_pipenv_requirements( rule_runner, diff --git a/src/python/pants/backend/python/macros/poetry_requirements_test.py b/src/python/pants/backend/python/macros/poetry_requirements_test.py index 18254b1d7a4..45a6fcf98f1 100644 --- a/src/python/pants/backend/python/macros/poetry_requirements_test.py +++ b/src/python/pants/backend/python/macros/poetry_requirements_test.py @@ -560,7 +560,7 @@ def test_lockfile_dependency(rule_runner: RuleRunner) -> None: rule_runner.set_options(["--python-enable-resolves"]) file_addr = Address("", target_name="reqs", relative_file_path="pyproject.toml") lock_addr = Address( - "3rdparty/python", target_name="python-default", relative_file_path="default.lock" + "3rdparty/python", target_name="_python-default_lockfile", relative_file_path="default.lock" ) assert_poetry_requirements( rule_runner, diff --git a/src/python/pants/backend/python/macros/python_requirements_test.py b/src/python/pants/backend/python/macros/python_requirements_test.py index c611927d4cb..da64003ba48 100644 --- a/src/python/pants/backend/python/macros/python_requirements_test.py +++ b/src/python/pants/backend/python/macros/python_requirements_test.py @@ -199,7 +199,7 @@ def test_lockfile_dependency(rule_runner: RuleRunner) -> None: rule_runner.set_options(["--python-enable-resolves"]) reqs_addr = Address("", target_name="reqs", relative_file_path="requirements.txt") lock_addr = Address( - "3rdparty/python", target_name="python-default", relative_file_path="default.lock" + "3rdparty/python", target_name="_python-default_lockfile", relative_file_path="default.lock" ) assert_python_requirements( rule_runner, diff --git a/src/python/pants/engine/internals/synthetic_targets_test.py b/src/python/pants/engine/internals/synthetic_targets_test.py index 423d6ead5cb..afde23b02e9 100644 --- a/src/python/pants/engine/internals/synthetic_targets_test.py +++ b/src/python/pants/engine/internals/synthetic_targets_test.py @@ -309,10 +309,10 @@ def test_target_name_collision_issue_17343(rule_runner: RuleRunner) -> None: "src/issues/17343/BUILD": softwrap( """ python_requirements( - name="python-default", + name="_python-default_lockfile", overrides={ "humbug": { - "dependencies": ["python-default#setuptools"], + "dependencies": ["_python-default_lockfile#setuptools"], }, }, ) @@ -330,7 +330,9 @@ def test_target_name_collision_issue_17343(rule_runner: RuleRunner) -> None: tgt = assert_target( rule_runner, - Address("src/issues/17343", target_name="python-default", generated_name="setuptools"), + Address( + "src/issues/17343", target_name="_python-default_lockfile", generated_name="setuptools" + ), alias="python_requirement", )