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

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

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

cburroughs
Copy link
Contributor

Pex exposes the --check flag to "check" if the resulting zipapp can be opened by CPython. Plumb that through so that Pants stops making PEXs that won't work at runtime.

NOTE: The default of error differs from Pex. I'm struggling to think of a case where one would want a zipapp CPython can't open and Pants has a history of swalling warnings from Pex (warn is Pex's default).

ref #19957

Pex exposes the `--check` flag to "check" if the resulting zipapp can
be opened by CPython.  Plumb that through so that Pants stops making
PEXs that won't work at runtime.

NOTE: The default of `error` differs from Pex.  I'm struggling to
think of a case where one would want a zipapp CPython can't open and
Pants has a history of swalling warnings from Pex (`warn` is Pex's
default).

ref pantsbuild#19957
@cburroughs cburroughs self-assigned this Feb 1, 2024
@cburroughs cburroughs added category:bugfix Bug fixes for released features backend: Python Python backend-related issues labels Feb 1, 2024
alias = "check"
valid_choices = ("none", "warn", "error")
expected_type = str
default = "error"
Copy link
Contributor

@huonw huonw Feb 6, 2024

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.)

@cburroughs cburroughs requested a review from huonw February 7, 2024 16:29
@huonw huonw merged commit 9ae7752 into pantsbuild:main Feb 8, 2024
24 checks passed
@huonw
Copy link
Contributor

huonw commented Feb 8, 2024

Thanks! (BTW, I took the liberty of removing the paragraph about the error default from the final commit message.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants