From b712ea51947dadb8043c587f893307dce7745861 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 14 Jan 2022 16:55:13 -0700 Subject: [PATCH 1/3] [internal] Check for ambiguity when running `generate-lockfiles` [ci skip-build-wheels] [ci skip-rust] --- .../pants/core/goals/generate_lockfiles.py | 82 ++++++++++++++++--- .../core/goals/generate_lockfiles_test.py | 41 ++++++++-- 2 files changed, 103 insertions(+), 20 deletions(-) diff --git a/src/python/pants/core/goals/generate_lockfiles.py b/src/python/pants/core/goals/generate_lockfiles.py index f7e7e1bd501..d129d292f12 100644 --- a/src/python/pants/core/goals/generate_lockfiles.py +++ b/src/python/pants/core/goals/generate_lockfiles.py @@ -5,7 +5,9 @@ import itertools import logging +from collections import defaultdict from dataclasses import dataclass +from enum import Enum from typing import ClassVar, Iterable, Sequence, cast from pants.engine.collection import Collection @@ -131,22 +133,76 @@ def __init__( ) +class _ResolveProviderType(Enum): + TOOL = 1 + USER = 2 + + +@dataclass(frozen=True, order=True) +class _ResolveProvider: + option_name: str + type_: _ResolveProviderType + + class AmbiguousResolveNamesError(Exception): - def __init__(self, ambiguous_names: list[str]) -> None: - if len(ambiguous_names) == 1: - first_paragraph = ( - "A resolve name from the option `[python].experimental_resolves` collides with the " - f"name of a tool resolve: {ambiguous_names[0]}" - ) + 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( + f"Two tools have the same options_scope: {ambiguous_name}. This " + "should not be possible. Please open a bug at " + "https://github.com/pantsbuild/pants/issues/new." + ) + if len(user_providers) == 1: + msg = ( + f"A resolve name from the option `{user_providers[0]}` collides with the " + f"name of a tool resolve: {ambiguous_name}\n\n" + f"To fix, please update `{user_providers[0]}` to use a different resolve name." + ) + else: + msg = ( + f"Multiple options define the resolve name `{ambiguous_name}`, but it is " + f"already claimed by a tool: {user_providers}\n\n" + f"To fix, please update these options so that none of them use " + f"`{ambiguous_name}`." + ) else: - first_paragraph = ( - "Some resolve names from the option `[python].experimental_resolves` collide with " - f"the names of tool resolves: {sorted(ambiguous_names)}" + assert len(user_providers) > 1 + msg = ( + f"The same resolve name `{ambiguous_name}` is used by multiple options, which " + f"causes ambiguity: {user_providers}\n\n" + f"To fix, please update these options so that `{ambiguous_name}` is not used more " + f"than once." ) - super().__init__( - f"{first_paragraph}\n\n" - "To fix, please update `[python].experimental_resolves` to use different resolve names." + super().__init__(msg) + + +def _check_ambiguous_resolve_names( + all_known_user_resolve_names: Iterable[KnownUserResolveNames], + all_tool_sentinels: Iterable[type[ToolLockfileSentinel]], +) -> None: + resolve_name_to_providers = defaultdict(set) + for sentinel in all_tool_sentinels: + resolve_name_to_providers[sentinel.options_scope].add( + _ResolveProvider(sentinel.options_scope, _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) + ) + + for resolve_name, providers in resolve_name_to_providers.items(): + if len(providers) > 1: + raise AmbiguousResolveNamesError(resolve_name, providers) def determine_resolves_to_generate( @@ -158,6 +214,8 @@ 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) + resolve_names_to_sentinels = { sentinel.options_scope: sentinel for sentinel in all_tool_sentinels } diff --git a/src/python/pants/core/goals/generate_lockfiles_test.py b/src/python/pants/core/goals/generate_lockfiles_test.py index d15bd178c1f..c000bfb3a20 100644 --- a/src/python/pants/core/goals/generate_lockfiles_test.py +++ b/src/python/pants/core/goals/generate_lockfiles_test.py @@ -8,6 +8,7 @@ from pants.core.goals.generate_lockfiles import ( DEFAULT_TOOL_LOCKFILE, NO_TOOL_LOCKFILE, + AmbiguousResolveNamesError, KnownUserResolveNames, LockfileRequest, RequestedUserResolveNames, @@ -74,15 +75,39 @@ def assert_chosen( with pytest.raises(UnrecognizedResolveNamesError): assert_chosen({"fake"}, expected_user_resolves=[], expected_tools=[]) - # TODO: Add ambiguity checks. # Error if same resolve name used for tool lockfiles and user lockfiles. - # class AmbiguousTool(ToolLockfileSentinel): - # options_scope = "ambiguous" - # - # with pytest.raises(AmbiguousResolveNamesError): - # determine_resolves_to_generate( - # {"ambiguous": "lockfile.txt"}, [AmbiguousTool], ["ambiguous"] - # ) + class AmbiguousTool(ToolLockfileSentinel): + options_scope = "ambiguous" + + with pytest.raises(AmbiguousResolveNamesError): + determine_resolves_to_generate( + [ + KnownUserResolveNames( + ("ambiguous",), + "[ambiguous].resolves", + requested_resolve_names_cls=Lang1Requested, + ) + ], + [AmbiguousTool], + set(), + ) + with pytest.raises(AmbiguousResolveNamesError): + determine_resolves_to_generate( + [ + KnownUserResolveNames( + ("ambiguous",), + "[ambiguous1].resolves", + requested_resolve_names_cls=Lang1Requested, + ), + KnownUserResolveNames( + ("ambiguous",), + "[ambiguous2].resolves", + requested_resolve_names_cls=Lang1Requested, + ), + ], + [], + set(), + ) def test_filter_tool_lockfile_requests() -> None: From 6d57383489b7f0a96ea71bc3b3e660aab80e06ef Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 14 Jan 2022 16:58:17 -0700 Subject: [PATCH 2/3] More clear example in the test # 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/core/goals/generate_lockfiles_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/python/pants/core/goals/generate_lockfiles_test.py b/src/python/pants/core/goals/generate_lockfiles_test.py index c000bfb3a20..ff98d831e04 100644 --- a/src/python/pants/core/goals/generate_lockfiles_test.py +++ b/src/python/pants/core/goals/generate_lockfiles_test.py @@ -84,7 +84,7 @@ class AmbiguousTool(ToolLockfileSentinel): [ KnownUserResolveNames( ("ambiguous",), - "[ambiguous].resolves", + "[lang].resolves", requested_resolve_names_cls=Lang1Requested, ) ], @@ -96,12 +96,12 @@ class AmbiguousTool(ToolLockfileSentinel): [ KnownUserResolveNames( ("ambiguous",), - "[ambiguous1].resolves", + "[lang1].resolves", requested_resolve_names_cls=Lang1Requested, ), KnownUserResolveNames( ("ambiguous",), - "[ambiguous2].resolves", + "[lang2].resolves", requested_resolve_names_cls=Lang1Requested, ), ], From 47ed8de8f9fb9655225978de1de676337b566f44 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Tue, 18 Jan 2022 11:52:21 -0700 Subject: [PATCH 3/3] Better error message for plugin authors # 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/core/goals/generate_lockfiles.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/python/pants/core/goals/generate_lockfiles.py b/src/python/pants/core/goals/generate_lockfiles.py index d129d292f12..fbced3d8846 100644 --- a/src/python/pants/core/goals/generate_lockfiles.py +++ b/src/python/pants/core/goals/generate_lockfiles.py @@ -157,8 +157,9 @@ def __init__(self, ambiguous_name: str, providers: set[_ResolveProvider]) -> Non if tool_providers: if not user_providers: raise AssertionError( - f"Two tools have the same options_scope: {ambiguous_name}. This " - "should not be possible. Please open a bug at " + f"{len(tool_providers)} tools have the same options_scope: {ambiguous_name}. " + "If you're writing a plugin, rename your `ToolLockfileSentinel`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: