Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Extract out
resolve_requirements
V2 rule for creating PEXes with requirements #7846Extract out
resolve_requirements
V2 rule for creating PEXes with requirements #7846Changes from 8 commits
37f7267
9960373
4431cf7
0f6920f
ff39520
bf24cde
1f1fd19
8302c5f
61231eb
4cdfb9f
d2ea50f
806d310
6629a45
9217294
bd47eab
ad3c408
bf733bc
8294b19
6f88299
d1ff3bf
c036d8c
a0619e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure this is kosher; I think it happens to work locally because of how we happen to have implemented
$PATH
suppression (pants/src/rust/engine/process_execution/src/local.rs
Lines 106 to 114 in 6ab5f7a
$PATH
shouldn't be used to look upargv[0]
even if it's set.Sadly I suspect the technically correct answer here is a little wrapper shell script along the lines of:
exec "$@"
which we'd invoke
lookup_on_path python ./output...
but I guess we can punt on this until it becomes a problem.
(We could also make our local process execution resemble the spec more closely by before running inspecting "does
argv[0]
contain a/
, if not, error"?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works on remoting and local. I made this change in #7844 specifically to fix the remoting case.
The reason it works (I think) is that we explicitly set the
env
to include the entryPATH: interpreter_search_paths
.--
Are you proposing having this be something like
/usr/bin/python
? Or parametrizing it as an option so that you can set which you want?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per https://github.com/bazelbuild/remote-apis/blob/a5c577357528b33a4adff88c0c7911dd086c6923/build/bazel/remote/execution/v2/remote_execution.proto#L433-L436 the execution system shouldn't do $PATH lookup; in particular, it's totally valid for it to
execv
and notexecvp
... Our local execution happens toexecvp
because that's what rust conveniently exposes to us, but it shouldn't...I'm surprised RBE does, if it does... Does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't worked on remoting in two weeks and the context switch to verify this is going to take too much time to be able to quickly check, but I know when working on #7844 the change allowed me to get further than before. Originally, iirc it would complain it could not find the file
/Users/eric/DocsLocal/code/projects/pants/build-support/pants_dev_deps.py36.venv/bin/python
; with the change, the error was just that Pex could not resolve requirements.Will hopefully be done with Py3 work soon and be able to go back to remoting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
This file was deleted.