fix(py_venv): Repair runfiles-relative venv roots (#579)

Due to differences between the MacOS and Linux implementations of
Bazel's sandboxing (reported as #573) the current implementation of
manipulating `argv[0]` to make an interpreter see itself as having a
path of `.runfiles/.../.venv/bin/python3` isn't effective.

Specifically on Linux it appears that we're more eagerly resolving
symlinks.

This means that `bazel-bin/.../*.runfiles/*/.venv/bin/python3` is being
immediately resolved to an `/execroot/bazel-bin/...` entry which
_contains only generated files_. An interpreter with relative path based
import roots within the execroot will be able to import relocated
sources (copied into the venv) but will not be able to import from
source files or external generated files.

Effectively spawning the interpreter is escaping the `.runfiles` tree
which breaks a bunch of our other assumptions.

The repair is to come up with a more portable way to identify the
interpreter without escaping from the runfiles sandbox.

As a solution this PR makes the interpreter shim depend both on the
`$VIRTUAL_ENV` variable which also serves as an anchor to the "real"
`.runfiles` tree. Due to the removals of `realpath` calls in the
`activate` script, it's possible that we could provide fallback behavior
by introspecting `$0`, but that verges on #581 so punting for now.

---

### Changes are visible to end-users: no

### Test plan

- [x] Add imports from unmodified sources to the test cases

Author: arrdem

py/private/py_venv/py_venv.bzl

@@ -57,19 +57,13 @@ def _py_venv_base_impl(ctx):
 
     # Check for duplicate virtual dependency names. Those that map to the same resolution target would have been merged by the depset for us.
     virtual_resolution = _py_library.resolve_virtuals(ctx)
+
+    # Note that this adds the workspace root for us (sigh), don't need to add to it
     imports_depset = _py_library.make_imports_depset(ctx, extra_imports_depsets = virtual_resolution.imports)
 
     pth_lines = ctx.actions.args()
     pth_lines.use_param_file("%s", use_always = True)
     pth_lines.set_param_file_format("multiline")
-
-    # FIXME: This was hardcoded in the original rule_py venv and is preserved
-    # for compatibility. Repo-absolute imports are Bad (TM) and shouldn't be on
-    # by default. I believe that as of recent rules_python, creating these
-    # repo-absolute imports is handled as part of the PyInfo calculation. If we
-    # get this from rules_python, it should be removed. Or it should be moved so
-    # that we calculate it as part of the imports depset logic.
-    pth_lines.add(".")
     pth_lines.add_all(imports_depset)
 
     site_packages_pth_file = ctx.actions.declare_file("{}.pth".format(ctx.attr.name))

py/tests/py_venv_conflict/BUILD.bazel

@@ -1,5 +1,16 @@
 load("@bazel_skylib//rules:build_test.bzl", "build_test")
-load("//py/unstable:defs.bzl", "py_venv")
+load("//py:defs.bzl", "py_library")
+load("//py/unstable:defs.bzl", "py_venv", "py_venv_test")
+
+py_library(
+    name = "lib",
+    srcs = [
+        "lib.py",
+    ],
+    imports = [
+        "..",
+    ],
+)
 
 py_venv(
     name = "test_venv_error",
@@ -9,6 +20,7 @@ py_venv(
         "manual",
     ],
     deps = [
+        ":lib",
         "//py/tests/py_venv_conflict/a",
         "//py/tests/py_venv_conflict/b",
     ],
@@ -18,6 +30,7 @@ py_venv(
     name = "test_venv_warning",
     package_collisions = "warning",
     deps = [
+        ":lib",
         "//py/tests/py_venv_conflict/a",
         "//py/tests/py_venv_conflict/b",
     ],
@@ -27,6 +40,7 @@ py_venv(
     name = "test_venv_ignore",
     package_collisions = "ignore",
     deps = [
+        ":lib",
         "//py/tests/py_venv_conflict/a",
         "//py/tests/py_venv_conflict/b",
     ],
@@ -39,3 +53,10 @@ build_test(
         ":test_venv_ignore",
     ],
 )
+
+py_venv_test(
+    name = "validate_import_roots",
+    srcs = ["test_import_roots.py"],
+    main = "test_import_roots.py",
+    venv = ":test_venv_ignore",
+)

py/tests/py_venv_conflict/lib.py

@@ -0,0 +1,4 @@
+#!/usr/bin/env python3
+
+def add(a, b):
+    return a + b

py/tests/py_venv_conflict/test_import_roots.py

@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+
+import os
+for k, v in os.environ.items():
+    if k.startswith("BUILD_") or k.startswith("RUNFILES_"):
+        print(k, ":", v)
+
+print("---")
+
+from pathlib import Path
+
+# prefix components:
+space =  '    '
+branch = '|   '
+# pointers:
+tee =    '+-- '
+last =   '+-- '
+
+
+def tree(dir_path: Path, prefix: str=''):
+    """A recursive generator, given a directory Path object
+    will yield a visual tree structure line by line
+    with each line prefixed by the same characters
+    """    
+    contents = list(dir_path.iterdir())
+    # contents each get pointers that are ├── with a final └── :
+    pointers = [tee] * (len(contents) - 1) + [last]
+    for pointer, path in zip(pointers, contents):
+        yield prefix + pointer + path.name
+        if path.is_dir(): # extend the prefix and recurse:
+            extension = branch if pointer == tee else space 
+            # i.e. space because last, └── , above so no more |
+            yield from tree(path, prefix=prefix+extension)
+
+here = Path(".")
+print(here.absolute().resolve())
+for line in tree(here):
+    print(line)
+
+print("---")
+
+import sys
+for e in sys.path:
+    print("-", e)
+
+print("---")
+
+print(sys.prefix)
+
+import conflict
+print(conflict.__file__)
+assert conflict.__file__.startswith(sys.prefix)
+
+import noconflict
+print(noconflict.__file__)
+assert noconflict.__file__.startswith(sys.prefix)
+
+import py_venv_conflict.lib as srclib
+print(srclib.__file__)
+assert not srclib.__file__.startswith(sys.prefix)

py/tests/py_venv_image_layer/BUILD.bazel

@@ -45,7 +45,7 @@ platform_transition_filegroup(
 )
 
 assert_tar_listing(
-    name = "my_app_amd64_layers_test",
+    name = "my_app_amd64_layers",
     actual = [":amd64_layers"],
     expected = ":my_app_amd64_layers_listing.yaml",
 )
@@ -57,7 +57,7 @@ platform_transition_filegroup(
 )
 
 assert_tar_listing(
-    name = "my_app_arm64_layers_test",
+    name = "my_app_arm64_layers",
     actual = [":arm64_layers"],
     expected = ":my_app_arm64_layers_listing.yaml",
 )

py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml

@@ -2442,7 +2442,7 @@ files:
 layer: 1
 files:
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/
-  - -rwxr-xr-x  0 0      0         356 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_aspect.pth
+  - -rwxr-xr-x  0 0      0         328 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_aspect.pth
   - -rwxr-xr-x  0 0      0          19 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_virtualenv.pth
   - -rwxr-xr-x  0 0      0        4342 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_virtualenv.py
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/colorama-0.4.6.dist-info/
@@ -2510,10 +2510,10 @@ files:
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/
-  - -rwxr-xr-x  0 0      0        7827 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/activate
-  - -rwxr-xr-x  0 0      0      813320 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python
-  - -rwxr-xr-x  0 0      0      813320 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3
-  - -rwxr-xr-x  0 0      0      813320 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3.9
+  - -rwxr-xr-x  0 0      0        8099 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/activate
+  - -rwxr-xr-x  0 0      0      817416 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python
+  - -rwxr-xr-x  0 0      0      817416 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3
+  - -rwxr-xr-x  0 0      0      817416 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3.9
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/
   - -rwxr-xr-x  0 0      0         323 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/pyvenv.cfg

py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml

@@ -2423,7 +2423,7 @@ files:
 layer: 1
 files:
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/
-  - -rwxr-xr-x  0 0      0         356 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_aspect.pth
+  - -rwxr-xr-x  0 0      0         328 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_aspect.pth
   - -rwxr-xr-x  0 0      0          19 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_virtualenv.pth
   - -rwxr-xr-x  0 0      0        4342 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/_virtualenv.py
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/site-packages/colorama-0.4.6.dist-info/
@@ -2491,10 +2491,10 @@ files:
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/
-  - -rwxr-xr-x  0 0      0        7828 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/activate
-  - -rwxr-xr-x  0 0      0      693968 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python
-  - -rwxr-xr-x  0 0      0      693968 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3
-  - -rwxr-xr-x  0 0      0      693968 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3.9
+  - -rwxr-xr-x  0 0      0        8100 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/activate
+  - -rwxr-xr-x  0 0      0      698064 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python
+  - -rwxr-xr-x  0 0      0      698064 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3
+  - -rwxr-xr-x  0 0      0      698064 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/bin/python3.9
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/
   - drwxr-xr-x  0 0      0           0 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/lib/python3.9/
   - -rwxr-xr-x  0 0      0         323 Jan  1  2023 ./py/tests/py_venv_image_layer/my_app_bin.runfiles/aspect_rules_py/py/tests/py_venv_image_layer/.my_app_bin/pyvenv.cfg

py/tools/py/src/activate.tmpl

@@ -58,9 +58,15 @@ deactivate nondestructive
 # The runfiles library code has some deps on this so we just set it :/
 : "${BASH_SOURCE:=$0}"
 
-VIRTUAL_ENV="$(realpath "$(dirname "$(dirname "${BASH_SOURCE}")")")"
+VIRTUAL_ENV="$(dirname "$(dirname "${BASH_SOURCE}")")"
 export VIRTUAL_ENV
 
+# HACK: (Ab)use the MacOS $PYTHONEXECUTABLE to record the `.runfiles`-relative
+# interpreter path. This helps us avoid issues with the interpreter's path being
+# `realpath`-ed in such a way that it escapes the `.runfiles` tree.
+PYTHONEXECUTABLE="${VIRTUAL_ENV}/bin/python"
+export PYTHONEXECUTABLE
+
 # unset PYTHONHOME if set
 # this will fail if PYTHONHOME is set to the empty string (which is bad anyway)
 # could use `if (set -u; : $PYTHONHOME) ;` in bash.

py/tools/py/src/runfiles_interpreter.tmpl

@@ -80,7 +80,7 @@ source "${RUNFILES_DIR:-/dev/null}/${f}" 2>/dev/null || \
 } >/dev/null
 
 # Look up the runfiles-based interpreter and put its dir _first_ on the path.
-INTERPRETER="$(realpath $(rlocation {{INTERPRETER_TARGET}}))"
+INTERPRETER="$(rlocation {{INTERPRETER_TARGET}})"
 
 # Figure out if we're dealing with just some program or a real install
 # <SOMEDIR> <- possible $PYTHONHOME