feat(py_venv): activate-less interpreter (#629)

This patch reworks how the interpreter shim operates to eliminate the
data dependency on the `activate` script having been evaluated and move
the parameters which previously came via the shell environment.

Because the behavior of `activate` isn't actually specified, we
previously assumed that it was safe to just extend `activate`. This
isn't true at all it turns out. Under normal operation the interpreter
initialization as implemented in `Modules/getpath.py` provides special
behavior when the interpreter's observed filename (`argv[0]`) is a
symlink as it is in a normal non-relocatable virtualenv. In this case
the interpreter and will search for a `pyvenv.cfg` relative to the
"interpreter" and implicitly activate the venv as needed.

Our `./bin/python` being an actual binary interrupts this which means
we're on our own to make `getpath` and `site` do the needful.

This should be as simple as setting the `home=` key in the `pyvenv.cfg`,
but there isn't a principled way other than going through the runfiles
library to locate where the interpreter lands in the runfiles tree
relative to the virtualenv. Ideally we'd set the `home=` key to a
relative path and mostly move on. Instead due in part to bzlmod munging
we have to do the repo mapping dance which can't be done statically. So
we need to do `rlocation` somewhere.

The solution this patch comes to is moving that `rlocation` logic out of
the `activate` script and into the launcher itself so that the launcher
can search for the needed `$PYTHONHOME` and desired interpreter without
the user having previously loaded the required parameters into the env
via `activate`.

Fixes #594.
Fixes #631.

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

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): yes
- Suggested release notes appear below: yes

The `py_venv_binary` and friends no longer depend on their virtualenvs
being `activated` via the shell. Their interpreters can safely be
invoked directly as under an IDE or in a Spark environment.

The `py_venv_binary` now no longer includes NEITHER the user's
site-packages NOR the interpreter's site-packages directory unless the
`enable_system_site_packages` or `enable_user_site_packages` attributes
are explicitly set. This prevents accidental non-hermetic imports.

### Test plan

- Covered by existing test cases
- Manual testing; please provide instructions so we can reproduce:

```shellsession
❯ aspect build //py/tests/py-internal-venv:test && bazel-bin/py/tests/py-internal-venv/test.runfiles/aspect_rules_py/py/tests/py-internal-venv/.test/bin/python -c 'import sys; from pprint import pprint; pprint(sys.path)'
INFO: Analyzed target //py/tests/py-internal-venv:test (2 packages loaded, 922 targets configured).
INFO: From Compiling Rust bin shim_macos_aarch64_build (1 files):
warning: fields `cfg` and `version_info` are never read
  --> py/tools/venv_shim/src/main.rs:45:5
   |
43 | struct PyCfg {
   |        ----- fields in this struct
44 |     root: PathBuf,
45 |     cfg: PathBuf,
   |     ^^^
46 |     version_info: String,
   |     ^^^^^^^^^^^^
   |
   = note: `PyCfg` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

warning: 1 warning emitted

INFO: Found 1 target...
Target //py/tests/py-internal-venv:test up-to-date:
  bazel-bin/py/tests/py-internal-venv/test
INFO: Elapsed time: 10.289s, Critical Path: 6.72s
INFO: 4 processes: 1 internal, 3 darwin-sandbox.
INFO: Build completed successfully, 4 total actions
"bazel-bin/py/tests/py-internal-venv/test.runfiles/MANIFEST"
['',
 '/private/var/tmp/_bazel_arrdem/93bfea6cdc1153cc29a75400cd38823a/external/python_toolchain_aarch64-apple-darwin/lib/python39.zip',
 '/private/var/tmp/_bazel_arrdem/93bfea6cdc1153cc29a75400cd38823a/external/python_toolchain_aarch64-apple-darwin/lib/python3.9',
 '/private/var/tmp/_bazel_arrdem/93bfea6cdc1153cc29a75400cd38823a/external/python_toolchain_aarch64-apple-darwin/lib/python3.9/lib-dynload',
 '/Users/arrdem/Documents/work/aspect/rules_py/bazel-bin/py/tests/py-internal-venv/test.runfiles/aspect_rules_py/py/tests/py-internal-venv/.test/lib/python3.9/site-packages',
 '/Users/arrdem/Documents/work/aspect/rules_py/bazel-bin/py/tests/py-internal-venv/test.runfiles/aspect_rules_py/py/tests/py-internal-venv',
 '/Users/arrdem/Documents/work/aspect/rules_py/bazel-bin/py/tests/py-internal-venv/test.runfiles/aspect_rules_py']
```

