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.

Sign up for GitHub

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

plumb through Pex's --check zipapp validation #20481

Merged
merged 2 commits into from
Feb 8, 2024
Merged

plumb through Pex's --check zipapp validation #20481

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
753b4c5
plumb through Pex's --check zipapp validation
cburroughs Feb 1, 2024
4db68c4
align with tool defaults
cburroughs Feb 7, 2024
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
4 changes: 4 additions & 0 deletions src/python/pants/backend/python/goals/package_pex_binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from pants.backend.python.target_types import (
PexArgsField,
PexBinaryDefaults,
PexCheckField,
PexCompletePlatformsField,
PexEmitWarningsField,
PexEntryPointField,
Expand Down Expand Up @@ -79,6 +80,7 @@ class PexBinaryFieldSet(PackageFieldSet, RunFieldSet):
venv_site_packages_copies: PexVenvSitePackagesCopies
venv_hermetic_scripts: PexVenvHermeticScripts
environment: EnvironmentField
check: PexCheckField

@property
def _execution_mode(self) -> PexExecutionMode:
Expand All @@ -96,6 +98,8 @@ def generate_additional_args(self, pex_binary_defaults: PexBinaryDefaults) -> Tu
args.append(f"--inherit-path={self.inherit_path.value}")
if self.sh_boot.value is True:
args.append("--sh-boot")
if self.check.value is not None:
args.append(f"--check={self.check.value}")
if self.shebang.value is not None:
args.append(f"--python-shebang={self.shebang.value}")
if self.strip_env.value is False:
Expand Down
19 changes: 19 additions & 0 deletions src/python/pants/backend/python/target_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,24 @@ class PexArgsField(StringSequenceField):
)


class PexCheckField(StringField):
alias = "check"
valid_choices = ("none", "warn", "error")
expected_type = str
default = "error"
Copy link
Contributor

@huonw huonw Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to keep this at warn, to avoid breaking existing users who have a build that works for them when they upgrade pants (e.g. maybe they manipulate the resulting zipapp as a zip file directly somehow... potentially better served with a different layout, but we shouldn't be encouraging that with a hard-error). It'd definitely be fine to switch the default with some sort of deprecation cycle.

Suggested change
default = "error"
default = "warn"

Of course, pants swallowing warnings from PEX isn't so good, but I see you have #20480 to help. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the judgement call going either way. Is there an example for what the deprecation system looks like for changing a default? I saw lots of examples for removing fields/flags, but not changing a default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a specific example exactly like this, but there's occasionally options in pants.toml to control global default behaviour, e.g. https://www.pantsbuild.org/2.13/reference/global-options#use_deprecated_directory_cli_args_semantics, so I could imagine putting an option in the [pex] subsystem or similar.

(Unresolving just to continue this conversation.)

help = help_text(
"""
Check that the built PEX is valid. Currently this only
applies to `--layout zipapp` where the PEX zip is
tested for importability of its `__main__` module by
the Python zipimport module. This check will fail for
PEX zips that use ZIP64 extensions since the Python
zipimport zipimporter only works with 32 bit zips. The
check no-ops for all other layouts.
"""
)


class PexEnvField(DictStringToStringField):
alias = "env"
help = help_text(
Expand Down Expand Up @@ -697,6 +715,7 @@ class PexVenvHermeticScripts(BoolField):
InterpreterConstraintsField,
PythonResolveField,
PexBinaryDependenciesField,
PexCheckField,
PexPlatformsField,
PexCompletePlatformsField,
PexResolveLocalPlatformsField,
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/util_rules/pex_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class PexCli(TemplatedExternalTool):

default_version = "v2.1.159"
default_url_template = "https://github.com/pantsbuild/pex/releases/download/{version}/pex"
version_constraints = ">=2.1.135,<3.0"
version_constraints = ">=2.1.148,<3.0"

@classproperty
def default_known_versions(cls):
Expand Down
Loading