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

Add --python-path to change interpreter search paths when building a PEX #1077

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

Eric-Arellano
Copy link
Contributor

Closes #1012.

Currently, PEX defaults to searching the $PATH when --interpreter-constraint and/or --resolve-local-platforms are used. (Otherwise, it uses the current interpreter or --python).

You can specify PEX_PYTHON_PATH, but this has several issues:

Instead of further using PEX_PYTHON_PATH, we add a proper --python-path flag. In a followup, we can remove the PEX_PYTHON_PATH mechanism.

"path to an interpreter, or specify a binary accessible on $PATH. This option "
"can be passed multiple times to create a multi-interpreter compatible pex. "
"Default: Use current interpreter.",
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.

Reworded. I wanted to add a little insight into how Pex only sometimes searches for interpreters.

'e.g. "CPython>=2.7,<3" (A CPython interpreter with version >=2.7 AND version <3) '
'or "PyPy" (A pypy interpreter of any version). This argument may be repeated multiple '
"times to OR the constraints.",
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.

Only reformatted.

)

group.add_option(
"--rcfile",
dest="rc_file",
default=None,
help="An additional path to a pexrc file to read during configuration parsing. "
"Used primarily for testing.",
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.

Reworded to emphasize this appends, rather than replaces. And to mention the env var.

pex/bin/pex.py Outdated Show resolved Hide resolved
pex/bin/pex.py Outdated Show resolved Hide resolved
@Eric-Arellano Eric-Arellano merged commit f6ec8ec into pex-tool:master Oct 15, 2020
@Eric-Arellano Eric-Arellano deleted the pex-python-buildtime branch October 15, 2020 00:15
@Eric-Arellano
Copy link
Contributor Author

Oops...my GitHub UI was messed up and showed this was green, when CI checks were still running. Let's hope this does go green..I'll fix forward otherwise.

@jsirois
Copy link
Member

jsirois commented Oct 15, 2020

Thanks for digging into this issue. This is definitely the way to go.

@Eric-Arellano
Copy link
Contributor Author

Yay! Should we indeed remove PEX_PYTHON_PATH from ever being used at build time? Happy to do so, only wanted your ack first.

@jsirois
Copy link
Member

jsirois commented Oct 15, 2020

I'm not sure. The whole PEX_PYTHON, PEX_PYTHON_PATH, .pexrc business was a Twitter attempt in good faith that is currently a mess. There is no urgency I can see at any rate.

@Eric-Arellano
Copy link
Contributor Author

The only urgency imo is that PEX_PYTHON_PATH will be used unless you set PEX_IGNORE_RCFILES=true. I wouldn't be very concerned if you have to set --rcfile for it to be activated. But as found in #1075, you only have to leave off PEX_IGNORE_RCFILES for this behavior. That's the default, which isn't ideal.

@jsirois
Copy link
Member

jsirois commented Oct 15, 2020

I think no-one except Twitter and Pants use PEX_PYTHON_PATH and IIUC Pants will now be dropping use - at least at buildtime. Twitter uses it in .pexrc files and so they (should) know what they're in for, especially since they wrote the feature.

@Eric-Arellano
Copy link
Contributor Author

IIUC Pants will now be dropping use - at least at buildtime.

This is correct. Pants will only use PEX_PYTHON_PATH at runtime if the Pex was created with internal_only=False. We currently never have Pants run an internal_only=False Pex, so in practice, we'll never end up setting it.

Eric-Arellano added a commit to pantsbuild/pants that referenced this pull request Oct 15, 2020
…interpreter_search_paths` (#10965)

### Problem

`PEX_PYTHON_PATH` is not used building a PEX, unless RC files are permitted, which we must ban. This means that `[python-setup].interpreter_search_paths` had zero impact when building a PEX, and Pex always looked at `$PATH` (controlled by `[pex].executable_search_paths`)

### Solution

Use the new `--python-path` build-time flag introduced in pex-tool/pex#1077.

Also fix the argv for our rule to select an interpreter, which was putting Pex flags in the `--` passthrough args section, so they were being ignored.

### Result

`[python-setup].interpreter_search_paths` correctly controls the interpreter search paths both when building a PEX and running a PEX.

[ci skip-rust]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow constraining the interpreter search path directly.
3 participants