### TODO
- [x] Need to add automated tests covering entrypoint bypass
- [x] Need to add automated tests covering the usersite behavior
- [x] Need to add automated tests covering the system site behavior

---------

Co-authored-by: Alex Payne <[email protected]>

Author: arrdem

.aspect/workflows/config.yaml

@@ -1,6 +1,7 @@
 # See https://docs.aspect.build/workflows/configuration
 tasks:
   - test:
+      queue: aspect-large
   - finalization:
       queue: aspect-small
 notifications:

Cargo.Bazel.lock

@@ -1,5 +1,5 @@
 {
-  "checksum": "81528401c6d6fde79ca22def4e50d7ebc47f7f182a83a106d38e9e1279da39a5",
+  "checksum": "f1be4baa5924515e3e3bfe91c4afbfa084b2751aec2f7560df597efbcdc4dc3b",
   "crates": {
     "addr2line 0.24.2": {
       "name": "addr2line",
@@ -14740,6 +14740,37 @@
       ],
       "license_file": "LICENSE"
     },
+    "runfiles 0.1.0": {
+      "name": "runfiles",
+      "version": "0.1.0",
+      "package_url": "https://github.com/aspect-build/rules_py",
+      "repository": null,
+      "targets": [
+        {
+          "Library": {
+            "crate_name": "runfiles",
+            "crate_root": "src/lib.rs",
+            "srcs": {
+              "allow_empty": true,
+              "include": [
+                "**/*.rs"
+              ]
+            }
+          }
+        }
+      ],
+      "library_target_name": "runfiles",
+      "common_attrs": {
+        "compile_data_glob": [
+          "**"
+        ],
+        "edition": "2021",
+        "version": "0.1.0"
+      },
+      "license": "Apache 2",
+      "license_ids": [],
+      "license_file": null
+    },
     "rust-netrc 0.1.1": {
       "name": "rust-netrc",
       "version": "0.1.1",
@@ -27114,6 +27145,7 @@
   "binary_crates": [],
   "workspace_members": {
     "py 0.1.0": "py/tools/py",
+    "runfiles 0.1.0": "py/tools/runfiles",
     "unpack_bin 0.1.0": "py/tools/unpack_bin",
     "venv_bin 0.1.0": "py/tools/venv_bin",
     "venv_shim 0.1.0": "py/tools/venv_shim"

Cargo.lock

@@ -5,6 +5,7 @@ members = [
     "py/tools/venv_bin",
     "py/tools/unpack_bin",
     "py/tools/venv_shim",
+    "py/tools/runfiles",
 ]
 
 [workspace.package]

Cargo.toml

@@ -87,6 +87,7 @@ crate.from_cargo(
         "//py/tools/unpack_bin:Cargo.toml",
         "//py/tools/venv_bin:Cargo.toml",
         "//py/tools/venv_shim:Cargo.toml",
+        "//py/tools/runfiles:Cargo.toml",
     ],
 )
 use_repo(crate, "crate_index")

MODULE.bazel

@@ -284,6 +284,7 @@ crates_repository(
         "//py/tools/unpack_bin:Cargo.toml",
         "//py/tools/venv_bin:Cargo.toml",
         "//py/tools/venv_shim:Cargo.toml",
+        "//py/tools/runfiles:Cargo.toml",
     ],
 )
 

WORKSPACE

@@ -39,6 +39,7 @@ crates_repository(
         "@aspect_rules_py//py/tools/unpack_bin:Cargo.toml",
         "@aspect_rules_py//py/tools/venv_bin:Cargo.toml",
         "@aspect_rules_py//py/tools/venv_shim:Cargo.toml",
+        "@aspect_rules_py//py/tools/runfiles:Cargo.toml",
     ],
 )
 

