-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
[internal] Add experimental_resolve
field to pex_binary
#12734
Changes from all commits
ad5a905
88cc938
9ff6af7
51a158e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,60 @@ def value_or_global_default(self, python_setup: PythonSetup) -> Tuple[str, ...]: | |
return python_setup.compatibility_or_constraints(self.value) | ||
|
||
|
||
class UnrecognizedResolveNamesError(Exception): | ||
def __init__( | ||
self, | ||
unrecognized_resolve_names: list[str], | ||
all_valid_names: Iterable[str], | ||
*, | ||
description_of_origin: str, | ||
) -> None: | ||
# TODO(#12314): maybe implement "Did you mean?" | ||
if len(unrecognized_resolve_names) == 1: | ||
unrecognized_str = unrecognized_resolve_names[0] | ||
name_description = "name" | ||
else: | ||
unrecognized_str = str(sorted(unrecognized_resolve_names)) | ||
name_description = "names" | ||
super().__init__( | ||
f"Unrecognized resolve {name_description} from {description_of_origin}: " | ||
f"{unrecognized_str}\n\nAll valid resolve names: {sorted(all_valid_names)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NB: the all_valid_names will be different when you're using the But it could be confusing. I wonder if this error message should better explain this nuance? Something like:
vs
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are tool resolves and user resolves conceptually in the same namespace? In other words, how ambiguous might this be when a tool resolve is included in this list? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A tool resolve is always its option scope, so When you run So the only time the two types of resolves should really interact is when you run Does that make sense? |
||
) | ||
|
||
|
||
class PythonResolveField(StringField, AsyncFieldMixin): | ||
alias = "experimental_resolve" | ||
# TODO(#12314): Figure out how to model the default and disabling lockfile, e.g. if we | ||
# hardcode to `default` or let the user set it. | ||
help = ( | ||
"The resolve from `[python-setup].experimental_resolves_to_lockfiles` to use, if any.\n\n" | ||
"This field is highly experimental and may change without the normal deprecation policy." | ||
) | ||
|
||
def validate(self, python_setup: PythonSetup) -> None: | ||
"""Check that the resolve name is recognized.""" | ||
if not self.value: | ||
return None | ||
if self.value not in python_setup.resolves_to_lockfiles: | ||
raise UnrecognizedResolveNamesError( | ||
[self.value], | ||
python_setup.resolves_to_lockfiles.keys(), | ||
description_of_origin=f"the field `{self.alias}` in the target {self.address}", | ||
) | ||
|
||
def resolve_and_lockfile(self, python_setup: PythonSetup) -> tuple[str, str] | None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This modeling took a lot of iterations. Originally I only had call sites pass the resolve name to It was less awkward imo to handle this validation here and having the callers pass the resolve_and_lockfile, rather than passing |
||
"""If configured, return the resolve name with its lockfile. | ||
|
||
Error if the resolve name is invalid. | ||
""" | ||
self.validate(python_setup) | ||
return ( | ||
(self.value, python_setup.resolves_to_lockfiles[self.value]) | ||
if self.value is not None | ||
else None | ||
) | ||
|
||
|
||
# ----------------------------------------------------------------------------------------------- | ||
# `pex_binary` target | ||
# ----------------------------------------------------------------------------------------------- | ||
|
@@ -383,6 +437,7 @@ class PexBinary(Target): | |
*COMMON_TARGET_FIELDS, | ||
OutputPathField, | ||
InterpreterConstraintsField, | ||
PythonResolveField, | ||
PexBinaryDependencies, | ||
PexEntryPointField, | ||
PexPlatformsField, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it would probably be simpler for this rule to execute per-resolve, rather than on a batch of resolves. The only thing that is saved by batching them is making a single pass to filter targets by resolve, but that should be cheap/linear time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh we generally always encourage people to use MultiGet, code like this is common. We could add a helper rule for an individual resolve if necessary, but that adds new boilerplate.
I think this is fine for now and we can refactor need be.