Skip to content

Commit 04e9d9b

Browse files
committed
Implement an _aspect.py + bzlpth flow
1 parent 038e6bc commit 04e9d9b

File tree

6 files changed

+145
-96
lines changed

6 files changed

+145
-96
lines changed

py/tests/py-venv-standalone-interpreter/test.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ set -ex
44

55
ROOT="$(dirname $0)"
66

7-
"$ROOT"/.ex/bin/python --help
7+
"$ROOT"/.ex/bin/python --help >/dev/null 2>&1
88

99
if [ "Hello, world!" != "$($ROOT/.ex/bin/python -c 'from ex import hello; print(hello())')" ]; then
1010
exit 1

py/tests/py_venv_image_layer/my_app_amd64_layers_listing.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2442,7 +2442,9 @@ files:
24422442
layer: 1
24432443
files:
24442444
- 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/
2445-
- -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
2445+
- -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
2446+
- -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
2447+
- -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
24462448
- -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
24472449
- -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
24482450
- 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/

py/tests/py_venv_image_layer/my_app_arm64_layers_listing.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2423,7 +2423,9 @@ files:
24232423
layer: 1
24242424
files:
24252425
- 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/
2426-
- -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
2426+
- -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
2427+
- -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
2428+
- -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
24272429
- -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
24282430
- -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
24292431
- 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/

py/tools/py/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ rust_library(
99
"src/venv.rs",
1010
],
1111
data = [
12+
"src/_aspect.py",
1213
"src/_virtualenv.py",
1314
"src/activate.tmpl",
1415
"src/pyvenv.cfg.tmpl",

py/tools/py/src/_aspect.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
"""Bazel runfiles appropriate pth extensions.
2+
3+
Very carefully avoids resolving symlink path parts as doing so can result in
4+
unintended escapes from runfiles sandbox trees.
5+
6+
"""
7+
8+
import os
9+
import site
10+
11+
site_dir = os.path.dirname(__file__)
12+
13+
# FIXME: Are there other runfiles dir identification strategies that matter?
14+
runfiles_dir = os.getenv("RUNFILES_DIR")
15+
if not runfiles_dir:
16+
p = site_dir
17+
while p != "/":
18+
if p.endswith(".runfiles"):
19+
break
20+
p = os.path.dirname(p)
21+
else:
22+
raise RuntimeError("Failed to identify the runfiles root by path traversal!")
23+
runfiles_dir = p
24+
25+
# Now that we have the runfiles root, the required additional site paths are
26+
# just the join of the runfiles root and the already bzlmod-transformed roots
27+
# provided by the `rules_py` venv creation code. Join them and add them.
28+
with open(os.path.join(site_dir, "_aspect.bzlpth")) as fp:
29+
for line in fp:
30+
line = line.strip()
31+
p = os.path.normpath(os.path.join(site_dir, line))
32+
# FIXME: Do we want to process embedded pth files? Or just insert
33+
# the paths into the sys.path
34+
site.addsitedir(p)

py/tools/py/src/venv.rs

Lines changed: 103 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ const RELOCATABLE_SHEBANG: &str = "\
196196
/// ./python3.${VERSION_MINOR} d
197197
/// ./site-packages d
198198
/// ./_virtualenv.py t
199-
/// ./_virtualenv.pth t
199+
/// ./_00_virtualenv.pth t
200200
///
201201
///
202202
/// Issues:
@@ -401,49 +401,41 @@ aspect-runfiles-repo = {1}
401401
Ok(venv)
402402
}
403403

