https://pantsbuild.org/ logo
Join Slack
Powered by
I think, long term, killing `run_pex_command` is t...
# pex
e
I think, long term, killing 
run_pex_command
is the way to go.
r
That would involve giving 
run_simple_pex
the ability to override environments, right?
e
I don't think so since it shells out.
r
I didn’t. I just wanted to mention that the 
env
parameter in 
run_pex_command
doesn’t do anything if we want to override something already set in 
ENV
, which could be solved with a call to 
patch
, but wanted to confirm that this behavior was not intentional 🙂 I’m probably missing a lot of the context here, so I want to be careful.
e
Aha. I'm pretty sure the behavior is not intentional. So the fix would be correct but dubious since that would encourage using 
run_pex_command
for a new set of cases of which your test would be the first instance. Whichever way you want to go is fine since dealing with the legacy of this fragile helper function is not something you should have to worry about.
r
Hmm, I’m leaning towards making 
run_pex_command
do what it says it does, since I’d expect the 
env
parameter to override 
ENV
. However, I’m also open to trying to express the test in terms of 
run_simple_pex
, which will take a while if it’s even possible.
e
Perhaps just check if folks actually pass env. If one or two cases, yank the param and have users erect the ENV context manager inline in their test. The whole point of ENV is for centralized reading of env vars in pex, so there should be no need to patch up both.
r
There are several instances, but wouldn’t be agains reformatting:
Copy code
$ rg "env=make_env" tests | wc -l
15
Meaning reformatting myself
e
Only in integration will folks be calling 
run_pex_command
, so it's only the cases in that one file calling that one function.
r
Copy code
$ rg "env=make_env" tests/test_integration.py | wc -l
15
Happy to create a PR that removes the env param
e
My assertion was false. The important exception is PATH which is read directly from env when finding interpreters. As such, your original suggested fix or else using the ENV context manager inline in your new test case are the only two viable solutions short of avoiding 
run_pex_comnand
.
r
Okay, I will add the ENV cm inline, and if I have time I’ll make a separate pr with the fix.
Thanks!
Re: https://github.com/pantsbuild/pex/issues/669, I was hoping to use it as an introduction to PEX for another member of the team, so would you mind discussing the approach here, or waiting on the PR? If it’s urgent I totally understand, but I can get my fixes with the ENV contextmanager
e
Sorry about that - already done.
run_pex_command
was just horrible. It cost you time and was always needlessly fragile.
I will say - despite that PR being tiny - actually understanding why it works would be a good intro to pex dev (ie: tox - not pex itself). So maybe that person could help review.
r
That’s a good idea!Thanks
will do
e
@red-balloon-89377 should I give the Twitter reviewer more time on https://github.com/pantsbuild/pex/pull/670 ?
r
NO, we just went though it
e
K
r
It’s good to go 🙂 and a very good solution
e
Did you all get why 
-mpex
worked?
r
We didn’t get to that, but I think it’s because it will just run 
pex.main()
with the correct env (where the interpreter is set by tox), right?
e
Yes. A fundamental guaranty of tox is that for any tox env, it runs your project setup.py 1st and puts the resulting dist on that env's PYTHONPATH.
r
Yeah, I didn’t know that tox existed, and it’s awesome!
Open in Slack
PreviousNext
Powered by