From d80279bc74a12a6f2d9f82ac6babce578b259a4e Mon Sep 17 00:00:00 2001 From: "Reid D. McKenzie" Date: Tue, 2 Sep 2025 14:47:24 -0600 Subject: [PATCH 1/6] chore(#610): Create a test case --- MODULE.bazel | 6 ++++++ py/tests/rpy610/BUILD.bazel | 16 ++++++++++++++ py/tests/rpy610/subrepo/BUILD.bazel | 10 +++++++++ py/tests/rpy610/subrepo/MODULE.bazel | 0 py/tests/rpy610/subrepo/foo.py | 2 ++ py/tests/rpy610/test.py | 32 ++++++++++++++++++++++++++++ py/tools/py/src/venv.rs | 10 +++++++-- 7 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 py/tests/rpy610/BUILD.bazel create mode 100644 py/tests/rpy610/subrepo/BUILD.bazel create mode 100644 py/tests/rpy610/subrepo/MODULE.bazel create mode 100644 py/tests/rpy610/subrepo/foo.py create mode 100644 py/tests/rpy610/test.py diff --git a/MODULE.bazel b/MODULE.bazel index 920b210c..387b4732 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -114,3 +114,9 @@ oci.pull( tag = "latest", ) use_repo(oci, "ubuntu", "ubuntu_linux_amd64", "ubuntu_linux_arm64_v8") + +local_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository") +local_repository( + name = "rpy610_test", + path = "./py/tests/rpy610/subrepo", +) diff --git a/py/tests/rpy610/BUILD.bazel b/py/tests/rpy610/BUILD.bazel new file mode 100644 index 00000000..ddf3c975 --- /dev/null +++ b/py/tests/rpy610/BUILD.bazel @@ -0,0 +1,16 @@ +load("//py/private/py_venv:defs.bzl", "py_venv_test") + +py_venv_test( + name = "test", + srcs = [ + "test.py", + ], + imports = [ + ".", + ], + main = "test.py", + deps = [ + "@pypi_cowsay//:pkg", + "@rpy610_test//:foo" + ], +) diff --git a/py/tests/rpy610/subrepo/BUILD.bazel b/py/tests/rpy610/subrepo/BUILD.bazel new file mode 100644 index 00000000..8e0b15fe --- /dev/null +++ b/py/tests/rpy610/subrepo/BUILD.bazel @@ -0,0 +1,10 @@ +load("@aspect_rules_py//py:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = [ + "foo.py" + ], + imports = ["."], + visibility = ["//visibility:public"] +) diff --git a/py/tests/rpy610/subrepo/MODULE.bazel b/py/tests/rpy610/subrepo/MODULE.bazel new file mode 100644 index 00000000..e69de29b diff --git a/py/tests/rpy610/subrepo/foo.py b/py/tests/rpy610/subrepo/foo.py new file mode 100644 index 00000000..d0aabc61 --- /dev/null +++ b/py/tests/rpy610/subrepo/foo.py @@ -0,0 +1,2 @@ +def foo(x): + return x ** 3.15 diff --git a/py/tests/rpy610/test.py b/py/tests/rpy610/test.py new file mode 100644 index 00000000..fd0befb2 --- /dev/null +++ b/py/tests/rpy610/test.py @@ -0,0 +1,32 @@ +#!/usr/bin/env python3 + +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 + +# Note that we can't assume that a `.runfiles` tree has been created as CI may +# use a different layout. + +# The virtualenv changes the sys.prefix, which should be in our runfiles +assert sys.prefix.endswith("/py/tests/rpy610/.test") + +# That prefix should also be "the" prefix per site.PREFIXES +assert site.PREFIXES[0].endswith("/py/tests/rpy610/.test") + +# The virtualenv also changes the sys.executable (if we've done this right) +assert sys.executable.find("/py/tests/rpy610/.test/bin/python") != -1 + +# aspect-build/rules_py#610, these imports aren't quite right +import foo +print(foo.__file__) diff --git a/py/tools/py/src/venv.rs b/py/tools/py/src/venv.rs index b5de92a7..1da72949 100644 --- a/py/tools/py/src/venv.rs +++ b/py/tools/py/src/venv.rs @@ -483,7 +483,7 @@ pub fn populate_venv_with_copies( .into_diagnostic()?; for line in BufReader::new(source_pth).lines().map_while(Result::ok) { - #[cfg(feature = "debug")] + //#[cfg(feature = "debug")] eprintln!("Got pth line {}", &line); let line = line.trim().to_string(); @@ -548,6 +548,13 @@ pub fn populate_venv_with_copies( // // [1] https://github.com/python/cpython/blob/ce31ae5209c976d28d1c21fcbb06c0ae5e50a896/Lib/site.py#L215 + // aspect-build/rules_py#610 + // + // While these relative paths seem to work fine for _internal_ + // runfiles within the `_main` workspace, problems occur when we + // try to take relative paths to _other_ workspaces because bzlmod + // may munge the directory names to be something that doesn't + // exist. let path_to_runfiles = diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir)).unwrap(); @@ -561,7 +568,6 @@ pub fn populate_venv_with_copies( } } - //Err(miette!("Failing for debug purposes")) Ok(()) } From 5547478efc652acae0a16c03014c9d4f2541c0a9 Mon Sep 17 00:00:00 2001 From: "Reid D. McKenzie" Date: Tue, 2 Sep 2025 16:08:16 -0600 Subject: [PATCH 2/6] fix(py_venv): External module imports repaired --- WORKSPACE | 5 ++++ py/tools/py/src/venv.rs | 49 ++++++++++++++++------------------- py/tools/venv_bin/src/main.rs | 20 +++++++++----- 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 4fa5c8ec..43e7c430 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -317,3 +317,8 @@ oci_pull( load("@container_structure_test//:repositories.bzl", "container_structure_test_register_toolchain") container_structure_test_register_toolchain(name = "cst") + +local_repository( + name = "rpy610_test", + path = "./py/tests/rpy610/subrepo", +) diff --git a/py/tools/py/src/venv.rs b/py/tools/py/src/venv.rs index 1da72949..a99d9831 100644 --- a/py/tools/py/src/venv.rs +++ b/py/tools/py/src/venv.rs @@ -445,6 +445,7 @@ aspect-runfiles-repo = {1} /// the depth of the `ip` then back down to the workspace-relative path of /// the target file. pub fn populate_venv_with_copies( + repo: &str, venv: Virtualenv, pth_file: PthFile, bin_dir: PathBuf, @@ -455,7 +456,6 @@ pub fn populate_venv_with_copies( // Get $PWD, which is the build working directory. let action_src_dir = current_dir().into_diagnostic()?; - let main_module = action_src_dir.file_name().unwrap(); let action_bin_dir = action_src_dir.join(bin_dir); #[cfg(feature = "debug")] @@ -483,7 +483,7 @@ pub fn populate_venv_with_copies( .into_diagnostic()?; for line in BufReader::new(source_pth).lines().map_while(Result::ok) { - //#[cfg(feature = "debug")] + #[cfg(feature = "debug")] eprintln!("Got pth line {}", &line); let line = line.trim().to_string(); @@ -496,12 +496,12 @@ pub fn populate_venv_with_copies( format!("{}/", line) }; - let Some((workspace, entry_path)) = line.split_once("/") else { + let Some((entry_repo, entry_path)) = line.split_once("/") else { return Err(miette!("Invalid path file entry!")); }; #[cfg(feature = "debug")] - eprintln!("Got pth entry @{}//{}", workspace, entry_path); + eprintln!("Got pth entry @{}//{}", entry_repo, entry_path); let mut entry = PathBuf::from(entry_path); @@ -511,9 +511,10 @@ pub fn populate_venv_with_copies( eprintln!("Entry is site-packages..."); // If the entry is external then we have to adjust the path - if workspace != main_module { + // FIXME: This isn't quite right outside of bzlmod + if entry_repo != repo { entry = PathBuf::from("external") - .join(PathBuf::from(workspace)) + .join(PathBuf::from(entry_repo)) .join(entry) } @@ -537,32 +538,25 @@ pub fn populate_venv_with_copies( } } } else { - // Need to insert an appropriate pth file entry. Pth file lines - // are relativized to the site dir [1] so here we need to take - // the path from the site dir back to the root of the runfiles - // tree and then append the entry to that relative path. - // - // This is the path from the venv's site-packages destination - // "back up to" the bazel-bin dir we're building into, plus one - // level. - // - // [1] https://github.com/python/cpython/blob/ce31ae5209c976d28d1c21fcbb06c0ae5e50a896/Lib/site.py#L215 - - // aspect-build/rules_py#610 - // - // While these relative paths seem to work fine for _internal_ - // runfiles within the `_main` workspace, problems occur when we - // try to take relative paths to _other_ workspaces because bzlmod - // may munge the directory names to be something that doesn't - // exist. - let path_to_runfiles = - diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir)).unwrap(); + if entry_repo != repo { + eprintln!("Warning: @@{entry_repo}//{entry_path} is not `site-packages`via pth rather than copy",) + } + // The path to the runfiles root is _one more than_ the relative + // oath from the venv's target dir to the root of the module + // containing the venv. + let path_to_runfiles = diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir)) + .unwrap() + .join(".."); writeln!(dest_pth_writer, "# @{}", line).into_diagnostic()?; writeln!( dest_pth_writer, "{}", - path_to_runfiles.join(entry).to_str().unwrap() + path_to_runfiles // .runfiles + .join(entry_repo) // ${REPO} + .join(entry_path) // ${PATH} + .to_str() + .unwrap() ) .into_diagnostic()?; } @@ -579,6 +573,7 @@ pub fn populate_venv_with_copies( #[expect(unused_variables)] pub fn populate_venv_with_pth( + repo: &str, venv: Virtualenv, pth_file: PthFile, bin_dir: PathBuf, diff --git a/py/tools/venv_bin/src/main.rs b/py/tools/venv_bin/src/main.rs index 52a50f5b..29128f07 100644 --- a/py/tools/venv_bin/src/main.rs +++ b/py/tools/venv_bin/src/main.rs @@ -130,10 +130,13 @@ fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> { return Err(miette!("Version must be provided for static venv modes")); }; + let repo = args + .repo + .as_deref() + .expect("The --repo argument is required for static venvs!"); + let venv = py::venv::create_empty_venv( - args.repo - .as_deref() - .expect("The --repo argument is required for static venvs!"), + repo, &args.python, py::venv::PythonVersionInfo::from_str(&version)?, &args.location, @@ -145,6 +148,7 @@ fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> { )?; py::venv::populate_venv_with_copies( + repo, venv, pth_file, args.bin_dir.unwrap(), @@ -160,10 +164,13 @@ fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> { return Err(miette!("Version must be provided for static venv modes")); }; + let repo = args + .repo + .as_deref() + .expect("The --repo argument is required for static venvs!"); + let venv = py::venv::create_empty_venv( - args.repo - .as_deref() - .expect("The --repo argument is required for static venvs!"), + repo, &args.python, py::venv::PythonVersionInfo::from_str(&version)?, &args.location, @@ -175,6 +182,7 @@ fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> { )?; py::venv::populate_venv_with_pth( + repo, venv, pth_file, args.bin_dir.unwrap(), From c8c36238cfeb86e6a1a206fc9defd466ddfb0146 Mon Sep 17 00:00:00 2001 From: "Reid D. McKenzie" Date: Tue, 2 Sep 2025 16:25:36 -0600 Subject: [PATCH 3/6] Update test image listings --- py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml | 2 +- py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml b/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml index 4eff121b..44ca9291 100644 --- a/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml +++ b/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 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 385 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/ diff --git a/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml b/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml index 4ab0374c..85a67d77 100644 --- a/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml +++ b/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 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 385 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/ From 038e6bc9791c3ef85ac60aacc08f15d9c7866299 Mon Sep 17 00:00:00 2001 From: "Reid D. McKenzie" Date: Tue, 2 Sep 2025 16:26:19 -0600 Subject: [PATCH 4/6] Format. --- MODULE.bazel | 1 + py/tests/rpy610/BUILD.bazel | 2 +- py/tests/rpy610/subrepo/BUILD.bazel | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 387b4732..41f9462f 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -116,6 +116,7 @@ oci.pull( use_repo(oci, "ubuntu", "ubuntu_linux_amd64", "ubuntu_linux_arm64_v8") local_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository") + local_repository( name = "rpy610_test", path = "./py/tests/rpy610/subrepo", diff --git a/py/tests/rpy610/BUILD.bazel b/py/tests/rpy610/BUILD.bazel index ddf3c975..ff5fd0d3 100644 --- a/py/tests/rpy610/BUILD.bazel +++ b/py/tests/rpy610/BUILD.bazel @@ -11,6 +11,6 @@ py_venv_test( main = "test.py", deps = [ "@pypi_cowsay//:pkg", - "@rpy610_test//:foo" + "@rpy610_test//:foo", ], ) diff --git a/py/tests/rpy610/subrepo/BUILD.bazel b/py/tests/rpy610/subrepo/BUILD.bazel index 8e0b15fe..f3d8cb46 100644 --- a/py/tests/rpy610/subrepo/BUILD.bazel +++ b/py/tests/rpy610/subrepo/BUILD.bazel @@ -3,8 +3,8 @@ load("@aspect_rules_py//py:defs.bzl", "py_library") py_library( name = "foo", srcs = [ - "foo.py" + "foo.py", ], imports = ["."], - visibility = ["//visibility:public"] + visibility = ["//visibility:public"], ) From 04e9d9b7e828102cbcf0f9c30bdf6f0f70581b11 Mon Sep 17 00:00:00 2001 From: "Reid D. McKenzie" Date: Mon, 8 Sep 2025 18:37:50 -0700 Subject: [PATCH 5/6] Implement an _aspect.py + bzlpth flow --- .../py-venv-standalone-interpreter/test.sh | 2 +- .../my_app_amd64_layers_listing.yaml | 4 +- .../my_app_arm64_layers_listing.yaml | 4 +- py/tools/py/BUILD.bazel | 1 + py/tools/py/src/_aspect.py | 34 +++ py/tools/py/src/venv.rs | 196 +++++++++--------- 6 files changed, 145 insertions(+), 96 deletions(-) create mode 100644 py/tools/py/src/_aspect.py diff --git a/py/tests/py-venv-standalone-interpreter/test.sh b/py/tests/py-venv-standalone-interpreter/test.sh index d3775b73..fb0d923e 100755 --- a/py/tests/py-venv-standalone-interpreter/test.sh +++ b/py/tests/py-venv-standalone-interpreter/test.sh @@ -4,7 +4,7 @@ set -ex ROOT="$(dirname $0)" -"$ROOT"/.ex/bin/python --help +"$ROOT"/.ex/bin/python --help >/dev/null 2>&1 if [ "Hello, world!" != "$($ROOT/.ex/bin/python -c 'from ex import hello; print(hello())')" ]; then exit 1 diff --git a/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml b/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml index 44ca9291..e7d27f64 100644 --- a/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml +++ b/py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml @@ -2442,7 +2442,9 @@ 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 385 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 416 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.bzlpth + - -rwxr-xr-x 0 0 0 15 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 1151 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.py - -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/ diff --git a/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml b/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml index 85a67d77..67075b22 100644 --- a/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml +++ b/py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml @@ -2423,7 +2423,9 @@ 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 385 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 416 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.bzlpth + - -rwxr-xr-x 0 0 0 15 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 1151 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.py - -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/ diff --git a/py/tools/py/BUILD.bazel b/py/tools/py/BUILD.bazel index 0c1a0c73..076e3f1a 100644 --- a/py/tools/py/BUILD.bazel +++ b/py/tools/py/BUILD.bazel @@ -9,6 +9,7 @@ rust_library( "src/venv.rs", ], data = [ + "src/_aspect.py", "src/_virtualenv.py", "src/activate.tmpl", "src/pyvenv.cfg.tmpl", diff --git a/py/tools/py/src/_aspect.py b/py/tools/py/src/_aspect.py new file mode 100644 index 00000000..f604fb01 --- /dev/null +++ b/py/tools/py/src/_aspect.py @@ -0,0 +1,34 @@ +"""Bazel runfiles appropriate pth extensions. + +Very carefully avoids resolving symlink path parts as doing so can result in +unintended escapes from runfiles sandbox trees. + +""" + +import os +import site + +site_dir = os.path.dirname(__file__) + +# FIXME: Are there other runfiles dir identification strategies that matter? +runfiles_dir = os.getenv("RUNFILES_DIR") +if not runfiles_dir: + p = site_dir + while p != "/": + if p.endswith(".runfiles"): + break + p = os.path.dirname(p) + else: + raise RuntimeError("Failed to identify the runfiles root by path traversal!") + runfiles_dir = p + +# Now that we have the runfiles root, the required additional site paths are +# just the join of the runfiles root and the already bzlmod-transformed roots +# provided by the `rules_py` venv creation code. Join them and add them. +with open(os.path.join(site_dir, "_aspect.bzlpth")) as fp: + for line in fp: + line = line.strip() + p = os.path.normpath(os.path.join(site_dir, line)) + # FIXME: Do we want to process embedded pth files? Or just insert + # the paths into the sys.path + site.addsitedir(p) diff --git a/py/tools/py/src/venv.rs b/py/tools/py/src/venv.rs index a99d9831..8c58116e 100644 --- a/py/tools/py/src/venv.rs +++ b/py/tools/py/src/venv.rs @@ -196,7 +196,7 @@ const RELOCATABLE_SHEBANG: &str = "\ /// ./python3.${VERSION_MINOR} d /// ./site-packages d /// ./_virtualenv.py t -/// ./_virtualenv.pth t +/// ./_00_virtualenv.pth t /// /// /// Issues: @@ -401,49 +401,41 @@ aspect-runfiles-repo = {1} Ok(venv) } -/// The tricky bit is that we then need to create _dangling at creation time_ -/// symlinks to where `.runfiles` entries _will_ be once the current action -/// completes. +/// Poppulate the virtualenv with files from installed packages. /// -/// The input `.pth` file specifies paths in the format +/// Bazel handles symlinks inside TreeArtifacts at least as of 8.4.0 and before +/// by converting them into copies of the link targets. This prevents us from +/// creating a forrest of symlinks directly as `rules_python` does. Instead we +/// settle for copying files out of the install external repos and into the venv. /// -/// / +/// This is inefficient but it does have the advantage of avoiding potential +/// issues around `realpath(__file__)` causing escapes from the venv root. /// -/// These paths can exist in one of four places -/// 1. `./` (source files in the same workspace) -/// 2. `./external` (source files in a different workspace) -/// 3. `./bazel-out/fastbuild/bin` (generated files in this workspace) -/// 4. `./bazel-out/fastbuild/bin/external` (generated files in a different workspace) +/// In order to handle internal imports (eg. not from external `pip`) we also +/// generate a `_aspect.bzlpth` file which contains Bazel label (label prefixes +/// technically) which can be materialized into import roots against the +/// runfiles structure at interpreter startup time. This allows for behavior +/// similar to the original `rules_python` strategy of just shoving a bunch of +/// stuff into the `$PYTHONPATH` while sidestepping issues around blowing up the +/// env/system arg limits. /// -/// In filling out a static symlink venv we have to: -/// -/// 0. Be told by the caller what a relative path _from the venv root_ back up to the -/// -/// 1. Go over every entry in the computed `.pth` list -/// -/// 2. Identify entries which end in `site-packages` -/// -/// 3. Compute a `.runfiles` time location for the root of that import -/// -/// 4. For each of the two possible roots of that import (./, ./bazel-bin/,) -/// walk the directory tree there -/// -/// 5. For every entry in that directory tree, take the path of that entry `e` -/// -/// 6. Relativeize the path of the entry to the import root, `ip` -/// -/// 7. Relativize the path of the entry to the `.runfiles` time workspace root `rp` -/// -/// 6. Create an _unverified_ _dangling_ symlink in the venv. -/// -/// At the time that we create these links the targets won't have been -/// emplaced yet. Bazel will create them when the `.runfiles` directory is -/// materialized by assembling all the input files. +/// The tree we want to create is as follows: /// -/// The link needs to go up the depth of the target plus one to drop `_main` -/// or any other workspace name plus four for the site-packages prefix plus -/// the depth of the `ip` then back down to the workspace-relative path of -/// the target file. +/// .// +/// ./pyvenv.cfg t +/// ./bin/ +/// ./python l ${PYTHON} +/// ./python3 l ./python +/// ./python3.${VERSION_MINOR} l ./python +/// ./lib d +/// ./python3.${VERSION_MINOR} d +/// ./site-packages d +/// ./_aspect.py t +/// ./_aspect.bzlpth t +/// ./ + +// TODO: Need to rework this so that what gets copied vs what gets added to the +// pth is controlled by some sort of pluggable policy. pub fn populate_venv_with_copies( repo: &str, venv: Virtualenv, @@ -464,24 +456,33 @@ pub fn populate_venv_with_copies( #[cfg(feature = "debug")] eprintln!("action_bin_dir: {}", &action_bin_dir.to_str().unwrap()); - let source_pth = File::open(pth_file.src.as_path()) - .into_diagnostic() - .wrap_err("Unable to open source .pth file")?; + // Add our own venv initialization plugin that's designed to handle the bzlpth mess + fs::write(&venv.site_dir.join("_aspect.pth"), "import _aspect\n").into_diagnostic()?; + fs::write( + &venv.site_dir.join("_aspect.py"), + include_str!("_aspect.py"), + ) + .into_diagnostic()?; - let dest_pth = File::create(dest.join("_aspect.pth")) + let dest_pth = File::create(dest.join("_aspect.bzlpth")) .into_diagnostic() - .wrap_err("Unable to create destination .pth file")?; + .wrap_err("Unable to create destination .bzlpth file")?; let mut dest_pth_writer = BufWriter::new(dest_pth); dest_pth_writer .write( b"\ -# Generated by Aspect py_binary -# Contains relative import paths to non site-package trees within the .runfiles +# Generated by Aspect py_venv_* +# Contains Bazel runfiles path suffixes to import roots +# See _aspect.py for the relevant processing machinery ", ) .into_diagnostic()?; + let source_pth = File::open(pth_file.src.as_path()) + .into_diagnostic() + .wrap_err("Unable to open source .pth file")?; + for line in BufReader::new(source_pth).lines().map_while(Result::ok) { #[cfg(feature = "debug")] eprintln!("Got pth line {}", &line); @@ -506,59 +507,68 @@ pub fn populate_venv_with_copies( let mut entry = PathBuf::from(entry_path); // FIXME: Handle other wheel install dirs like bin? - if entry.file_name() == Some(OsStr::new("site-packages")) { - #[cfg(feature = "debug")] - eprintln!("Entry is site-packages..."); - - // If the entry is external then we have to adjust the path - // FIXME: This isn't quite right outside of bzlmod - if entry_repo != repo { - entry = PathBuf::from("external") - .join(PathBuf::from(entry_repo)) - .join(entry) - } + match entry.file_name().map(|it| it.to_str().unwrap()) { + // FIXME: dist-packages is a Debian-ism that exists in some customer + // environments. It would be better if we could manage to make this + // decison a policy under user controll. Hard-coding for now. + Some("site-packages") | Some("dist-packages") => { + #[cfg(feature = "debug")] + eprintln!("Entry is site-packages..."); + + // If the entry is external then we have to adjust the path + // FIXME: This isn't quite right outside of bzlmod + if entry_repo != repo { + entry = PathBuf::from("external") + .join(PathBuf::from(entry_repo)) + .join(entry) + } - // Copy library sources in - for prefix in [&action_src_dir, &action_bin_dir] { - let src_dir = prefix.join(&entry); - if src_dir.exists() { - create_tree(&src_dir, &venv.site_dir, &collision_strategy)?; - } else { - #[cfg(feature = "debug")] - eprintln!("Unable to find srcs under {}", src_dir.to_str().unwrap()); + // Copy library sources in + for prefix in [&action_src_dir, &action_bin_dir] { + let src_dir = prefix.join(&entry); + if src_dir.exists() { + create_tree(&src_dir, &venv.site_dir, &collision_strategy)?; + } else { + #[cfg(feature = "debug")] + eprintln!("Unable to find srcs under {}", src_dir.to_str().unwrap()); + } } - } - // Copy scripts (bin entries) in - let bin_dir = entry.parent().unwrap().join("bin"); - for prefix in [&action_src_dir, &action_bin_dir] { - let src_dir = prefix.join(&bin_dir); - if src_dir.exists() { - create_tree(&src_dir, &venv.bin_dir, &collision_strategy)?; + // Copy scripts (bin entries) in + let bin_dir = entry.parent().unwrap().join("bin"); + for prefix in [&action_src_dir, &action_bin_dir] { + let src_dir = prefix.join(&bin_dir); + if src_dir.exists() { + create_tree(&src_dir, &venv.bin_dir, &collision_strategy)?; + } } } - } else { - if entry_repo != repo { - eprintln!("Warning: @@{entry_repo}//{entry_path} is not `site-packages`via pth rather than copy",) + _ => { + if entry_repo != repo { + eprintln!( + "Warning: @@{entry_repo}//{entry_path}/... included via pth rather than copy" + ) + } + // The path to the runfiles root is _one more than_ the relative + // oath from the venv's target dir to the root of the module + // containing the venv. + let path_to_runfiles = + diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir)) + .unwrap() + .join("../"); + + writeln!(dest_pth_writer, "# @{}", line).into_diagnostic()?; + writeln!( + dest_pth_writer, + "{}", + path_to_runfiles // .runfiles + .join(entry_repo) // ${REPO} + .join(entry_path) // ${PATH} + .to_str() + .unwrap() + ) + .into_diagnostic()?; } - // The path to the runfiles root is _one more than_ the relative - // oath from the venv's target dir to the root of the module - // containing the venv. - let path_to_runfiles = diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir)) - .unwrap() - .join(".."); - - writeln!(dest_pth_writer, "# @{}", line).into_diagnostic()?; - writeln!( - dest_pth_writer, - "{}", - path_to_runfiles // .runfiles - .join(entry_repo) // ${REPO} - .join(entry_path) // ${PATH} - .to_str() - .unwrap() - ) - .into_diagnostic()?; } } From 7e899c147d579ed1b01628268609d0e01a2ae4d3 Mon Sep 17 00:00:00 2001 From: "Reid D. McKenzie" Date: Mon, 8 Sep 2025 18:39:39 -0700 Subject: [PATCH 6/6] [NO TESTS] WIP --- py/tools/py/src/venv.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/tools/py/src/venv.rs b/py/tools/py/src/venv.rs index 8c58116e..943f374a 100644 --- a/py/tools/py/src/venv.rs +++ b/py/tools/py/src/venv.rs @@ -510,7 +510,7 @@ pub fn populate_venv_with_copies( match entry.file_name().map(|it| it.to_str().unwrap()) { // FIXME: dist-packages is a Debian-ism that exists in some customer // environments. It would be better if we could manage to make this - // decison a policy under user controll. Hard-coding for now. + // decision a policy under user control. Hard-coding for now. Some("site-packages") | Some("dist-packages") => { #[cfg(feature = "debug")] eprintln!("Entry is site-packages...");