https://pantsbuild.org/ logo
#pex
Title
e

enough-analyst-54434

02/12/2019, 3:16 PM
I think, long term, killing
run_pex_command
is the way to go.
r

red-balloon-89377

02/12/2019, 3:18 PM
That would involve giving
run_simple_pex
the ability to override environments, right?
e

enough-analyst-54434

02/12/2019, 3:19 PM
I don't think so since it shells out.
r

red-balloon-89377

02/12/2019, 3:21 PM
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

enough-analyst-54434

02/12/2019, 3:26 PM
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

red-balloon-89377

02/12/2019, 3:30 PM
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

enough-analyst-54434

02/12/2019, 3:38 PM
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

red-balloon-89377

02/12/2019, 3:40 PM
There are several instances, but wouldn’t be agains reformatting:
Copy code
$ rg "env=make_env" tests | wc -l
15
Meaning reformatting myself
e

enough-analyst-54434

02/12/2019, 3:41 PM
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

red-balloon-89377

02/12/2019, 3:42 PM
Copy code
$ rg "env=make_env" tests/test_integration.py | wc -l
15
Happy to create a PR that removes the env param
e

enough-analyst-54434

02/12/2019, 3:48 PM
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

red-balloon-89377

02/12/2019, 3:51 PM
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

enough-analyst-54434

02/12/2019, 5:31 PM
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

red-balloon-89377

02/12/2019, 5:37 PM
That’s a good idea!Thanks
will do
e

enough-analyst-54434

02/14/2019, 4:33 PM
@red-balloon-89377 should I give the Twitter reviewer more time on https://github.com/pantsbuild/pex/pull/670 ?
r

red-balloon-89377

02/14/2019, 5:01 PM
NO, we just went though it
e

enough-analyst-54434

02/14/2019, 5:01 PM
K
r

red-balloon-89377

02/14/2019, 5:01 PM
It’s good to go 🙂 and a very good solution
e

enough-analyst-54434

02/14/2019, 5:02 PM
Did you all get why
-mpex
worked?
r

red-balloon-89377

02/14/2019, 5:06 PM
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

enough-analyst-54434

02/14/2019, 5:07 PM
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

red-balloon-89377

02/14/2019, 5:08 PM
Yeah, I didn’t know that tox existed, and it’s awesome!