Relocate environment variable injection to before the interpreter is run #2260

erikwilliamson · 2023-10-02T13:13:33Z

jsirois · 2023-10-02T16:56:44Z

@erikwilliamson thanks for working on this. Please add a test to prove this works. You likely can just pile in the original PR test: https://github.com/pantsbuild/pex/pull/1948/files#diff-2b86577053b9bb459d54416c397bf7d1fa598becf7a5cc9e856290fb0427881e

You can then tox -eformat-run,typecheck to run black + mypy and tox -epyXY-integration -- -ktest_inject_env_and_args to run your tests via pytest. Here XY is the Python major minor of a Python you have installed, for example tox -epy311-integration. You can use ./dtox instead of tox with the rest of the command line being the same to run the tests in a docker environment that has all the Pythons Pex supports pre-installed. The 1st time you run, using BASE_MODE=pull ./dtox will likely speed things up by pulling the base docker image instead of building it.

ghost · 2023-10-16T20:35:03Z

@jsirois I'm sorry that this is taking me so long. I'm having trouble getting any tests to run successfully, even from the main branch. I'm totally new to Tox, too which isn't speeding things up.

I'm getting the following when running dtox.sh:

erik@mini pex % BASE_MODE=pull ./dtox.sh -epy311-integration
1ff12c11995a5b21b7b2774ccc974664997c955b: Pulling from pantsbuild/pex/base
... truncated the output ...
#6 0.182 + addgroup --gid=20 staff
#6 0.216 addgroup: The group `staff' already exists.

And while I know what this means, I'm not sure what the appropriate fix is - should I create a separate user and a different group for it to be in on my dev machine?

jsirois · 2023-10-17T08:44:26Z

This is the script that's failing: https://github.com/pantsbuild/pex/blob/main/docker/user/create_docker_image_user.sh

It runs in the context of building a docker image via: https://github.com/pantsbuild/pex/blob/a43c48c2a4b18b352be2c3175e0ae363ff8876d7/docker/user/Dockerfile#L4-L11

And those Dockerfile ARGs are fed here: https://github.com/pantsbuild/pex/blob/a43c48c2a4b18b352be2c3175e0ae363ff8876d7/dtox.sh#L41-L51

So, if you want to mess around and get that working for you, have at it.

That said, using dtox.sh is just intended to make development easier. It's giving you troubles though; so perhaps just install tox on your machine however you normally install Python tools and run tox -e... instead of ./dtox.sh -e....

I'll continue to be out of country and AFK through November 2nd. Hopefully you've found success by then, but, if not, I'll check back in.

jsirois · 2023-11-06T05:20:18Z

@erikwilliamson I wanted to check in and make sure you had what you needed to run tox -epyXY-integration -- -ktest_inject_env_and_args (#2260 (comment)). No rush of course, but without being able to run that it might be hard to work on adding a test.

erikwilliamson · 2023-11-06T19:13:48Z

Hey, I tried to join the Pants/Pex Slack but after filling out the questionnaire it's saying that the link has expired - I wondered if Slack might be a good place for me to ask a few questions rather than here. I'm able to run the tests no problem; however knowing whether the failures are valid now or if the tests need to be altered is perplexing :-)

jsirois · 2023-11-06T19:19:40Z

I'm not a part of the Pants slack. Since you now can run tests, if you can run tox -epyXY-integration -- -k your_new_test_name, even if that fails for perplexing reasons, it's probably time to add the test to this PR. Once you do that I'll approve the CI workflow and we can have a shared failure experience to talk about.

erikwilliamson · 2023-11-06T21:29:57Z

Learning as I go here! I saw that that there's pex/venv/installer.py which had environment variable injection happening. Tests are passing for me now - Thanks for your patience and I look forward to your thoughts.

jsirois

@erikwilliamson this LGTM mod commented out tests. I'll let CI fly, but please circle back to get those un-commented / updated to work with this change.

jsirois · 2023-11-06T22:38:56Z

For the formatting shard, tox -eformat-run,typecheck is what you want to pass locally.

erikwilliamson · 2023-11-06T22:47:13Z

I'm getting this on OSX 14.1 - I can fire up a linux VM to make it go away in the meantime though

Typechecking 338 files using Python 3.11.6 against Python 3.12 ...
pex/compatibility.py: note: In function "cpu_count":
pex/compatibility.py:163:23: error: Module has no attribute
"sched_getaffinity" [attr-defined]
cpu_set = os.sched_getaffinity(0)

jsirois · 2023-11-06T22:53:29Z

@erikwilliamson if that's the only error, you should be good to go since it was only the format check that failed in CI (https://github.com/pantsbuild/pex/actions/runs/6776741887/job/18420675544?pr=2260#step:4:160) and not the type check.

