Skip to content

Commit

Permalink
Don't require a local_environment target for each platform (#16983)
Browse files Browse the repository at this point in the history
We now are using subsystems as the default value of env vars & executable search paths. That is, `local_environment` targets are solely needed if you want to _override_ those option values.

We want it to be ergonomic to add a `local_environment` target to handle one specific platform, e.g. macOS ARM often being problematic, without you needing to also add boilerplate for the other platforms. There was no good reason we were erroring for other platforms, given the new subsystem approach.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Sep 26, 2022
1 parent 6eccce4 commit 9a6dc00
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 47 deletions.
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 @@ -98,12 +98,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 @@ -304,10 +307,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 @@ -402,50 +401,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

0 comments on commit 9a6dc00

Please sign in to comment.