Fix runfiles interacting badly with bazel_tools runfiles; user site flag

Author: arrdem

py/private/py_venv/py_venv.bzl

@@ -101,10 +101,10 @@ def _py_venv_base_impl(ctx):
     rfs = _py_library.make_merged_runfiles(
         ctx,
         extra_depsets = [
-                            py_toolchain.files,
-                            srcs_depset,
-                        ] + virtual_resolution.srcs + virtual_resolution.runfiles +
-                        ([py_toolchain.files] if py_toolchain.runfiles_interpreter else []),
+            py_toolchain.files,
+            srcs_depset,
+        ] + virtual_resolution.srcs + virtual_resolution.runfiles +
+        ([py_toolchain.files] if py_toolchain.runfiles_interpreter else []),
         extra_runfiles_depsets = [
             ctx.attr._runfiles_lib[DefaultInfo].default_runfiles,
         ],
@@ -137,6 +137,10 @@ def _py_venv_base_impl(ctx):
                 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 = [
@@ -299,7 +303,7 @@ A collision can occur when multiple packages providing the same file are install
         default = False,
     ),
     "include_system_site_packages": attr.bool(
-        default = True,
+        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
@@ -314,6 +318,24 @@ 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.
+
 """
     ),
 })

py/tools/py/src/activate.tmpl

@@ -49,8 +49,6 @@ deactivate () {
     fi
 }
 
-{{DEBUG}}
-
 # unset irrelevant variables
 deactivate nondestructive
 

py/tools/py/src/pyvenv.cfg.tmpl

@@ -2,5 +2,6 @@ home = ./bin/python3
 implementation = CPython
 version_info = {{MAJOR}}.{{MINOR}}.{{PATCH}}
 include-system-site-packages = {{INCLUDE_SYSTEM_SITE}}
+aspect-include-user-site-packages = {{INCLUDE_USER_SITE}}
 relocatable = true
 {{INTERPRETER}}

py/tools/py/src/venv.rs

@@ -211,6 +211,7 @@ pub fn create_empty_venv<'a>(
     venv_shim: &Option<PathBuf>,
     debug: bool,
     include_system_site_packages: bool,
+    include_user_site_packages: bool,
 ) -> miette::Result<Virtualenv> {
     let build_dir = current_dir().into_diagnostic()?;
     let home_dir = &build_dir.join(location.to_path_buf());
@@ -245,17 +246,17 @@ pub fn create_empty_venv<'a>(
     let using_runfiles_interpreter = !python.exists() && venv_shim != &None;
 
     let interpreter_cfg_snippet = if using_runfiles_interpreter {
-        &format!(
+        format!(
             "
 # Non-standard extension keys used by the Aspect shim
-aspect_runfiles_interpreter = {0}
-aspect_runfiles_repo = {1}
+aspect-runfiles-interpreter = {0}
+aspect-runfiles-repo = {1}
 ",
             python.display(),
             repo
         )
     } else {
-        ""
+        "".to_owned()
     };
 
     // Create the `pyvenv.cfg` file
@@ -266,10 +267,14 @@ aspect_runfiles_repo = {1}
             .replace("{{MAJOR}}", &venv.version_info.major.to_string())
             .replace("{{MINOR}}", &venv.version_info.minor.to_string())
             .replace("{{PATCH}}", &venv.version_info.patch.to_string())
-            .replace("{{INTERPRETER}}", interpreter_cfg_snippet)
+            .replace("{{INTERPRETER}}", &interpreter_cfg_snippet)
             .replace(
                 "{{INCLUDE_SYSTEM_SITE}}",
                 &include_system_site_packages.to_string(),
+            )
+            .replace(
+                "{{INCLUDE_USER_SITE}}",
+                &include_user_site_packages.to_string(),
             ),
     )
     .into_diagnostic()?;

py/tools/runfiles/src/lib.rs

@@ -155,14 +155,17 @@ impl Runfiles {
     /// RUNFILES_MANIFEST_FILE environment variable is present,
     /// or a directory based Runfiles object otherwise.
     pub fn create() -> Result<Self> {
-        let mode = if let Some(manifest_file) = std::env::var_os(MANIFEST_FILE_ENV_VAR) {
-            Self::create_manifest_based(Path::new(&manifest_file))?
-        } else {
-            let dir = find_runfiles_dir()?;
-            let manifest_path = dir.join("MANIFEST");
-            match manifest_path.exists() {
-                true => Self::create_manifest_based(&manifest_path)?,
-                false => Mode::DirectoryBased(dir),
+        let mode = match std::env::var_os(MANIFEST_FILE_ENV_VAR) {
+            Some(manifest_file) if (!manifest_file.is_empty()) => {
+                Self::create_manifest_based(Path::new(&manifest_file))?
+            }
+            _ => {
+                let dir = find_runfiles_dir()?;
+                let manifest_path = dir.join("MANIFEST");
+                match manifest_path.exists() {
+                    true => Self::create_manifest_based(&manifest_path)?,
+                    false => Mode::DirectoryBased(dir),
+                }
             }
         };
 
@@ -179,6 +182,7 @@ impl Runfiles {
     }
 
     fn create_manifest_based(manifest_path: &Path) -> Result<Mode> {
+        eprintln!("{manifest_path:?}");
         let manifest_content = std::fs::read_to_string(manifest_path)
             .map_err(RunfilesError::RunfilesManifestIoError)?;
         let path_mapping = manifest_content
@@ -266,11 +270,11 @@ fn parse_repo_mapping(path: PathBuf) -> Result<RepoMapping> {
 
 /// Returns the .runfiles directory for the currently executing binary.
 pub fn find_runfiles_dir() -> Result<PathBuf> {
-    assert!(
-        std::env::var_os(MANIFEST_FILE_ENV_VAR).is_none(),
-        "Unexpected call when {} exists",
-        MANIFEST_FILE_ENV_VAR
-    );
+    // assert!(
+    //     std::env::var_os(MANIFEST_FILE_ENV_VAR).is_none(),
+    //     "Unexpected call when {} exists",
+    //     MANIFEST_FILE_ENV_VAR
+    // );
 
     // If Bazel told us about the runfiles dir, use that without looking further.
     if let Some(runfiles_dir) = std::env::var_os(RUNFILES_DIR_ENV_VAR).map(PathBuf::from) {

py/tools/venv_bin/src/main.rs

@@ -94,13 +94,23 @@ struct VenvArgs {
 
     #[clap(
         long,
-        default_missing_value("true"),
-        default_value("true"),
+        default_missing_value("false"),
+        default_value("false"),
         num_args(0..=1),
         require_equals(true),
         action = ArgAction::Set,
     )]
     include_system_site_packages: bool,
+
+    #[clap(
+        long,
+        default_missing_value("false"),
+        default_value("false"),
+        num_args(0..=1),
+        require_equals(true),
+        action = ArgAction::Set,
+    )]
+    include_user_site_packages: bool,
 }
 
 fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> {
@@ -131,6 +141,7 @@ fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> {
                 &args.venv_shim,
                 args.debug,
                 args.include_system_site_packages,
+                args.include_user_site_packages,
             )?;
 
             py::venv::populate_venv_with_copies(
@@ -160,6 +171,7 @@ fn venv_cmd_handler(args: VenvArgs) -> miette::Result<()> {
                 &args.venv_shim,
                 args.debug,
                 args.include_system_site_packages,
+                args.include_user_site_packages,
             )?;
 
             py::venv::populate_venv_with_pth(

py/tools/venv_shim/src/main.rs

@@ -45,6 +45,7 @@ struct PyCfg {
     cfg: PathBuf,
     version_info: String,
     interpreter: InterpreterConfig,
+    user_site: bool,
 }
 
 fn parse_venv_cfg(venv_root: &Path, cfg_path: &Path) -> miette::Result<PyCfg> {
@@ -53,6 +54,7 @@ fn parse_venv_cfg(venv_root: &Path, cfg_path: &Path) -> miette::Result<PyCfg> {
     let mut version: Option<String> = None;
     let mut bazel_interpreter: Option<String> = None;
     let mut bazel_repo: Option<String> = None;
+    let mut user_site: Option<bool> = None;
 
     // FIXME: Errors possible here?
     let reader = io::BufReader::new(file);
@@ -66,12 +68,15 @@ fn parse_venv_cfg(venv_root: &Path, cfg_path: &Path) -> miette::Result<PyCfg> {
                 "version_info" => {
                     version = Some(value.to_string());
                 }
-                "aspect_runfiles_interpreter" => {
+                "aspect-runfiles-interpreter" => {
                     bazel_interpreter = Some(value.to_string());
                 }
-                "aspect_runfiles_repo" => {
+                "aspect-runfiles-repo" => {
                     bazel_repo = Some(value.to_string());
                 }
+                "aspect-include-user-site-packages" => {
+                    user_site = Some(value == "true");
+                }
                 // We don't care about other keys
                 &_ => {}
             }
@@ -87,6 +92,7 @@ fn parse_venv_cfg(venv_root: &Path, cfg_path: &Path) -> miette::Result<PyCfg> {
                 rpath: rloc,
                 repo: repo,
             },
+            user_site: user_site.expect("User site flag not set!"),
         }),
         // FIXME: Kinda useless copy in this case
         (Some(version), None, None) => Ok(PyCfg {
@@ -96,6 +102,7 @@ fn parse_venv_cfg(venv_root: &Path, cfg_path: &Path) -> miette::Result<PyCfg> {
             interpreter: InterpreterConfig::External {
                 version: parse_version_info(&version).unwrap(),
             },
+            user_site: user_site.expect("User site flag not set!"),
         }),
         (None, _, _) => Err(miette!(
             "Invalid pyvenv.cfg file! no interpreter version specified!"
@@ -230,6 +237,12 @@ fn main() -> miette::Result<()> {
     // Set the executable pointer for MacOS, but we do it consistently
     cmd.env("PYTHONEXECUTABLE", &venv_interpreter);
 
+    // Similar to `-s` but this avoids us having to muck with the argv in ways
+    // that could be visible to the called program.
+    if !venv_config.user_site {
+        cmd.env("PYTHONNOUSERSITE", "1");
+    }
+
     // Set the interpreter home to the resolved install base. This works around
     // the home = property in the pyvenv.cfg being wrong because we don't
     // (currently) have a good way to map the interpreter rlocation to a