e2e/smoke/WORKSPACE.bazel

@@ -118,6 +118,10 @@ def _py_venv_base_impl(ctx):
         arguments = [
             "--location=" + venv_dir.path,
             "--venv-shim=" + py_shim.bin.bin.path,
+            # Post-bzlmod we need to record the current repository in case the
+            # user tries to consume a `py_venv_binary` across repo boundaries
+            # which could cause repo mapping to become relevant.
+            "--repo=" + (ctx.label.repo_name or ctx.workspace_name),
             "--python=" + to_rlocation_path(ctx, py_toolchain.python) if py_toolchain.runfiles_interpreter else py_toolchain.python.path,
             "--pth-file=" + site_packages_pth_file.path,
             "--env-file=" + env_file.path,
@@ -129,6 +133,14 @@ def _py_venv_base_impl(ctx):
                 py_toolchain.interpreter_version_info.major,
                 py_toolchain.interpreter_version_info.minor,
             ),
+            "--include-system-site-packages=" + ({
+                True: "true",
+                False: "false",
+            }[ctx.attr.include_system_site_packages]),
+            "--include-user-site-packages=" + ({
+                True: "true",
+                False: "false",
+            }[ctx.attr.include_system_site_packages]),
         ] + (["--debug"] if ctx.attr.debug else []),
         inputs = rfs.merge_all([
             ctx.runfiles(files = [
@@ -290,6 +302,42 @@ A collision can occur when multiple packages providing the same file are install
     "debug": attr.bool(
         default = False,
     ),
+    "include_system_site_packages": attr.bool(
+        default = False,
+        doc = """`pyvenv.cfg` feature flag for the `include-system-site-packages` key.
+
+When `True`, the user's site directory AND the interpreter's site directory will
+be included into the runtime pythonpath.
+
+When `False`, only the virtualenv's site directory and the interpreter's core
+libraries will be included into the runtime pythonpath.
+
+`False` is obviously preferable as it increases hermeticity, but the choice of
+`False` cases for instance a `pip` or `setuptools` bundled into the interpreter
+to be unusable. Many libraries assume these packages will always be available
+and may not reliably declare their dependencies such that Bazel will satisfy
+them, so choosing isolation could expose packaging errors.
+
+""",
+    ),
+    "include_user_site_packages": attr.bool(
+        default = False,
+        doc = """`pyvenv.cfg` feature flag for the `aspect-include-user-site-packages` extension key.
+
+When `True`, the user's site directory directory will be included into the
+runtime pythonpath.
+
+When `False`, only the virtualenv's site directory and the interpreter's core
+libraries will be included into the runtime pythonpath.
+
+`False` is obviously preferable as it increases hermeticity, but the choice of
+`False` cases for instance a `pip` or `setuptools` bundled into the interpreter
+to be unusable. Many libraries assume these packages will always be available
+and may not reliably declare their dependencies such that Bazel will satisfy
+them, so choosing isolation could expose packaging errors.
+
+""",
+    ),
 })
 
 _attrs.update(**_py_library.attrs)

py/private/py_venv/py_venv.bzl

@@ -0,0 +1,16 @@
+load("//py/private/py_venv:defs.bzl", "py_venv_test")
+
+py_venv_test(
+    name = "test",
+    srcs = [
+        "test.py",
+    ],
+    imports = [
+        ".",
+    ],
+    include_system_site_packages = False,
+    main = "test.py",
+    deps = [
+        "@pypi_cowsay//:pkg",
+    ],
+)

py/tests/py-venv-disable-systemsite/BUILD.bazel

@@ -0,0 +1,17 @@
+import os
+import sys
+import site
+
+print("---")
+print("__file__:", __file__)
+print("sys.prefix:", sys.prefix)
+print("sys.executable:", sys.executable)
+print("site.PREFIXES:")
+for p in site.PREFIXES:
+    print(" -", p)
+
+# The virtualenv module should have already been loaded at interpreter startup
+assert "_virtualenv" in sys.modules
+print(site.PREFIXES)
+
+assert len(site.getsitepackages()) == 1