... and I <3 macOS.

kaos · 2023-11-06T22:56:35Z

... and I <3 macOS.

soon I'll believe you 😂

jsirois

Thanks @erikwilliamson, this looks good with two small fixes.

jsirois · 2023-11-07T02:37:52Z

tests/integration/test_inject_env_and_args.py

-    assert {"args": ["foo", "bar"], "MESSAGE": None} == json.loads(
+    # Switching away from the built-in entrypoint should disable injected args but not the en env.
+    assert {"args": ["foo", "bar"], "MESSAGE": "Hello, world!"} == json.loads(
+        # assert {"MESSAGE": "Hello, world!"} == json.loads(


Kill this commented out line.

jsirois · 2023-11-07T02:38:57Z

tests/integration/test_inject_env_and_args.py

@@ -240,8 +240,9 @@ def assert_message(
    assert_message(b"Hello, world!")
    assert_message(b"42", MESSAGE="42")

-    # Switching away from the built-in entrypoint should disable injected args and env.
-    assert {"args": ["foo", "bar"], "MESSAGE": None} == json.loads(
+    # Switching away from the built-in entrypoint should disable injected args but not the en env.


Fix the typo at the end of the sentence. Should be: ... not the env.

pex/venv/installer.py

…T is defined and run

jsirois · 2023-11-07T18:55:06Z

Alright @erikwilliamson this is now available in Pex 2.1.151:

Thanks for your contribution.

erikwilliamson · 2023-11-07T19:23:43Z

Thanks so much for guiding me, @jsirois ! Your help and patience were greatly appreciated.

relocate environment variable injection to before the interpreter is run

b1a0046

erikwilliamson mentioned this pull request Oct 2, 2023

I'm missing something: --inject-env isn't injecting #2224

Closed

Merge branch 'pantsbuild:main' into relocate-environment-injection

e71813f

Merge branch 'pantsbuild:main' into relocate-environment-injection

d4530fd

Erik Williamson and others added 3 commits November 6, 2023 16:27

Update tests to reflect relocation of where we're injecting ENV vars

fdbdf08

relocating env var injection

632b592

move argument injection back to where it came from

b5c1e23

jsirois reviewed Nov 6, 2023

View reviewed changes

erikwilliamson added 2 commits November 6, 2023 18:43

updated to work with relocation of env var injection

0096b2c

missed uncommenting test_inject_env_invalid()

29a7925

jsirois approved these changes Nov 7, 2023

View reviewed changes

jsirois mentioned this pull request Nov 7, 2023

Release 2.1.151 #2277

Closed

2 tasks

fix typo & remove commented out code

0c62995

jsirois reviewed Nov 7, 2023

View reviewed changes

pex/venv/installer.py Outdated Show resolved Hide resolved

move environment variable injection up more for cases where PEX_SCRIP…

840d601

…T is defined and run

jsirois merged commit 4089022 into pex-tool:main Nov 7, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relocate environment variable injection to before the interpreter is run #2260

Relocate environment variable injection to before the interpreter is run #2260

erikwilliamson commented Oct 2, 2023

jsirois commented Oct 2, 2023

ghost commented Oct 16, 2023

jsirois commented Oct 17, 2023

jsirois commented Nov 6, 2023

erikwilliamson commented Nov 6, 2023

jsirois commented Nov 6, 2023

erikwilliamson commented Nov 6, 2023

jsirois left a comment

jsirois commented Nov 6, 2023

erikwilliamson commented Nov 6, 2023

jsirois commented Nov 6, 2023

kaos commented Nov 6, 2023

jsirois left a comment

jsirois Nov 7, 2023

jsirois Nov 7, 2023

jsirois commented Nov 7, 2023

erikwilliamson commented Nov 7, 2023

Relocate environment variable injection to before the interpreter is run #2260

Relocate environment variable injection to before the interpreter is run #2260

Conversation

erikwilliamson commented Oct 2, 2023

jsirois commented Oct 2, 2023

ghost commented Oct 16, 2023

jsirois commented Oct 17, 2023

jsirois commented Nov 6, 2023

erikwilliamson commented Nov 6, 2023

jsirois commented Nov 6, 2023

erikwilliamson commented Nov 6, 2023

jsirois left a comment

Choose a reason for hiding this comment

jsirois commented Nov 6, 2023

erikwilliamson commented Nov 6, 2023

jsirois commented Nov 6, 2023

kaos commented Nov 6, 2023

jsirois left a comment

Choose a reason for hiding this comment

jsirois Nov 7, 2023

Choose a reason for hiding this comment

jsirois Nov 7, 2023

Choose a reason for hiding this comment

jsirois commented Nov 7, 2023

erikwilliamson commented Nov 7, 2023