Avoid fast-path in --sh-boot script when PEX_TOOLS=1 #2726
Conversation
|
I haven't added a test yet, to confirm the direction: is the sh-boot script the right place to put this, or is it better placed in |
pex/sh_boot.py
Outdated
| @@ -199,10 +199,11 @@ def create_sh_boot_script( | |||
| PEX_ROOT="${{PEX_ROOT:-${{DEFAULT_PEX_ROOT}}}}" | |||
| INSTALLED_PEX="${{PEX_ROOT}}/{pex_installed_relpath}" | |||
|
|
|||
| if [ -n "${{VENV}}" -a -x "${{INSTALLED_PEX}}" ]; then | |||
| if [ -n "${{VENV}}" -a -x "${{INSTALLED_PEX}}" -a "${{PEX_TOOLS:-}}" != "" ]; then | |||
There was a problem hiding this comment.
You inverted your logic here which the existing IT caught. Also maybe just use -z instead of = to be (anti)parallel with the -n logic at the front of the line.
There was a problem hiding this comment.
Oh, yes 🤦
-z is much better. Thanks.
This is the right place. The venv does not contain any PEX So that's 2 Python binary startups. My shortcut accounting for a Python invocation with some stdlib imports at least is ~50ms. With your change, you just have 1 Python binary startup. The clincher though is your change keeps the logic central instead of spreading it out in 2 files. |
|
@huonw if you want to update |
|
I've updated:
Thanks for the initial review.
Ah, yes, that makes the "proper" approach (that in this PR) clearly the better one. Thank you. |
|
Thanks @huonw. This is now available in 2.33.7: https://github.com/pex-tool/pex/releases/tag/v2.33.7 |
This patch stops the "execute venv entrypoint directly" fast-path from happening when the user has set
PEX_TOOLS=1with a--sh-bootPEX, mimicking the similar check performed for Python bootstrapping:pex/pex/pex_boot.py
Lines 204 to 205 in 17bd416
This change ensures that
PEX_TOOLS=1 ./some.pexalways works, no matter the state of thePEX_ROOTc 8000 ache.Fixes #2725