404-
/// The tricky bit is that we then need to create _dangling at creation time_
405-
/// symlinks to where `.runfiles` entries _will_ be once the current action
406-
/// completes.
404+
/// Poppulate the virtualenv with files from installed packages.
407405
///
408-
/// The input `.pth` file specifies paths in the format
406+
/// Bazel handles symlinks inside TreeArtifacts at least as of 8.4.0 and before
407+
/// by converting them into copies of the link targets. This prevents us from
408+
/// creating a forrest of symlinks directly as `rules_python` does. Instead we
409+
/// settle for copying files out of the install external repos and into the venv.
409410
///
410-
/// <workspace name>/<path>
411+
/// This is inefficient but it does have the advantage of avoiding potential
412+
/// issues around `realpath(__file__)` causing escapes from the venv root.
411413
///
412-
/// These paths can exist in one of four places
413-
/// 1. `./` (source files in the same workspace)
414-
/// 2. `./external` (source files in a different workspace)
415-
/// 3. `./bazel-out/fastbuild/bin` (generated files in this workspace)
416-
/// 4. `./bazel-out/fastbuild/bin/external` (generated files in a different workspace)
414+
/// In order to handle internal imports (eg. not from external `pip`) we also
415+
/// generate a `_aspect.bzlpth` file which contains Bazel label (label prefixes
416+
/// technically) which can be materialized into import roots against the
417+
/// runfiles structure at interpreter startup time. This allows for behavior
418+
/// similar to the original `rules_python` strategy of just shoving a bunch of
419+
/// stuff into the `$PYTHONPATH` while sidestepping issues around blowing up the
420+
/// env/system arg limits.
417421
///
418-
/// In filling out a static symlink venv we have to:
419-
///
420-
/// 0. Be told by the caller what a relative path _from the venv root_ back up to the
421-
///
422-
/// 1. Go over every entry in the computed `.pth` list
423-
///
424-
/// 2. Identify entries which end in `site-packages`
425-
///
426-
/// 3. Compute a `.runfiles` time location for the root of that import
427-
///
428-
/// 4. For each of the two possible roots of that import (./, ./bazel-bin/,)
429-
/// walk the directory tree there
430-
///
431-
/// 5. For every entry in that directory tree, take the path of that entry `e`
432-
///
433-
/// 6. Relativeize the path of the entry to the import root, `ip`
434-
///
435-
/// 7. Relativize the path of the entry to the `.runfiles` time workspace root `rp`
436-
///
437-
/// 6. Create an _unverified_ _dangling_ symlink in the venv.
438-
///
439-
/// At the time that we create these links the targets won't have been
440-
/// emplaced yet. Bazel will create them when the `.runfiles` directory is
441-
/// materialized by assembling all the input files.
422+
/// The tree we want to create is as follows:
442423
///
443-
/// The link needs to go up the depth of the target plus one to drop `_main`
444-
/// or any other workspace name plus four for the site-packages prefix plus
445-
/// the depth of the `ip` then back down to the workspace-relative path of
446-
/// the target file.
424+
/// ./<venv>/
425+
/// ./pyvenv.cfg t
426+
/// ./bin/
427+
/// ./python l ${PYTHON}
428+
/// ./python3 l ./python
429+
/// ./python3.${VERSION_MINOR} l ./python
430+
/// ./lib d
431+
/// ./python3.${VERSION_MINOR} d
432+
/// ./site-packages d
433+
/// ./_aspect.py t
434+
/// ./_aspect.bzlpth t
435+
/// ./<included subtrees>
436+
437+
// TODO: Need to rework this so that what gets copied vs what gets added to the
438+
// pth is controlled by some sort of pluggable policy.
447439
pub fn populate_venv_with_copies(
448440
repo: &str,
449441
venv: Virtualenv,
@@ -464,24 +456,33 @@ pub fn populate_venv_with_copies(
464456
#[cfg(feature = "debug")]
465457
eprintln!("action_bin_dir: {}", &action_bin_dir.to_str().unwrap());
466458

467-
let source_pth = File::open(pth_file.src.as_path())
468-
.into_diagnostic()
469-
.wrap_err("Unable to open source .pth file")?;
459+
// Add our own venv initialization plugin that's designed to handle the bzlpth mess
460+
fs::write(&venv.site_dir.join("_aspect.pth"), "import _aspect\n").into_diagnostic()?;
461+
fs::write(
462+
&venv.site_dir.join("_aspect.py"),
463+
include_str!("_aspect.py"),
464+
)
465+
.into_diagnostic()?;
470466

471-
let dest_pth = File::create(dest.join("_aspect.pth"))
467+
let dest_pth = File::create(dest.join("_aspect.bzlpth"))
472468
.into_diagnostic()
473-
.wrap_err("Unable to create destination .pth file")?;
469+
.wrap_err("Unable to create destination .bzlpth file")?;
474470

475471
let mut dest_pth_writer = BufWriter::new(dest_pth);
476472
dest_pth_writer
477473
.write(
478474
b"\
479-
# Generated by Aspect py_binary
480-
# Contains relative import paths to non site-package trees within the .runfiles
475+
# Generated by Aspect py_venv_*
476+
# Contains Bazel runfiles path suffixes to import roots
477+
# See _aspect.py for the relevant processing machinery
481478
",
482479
)
483480
.into_diagnostic()?;
484481

482+
let source_pth = File::open(pth_file.src.as_path())
483+
.into_diagnostic()
484+
.wrap_err("Unable to open source .pth file")?;
485+
485486
for line in BufReader::new(source_pth).lines().map_while(Result::ok) {
486487
#[cfg(feature = "debug")]
487488
eprintln!("Got pth line {}", &line);
@@ -506,59 +507,68 @@ pub fn populate_venv_with_copies(
506507
let mut entry = PathBuf::from(entry_path);
507508

508509
// FIXME: Handle other wheel install dirs like bin?
509-
if entry.file_name() == Some(OsStr::new("site-packages")) {
510-
#[cfg(feature = "debug")]
511-
eprintln!("Entry is site-packages...");
512-
513-
// If the entry is external then we have to adjust the path
514-
// FIXME: This isn't quite right outside of bzlmod
515-
if entry_repo != repo {
516-
entry = PathBuf::from("external")
517-
.join(PathBuf::from(entry_repo))
518-
.join(entry)
519-
}
510+
match entry.file_name().map(|it| it.to_str().unwrap()) {
511+
// FIXME: dist-packages is a Debian-ism that exists in some customer
512+
// environments. It would be better if we could manage to make this
513+
// decison a policy under user controll. Hard-coding for now.
514+
Some("site-packages") | Some("dist-packages") => {
515+
#[cfg(feature = "debug")]
516+
eprintln!("Entry is site-packages...");
517+
518+
// If the entry is external then we have to adjust the path
519+
// FIXME: This isn't quite right outside of bzlmod
520+
if entry_repo != repo {
521+
entry = PathBuf::from("external")
522+
.join(PathBuf::from(entry_repo))
523+
.join(entry)
524+
}
520525

521-
// Copy library sources in
522-
for prefix in [&action_src_dir, &action_bin_dir] {
523-
let src_dir = prefix.join(&entry);
524-
if src_dir.exists() {
525-
create_tree(&src_dir, &venv.site_dir, &collision_strategy)?;
526-
} else {
527-
#[cfg(feature = "debug")]
528-
eprintln!("Unable to find srcs under {}", src_dir.to_str().unwrap());
526+
// Copy library sources in
527+
for prefix in [&action_src_dir, &action_bin_dir] {
528+
let src_dir = prefix.join(&entry);
529+
if src_dir.exists() {
530+
create_tree(&src_dir, &venv.site_dir, &collision_strategy)?;
531+
} else {
532+
#[cfg(feature = "debug")]
533+
eprintln!("Unable to find srcs under {}", src_dir.to_str().unwrap());
534+
}
529535
}
530-
}
531536

532-
// Copy scripts (bin entries) in
533-
let bin_dir = entry.parent().unwrap().join("bin");
534-
for prefix in [&action_src_dir, &action_bin_dir] {
535-
let src_dir = prefix.join(&bin_dir);
536-
if src_dir.exists() {
537-
create_tree(&src_dir, &venv.bin_dir, &collision_strategy)?;
537+
// Copy scripts (bin entries) in
538+
let bin_dir = entry.parent().unwrap().join("bin");
539+
for prefix in [&action_src_dir, &action_bin_dir] {
540+
let src_dir = prefix.join(&bin_dir);
541+
if src_dir.exists() {
542+
create_tree(&src_dir, &venv.bin_dir, &collision_strategy)?;
543+
}
538544
}
539545
}
540-
} else {
541-
if entry_repo != repo {
542-
eprintln!("Warning: @@{entry_repo}//{entry_path} is not `site-packages`via pth rather than copy",)
546+
_ => {
547+
if entry_repo != repo {
548+
eprintln!(
549+
"Warning: @@{entry_repo}//{entry_path}/... included via pth rather than copy"
550+
)
551+
}
552+
// The path to the runfiles root is _one more than_ the relative
553+
// oath from the venv's target dir to the root of the module
554+
// containing the venv.
555+
let path_to_runfiles =
556+
diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir))
557+
.unwrap()
558+
.join("../");
559+
560+
writeln!(dest_pth_writer, "# @{}", line).into_diagnostic()?;
561+
writeln!(
562+
dest_pth_writer,
563+
"{}",
564+
path_to_runfiles // .runfiles
565+
.join(entry_repo) // ${REPO}
566+
.join(entry_path) // ${PATH}
567+
.to_str()
568+
.unwrap()
569+
)
570+
.into_diagnostic()?;
543571
}
544-
// The path to the runfiles root is _one more than_ the relative
545-
// oath from the venv's target dir to the root of the module
546-
// containing the venv.
547-
let path_to_runfiles = diff_paths(&action_bin_dir, action_bin_dir.join(&venv.site_dir))
548-
.unwrap()
549-
.join("..");
550-
551-
writeln!(dest_pth_writer, "# @{}", line).into_diagnostic()?;
552-
writeln!(
553-
dest_pth_writer,
554-
"{}",
555-
path_to_runfiles // .runfiles
556-
.join(entry_repo) // ${REPO}
557-
.join(entry_path) // ${PATH}
558-
.to_str()
559-
.unwrap()
560-
)
561-
.into_diagnostic()?;
562572
}
563573
}
564574

0 commit comments

Comments
 (0)