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

Extra spaces added to lock files - probable issue w/ interpreter selection. #18601

Closed
asherf opened this issue Mar 27, 2023 · 4 comments · Fixed by #18610
Closed

Extra spaces added to lock files - probable issue w/ interpreter selection. #18601

asherf opened this issue Mar 27, 2023 · 4 comments · Fixed by #18610
Assignees
Labels

Comments

@asherf
Copy link
Member

asherf commented Mar 27, 2023

This was introduced in 2.16.0 a1:
Screenshot 2023-03-27 at 4 20 58 PM
While the spaces themselves are not a big deal, there might be an underlying issue with more serious implications.
More information in the slack thread

Relevant PR: #18495
cc: @jsirois @thejcannon

@asherf asherf added the bug label Mar 27, 2023
@jsirois
Copy link
Contributor

jsirois commented Mar 27, 2023

@thejcannon the issue is #18495 leads to interpreter selection for the Pex CLI selecting Python 2.7 - which is the cause of the JSON trailing whitespace. In @asherf's case, Python 2.7 is not in bounds in any repo ICs and prior to #18495 Python 2.7 was not selected to run the Pex CLI with.

@thejcannon
Copy link
Member

Here's where we choose ICs for lockfile generation:

interpreter_constraints=InterpreterConstraints(

Here's where we pass it to PexCliProcess:
*req.interpreter_constraints.generate_pex_arg_list(),

Neither of which changed in the commit.

Additionally, the changes to pex_cli.py seem to be relatively safe to have done, aside from the obvious "pex is now run using sys.executable" The cmdline args shouldn't have changed.

@jsirois
Copy link
Contributor

jsirois commented Mar 27, 2023

If you follow the slack thread all good commit prior, reproducibly bad at that exact commit. So something is up.

@jsirois
Copy link
Contributor

jsirois commented Mar 27, 2023

If it's not clear the issue is the Python used to run the Pex CLI, not the arguments passed to it.

@thejcannon thejcannon self-assigned this Mar 28, 2023
thejcannon added a commit that referenced this issue Mar 29, 2023
Fixes #18601 (among likely
other issues) by ensuring we always run the pex PEX with a `--python`
set to the bootstrap Python.

Note that none of the callers were passing `python` so I removed the
parameter.
cognifloyd pushed a commit that referenced this issue Mar 31, 2023
Fixes #18601 (among likely
other issues) by ensuring we always run the pex PEX with a `--python`
set to the bootstrap Python.

Note that none of the callers were passing `python` so I removed the
parameter.
jsirois pushed a commit that referenced this issue Mar 31, 2023
Fixes #18601 (among likely
other issues) by ensuring we always run the pex PEX with a `--python`
set to the bootstrap Python.

Note that none of the callers were passing `python` so I removed the
parameter.

Co-authored-by: Joshua Cannon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants