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 sh-boot through to pex_binary #19925

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

cburroughs
Copy link
Contributor

ref #19514

@AlexTereshenkov AlexTereshenkov added backend: Python Python backend-related issues category:new feature labels Sep 25, 2023
@huonw
Copy link
Contributor

huonw commented Sep 27, 2023

Hey @cburroughs, thanks for the PR. Just checking: is the DRAFT in the title still currently, or is this ready for review?

@cburroughs
Copy link
Contributor Author

I think #19514 needs some more discussion first before this can be expanded and un-DRAFT-ified.

@huonw
Copy link
Contributor

huonw commented Oct 3, 2023

Cool, thanks for the explanation. I'll mark this as a GitHub draft too so that it filters out of PR searches, and asked on slack for guidance about your research there.

@huonw huonw marked this pull request as draft October 3, 2023 21:48
@huonw huonw requested a review from thejcannon October 3, 2023 22:22
@huonw huonw mentioned this pull request Dec 30, 2023
@cburroughs
Copy link
Contributor Author

I think with pex-tool/pex#2296 available it makes sense to plumb through sh-boot, but not mess with defaults. Ready for review.

@cburroughs cburroughs marked this pull request as ready for review January 4, 2024 20:39
@cburroughs cburroughs changed the title DRAFT: plumb sh-boot through to pex_binary plumb sh-boot through to pex_binary Jan 4, 2024
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Looks good. Could we add a test that validates the --sh-boot flag is passed appropriately?

@cburroughs cburroughs requested a review from huonw January 5, 2024 19:40
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Awesome!

I idly wonder if we could (eventually/with a deprecation cycle) switch this to True, given the description sounds like it's almost always better.

huonw added a commit that referenced this pull request Jan 9, 2024
All changes:

- https://github.com/pantsbuild/pex/releases/tag/v2.1.153
- https://github.com/pantsbuild/pex/releases/tag/v2.1.154
- https://github.com/pantsbuild/pex/releases/tag/v2.1.155

Highlights:

- `--no-pre-install-wheels` (and `--max-install-jobs`) that likely helps
with:
  - #15062 
  - (the root cause of) #20227
  - _maybe_ arguably #18293, #18965, #19681 
- improved shebang selection, helping with
#19514, but probably not the
full solution (#19925)
- performance improvements
@@ -486,6 +486,29 @@ class PexIgnoreErrorsField(BoolField):
help = "Should PEX ignore errors when it cannot resolve dependencies?"


class PexShBootField(BoolField):
alias = "sh_boot"
default = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Since people very likely want to set this to True, should we consider a deprecation cycle to change this to be the default? In a clean world with no backwards compatibility considerations I imagine this would have been the default in Pex.

And/or, should there be an option to set this globally?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will merge now, since this can be done in a followup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since people very likely want to set this to True, should we consider a deprecation cycle to change this to be the default?

I think Pex tries to support constrained environments without sh, so I'm not sure of the hypothetical default there. But yeah for Pants I think this is value that is more likely to "just work".

And/or, should there be an option to set this globally?

Isn't __defaults__ or macros the usual answer in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, actually, we should just document the use of __defaults__ for this.

@benjyw benjyw merged commit b8c5fbf into pantsbuild:main Jan 11, 2024
24 checks passed
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:new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants