Venv PEXes lie about absolutifying PEX_ROOT

[Eric-Arellano]

As shown by the test at https://github.com/pantsbuild/pants/pull/14585/files#diff-14558645c06441dee2566b7974aeab888b1d6709791b19063ea9bd93d4711584, our code to absolutify via BASH_SOURCE is not working - it results in paths like ./.cache/pex_root, rather than /absolute/path/to/.cache/pex_root.

It appears this assumption is broken:

pants/src/python/pants/backend/python/util_rules/pex.py

Lines 631 to 632 in e970932

	# 2004. In turn, our use of BASH_SOURCE relies on the fact that this script is executed
	# by the engine via its absolute path.

This is broken on macOS and Linux.

I have not started bisecting when this broke.

Alternatives

FindBinary

We could use FindBinary to find pwd and use that. But the downside is performance. Even though it gets memoized, this does slow down Pants on cold runs. It'd be neat if we can get BASH_SOURCE working again.

Rewrite to Python

If we use Python scripts, we can use os.getcwd(). A benefit is this is a syscall, rather than external process like /bin/pwd.

Another benefit is we can rewrite our Go script to not use /bin/mkdir.

Rust absolutifies argv0

Something like #11452. I'm not sure how to deal with remote execution runner.

[jsirois]

Keep in mind the code clearly? works. If so the broken thing is the comment. If you un-"absolutifiy" the current code, I'm pretty sure things break. If so, then the code is doing something important and the comment is just wrong.

[stuhood]

IMO, it's time to start migrating away from bash for cases like this. #13340 added support for consuming PythonBinary in the core: these types of scripts quickly grow out of control in bash, and "get cwd" would just be os.getcwd.

[jsirois]

Cases like this lose a bunch of hard won ms by not using bash. Please keep that in mind and call the shot that sacrificing those gains is intended when switching to python.

[Eric-Arellano]

Keep in mind the code clearly? works.

PEX works with PEX_ROOT being a relative path. PyOxidizer does not, which is why #14585 is failing, yet this code has "worked" for months w/ PEX.

The unit test show the bash function is not working how we want. It relativizes, it does not absolutify. Try running ./pants test src/python/pants/engine/process_test.py -- -k ensure_absolute.

[Eric-Arellano]

Cases like this loose a bunch of hard won ms by using bash. Please keep that in mind and call the shot that sacrificing those gains is intended when switching to python.

Ack. Does having Rust absolutify argv0 sound reasonable?

[jsirois]

Eric, my point is this code fixed a bug when introduced by Benjy that is a tested fix:
#12347

My change refactored:
#12479

So Benjy's fix is still fixed by test. The comment is wrong though. And - as said - if you undo the wrongly named absoluting function I'm almost positive the tests Benjy added re-fail. That's what I recall. I generally don't write a bunch of code for the hell of it.

[Eric-Arellano]

A simpler reproduction. On main:

diff --git a/src/python/pants/backend/python/util_rules/pex.py b/src/python/pants/backend/python/util_rules/pex.py
index c08868ca8..d0f96e57c 100644
--- a/src/python/pants/backend/python/util_rules/pex.py
+++ b/src/python/pants/backend/python/util_rules/pex.py
@@ -645,6 +645,10 @@ class VenvScriptWriter:
             export {" ".join(env_vars)}
             export PEX_ROOT="$(ensure_absolute ${{PEX_ROOT}})"
 
+            echo $ABS_SANDBOX_ROOT
+            echo $PEX_ROOT
+            die 1
+
             execute_pex_args="{execute_pex_args}"
             target_venv_executable="$(ensure_absolute {target_venv_executable})"
             venv_dir="$(ensure_absolute {venv_dir})"

Then run ./pants:

pants.engine.process.ProcessExecutionFailure: Process 'Extracting plugin locations' failed with exit code 127.
stdout:
.
./.cache/pex_root

[jsirois]

Ack. Does having Rust absolutify argv0 sound reasonable?

It's not clear that does the trick here. Other things in the script are (not!) absolutified but apparently dotted which works?.

[stuhood]

There are two questions then.

1. What to do for the venv case, which is latency sensitive, and seemingly not-broken (unless the tests are broken),
2. What to do for other cases, which might be less latency sensitive.

Assuming that the tests are not broken, then leaving 1 as is seems fine. For 2 (and for non-trivial script-processes in general) I'd suggest using Python.

[jsirois]

For 2 (and for non-trivial script-processes in general) I'd suggest using Python.

Totally agreed. For cases other than this issue, it probably makes sense. For this case that script already exists - its the PEX bootstrap script inside PEX!, which proved too slow.

[jsirois]

So here is what the current code is doing, I ~comment it out and Benjy's test fails:

$ git diff
diff --git a/src/python/pants/backend/python/util_rules/pex.py b/src/python/pants/backend/python/util_rules/pex.py
index 632adf61a..1d895d618 100644
--- a/src/python/pants/backend/python/util_rules/pex.py
+++ b/src/python/pants/backend/python/util_rules/pex.py
@@ -638,7 +638,9 @@ class VenvScriptWriter:
                 if [ "${{value0:0:1}}" == "/" ]; then
                     echo "${{value0}}" "$@"
                 else
-                    echo "${{ABS_SANDBOX_ROOT}}/${{value0}}" "$@"
+                    echo "${{ABS_SANDBOX_ROOT}}/${{value0}}" "$@" > /tmp/debug.txt
+                    echo "${{value0}}" "$@" >> /tmp/debug.txt
+                    echo "${{value0}}" "$@"
                 fi
             }}
 

The test:

$ ./pants test src/python/pants/backend/python/util_rules/pex_test.py -- -vvsk test_pex_working_directory
...
20:07:37.17 [ERROR] Completed: Run Pytest - src/python/pants/backend/python/util_rules/pex_test.py:tests failed (exit code 1).
============================= test session starts ==============================
collecting ... collected 26 items / 24 deselected / 2 selected

src/python/pants/backend/python/util_rules/pex_test.py::test_pex_working_directory[Pex] PASSED
src/python/pants/backend/python/util_rules/pex_test.py::test_pex_working_directory[VenvPex] FAILED
...
E           pants.engine.internals.scheduler.ExecutionError: 1 Exception encountered:
E           
E           Engine traceback:
E             in select
E             in pants.engine.process.fallible_to_exec_result_or_raise
E           Traceback (most recent call last):
E             File "/tmp/process-executionlYOAlR/src/python/pants/engine/process.py", line 290, in fallible_to_exec_result_or_raise
E               process_cleanup=process_cleanup.val,
E           pants.engine.process.ProcessExecutionFailure: Process 'Run the pex and check its cwd' failed with exit code 2.
E           stdout:
E           
E           stderr:
E           /home/jsirois/.pyenv/versions/3.6.15/bin/python3.6: can't open file 'test.pex': [Errno 2] No such file or directory

And:

$ cat /tmp/debug.txt 
../../.cache/pex_root/venvs/7ff3b8bc8b03d3bddf3ed75191fe8c23552b7f4d/dddf6e1dc52f14c0b8a18330edfe1cc6242e8123
.cache/pex_root/venvs/7ff3b8bc8b03d3bddf3ed75191fe8c23552b7f4d/dddf6e1dc52f14c0b8a18330edfe1cc6242e8123

So the code needs to not be fixed, but the comment definitely does. When I have a clearer head I'll fix that up. Bad understanding is bad, and I clearly didn't understand what I was doing.