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

Don't require a local_environment target for each platform #16983

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 27 additions & 43 deletions src/python/pants/core/util_rules/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,15 @@ class CompatiblePlatformsField(StringSequenceField):
valid_choices = Platform
value: tuple[str, ...]
help = softwrap(
"""
f"""
Which platforms this environment can be used with.

This is used for Pants to automatically determine which environment target to use for
the user's machine. Currently, there must be exactly one environment target for the
platform.
the user's machine when the environment is set to the special value
`{LOCAL_ENVIRONMENT_MATCHER}`. Currently, there cannot be more than one environment target
registered in `[environments-preview].names` for a particular platform. If there is no
environment target for a certain platform, Pants will use the options system instead to
determine environment variables and executable search paths.
"""
)

Expand Down Expand Up @@ -302,10 +305,6 @@ def determine_bootstrap_environment(session: SchedulerSession) -> EnvironmentNam
return EnvironmentName(local_env.val)


class NoCompatibleEnvironmentError(Exception):
pass


class AmbiguousEnvironmentError(Exception):
pass

Expand Down Expand Up @@ -400,50 +399,35 @@ async def determine_local_environment(
all_environment_targets: AllEnvironmentTargets,
) -> ChosenLocalEnvironmentName:
platform = Platform.create_for_localhost()
if not all_environment_targets:
return ChosenLocalEnvironmentName(None)
compatible_name_and_targets = [
(name, tgt)
for name, tgt in all_environment_targets.items()
if tgt.has_field(CompatiblePlatformsField)
and platform.value in tgt[CompatiblePlatformsField].value
]
if not compatible_name_and_targets:
raise NoCompatibleEnvironmentError(
softwrap(
f"""
No `_local_environment` targets from `[environments-preview].names` are
compatible with the current platform: {platform.value}

To fix, either adjust the `{CompatiblePlatformsField.alias}` field from the targets
in `[environments-preview].names` to include `{platform.value}`, or define a new
`_local_environment` target with `{platform.value}` included in the
`{CompatiblePlatformsField.alias}` field. (Current targets from
`[environments-preview].names`:
{sorted(tgt.address.spec for tgt in all_environment_targets.values())})
"""
)
)
elif len(compatible_name_and_targets) > 1:
# TODO(#7735): Consider if we still want to error when no target is found, given that we
# are now falling back to subsystem values.
# TODO(#7735): Allow the user to disambiguate what __local__ means via an option.
raise AmbiguousEnvironmentError(
softwrap(
f"""
Multiple `_local_environment` targets from `[environments-preview].names`
are compatible with the current platform `{platform.value}`, so it is ambiguous
which to use:
{sorted(tgt.address.spec for _name, tgt in compatible_name_and_targets)}

To fix, either adjust the `{CompatiblePlatformsField.alias}` field from those
targets so that only one includes the value `{platform.value}`, or change
`[environments-preview].names` so that it does not define some of those targets.
"""
)
# That is, use the values from the options system instead, rather than from fields.
return ChosenLocalEnvironmentName(None)

if len(compatible_name_and_targets) == 1:
result_name, _tgt = compatible_name_and_targets[0]
return ChosenLocalEnvironmentName(result_name)

# TODO(#7735): Maybe allow the user to disambiguate what __local__ means via an option?
raise AmbiguousEnvironmentError(
softwrap(
f"""
Multiple `_local_environment` targets from `[environments-preview].names`
are compatible with the current platform `{platform.value}`, so it is ambiguous
which to use:
{sorted(tgt.address.spec for _name, tgt in compatible_name_and_targets)}

To fix, either adjust the `{CompatiblePlatformsField.alias}` field from those
targets so that only one includes the value `{platform.value}`, or change
`[environments-preview].names` so that it does not define some of those targets.
"""
)
result_name, _tgt = compatible_name_and_targets[0]
return ChosenLocalEnvironmentName(result_name)
)


@rule_helper
Expand Down
6 changes: 2 additions & 4 deletions src/python/pants/core/util_rules/environments_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
EnvironmentTarget,
FallbackEnvironmentField,
LocalEnvironmentTarget,
NoCompatibleEnvironmentError,
NoFallbackEnvironmentError,
RemoteEnvironmentTarget,
RemoteExtraPlatformPropertiesField,
Expand Down Expand Up @@ -189,10 +188,9 @@ def get_env() -> EnvironmentTarget:
rule_runner.set_options(["--environments-preview-names={'e': '//:e1'}"])
assert get_env().val.address == Address("", target_name="e1") # type: ignore[union-attr]

# Error if `--names` set, but no compatible platforms
# If `--names` is set, but no compatible platforms, do not choose an environment.
rule_runner.set_options(["--environments-preview-names={'e': '//:not-compatible'}"])
with engine_error(NoCompatibleEnvironmentError):
get_env()
assert get_env().val is None

# Error if >1 compatible targets.
rule_runner.set_options(["--environments-preview-names={'e1': '//:e1', 'e2': '//:e2'}"])
Expand Down