Repository: bazelbuild/bazel
Issue: Zipapps no longer deleting Bazel.runfiles_* directories placed in /tmp
Labels: type: bug
team-Rules-Python

### Description of the bug:


As of Bazel 6.0.0, a change was made to [bazel/rules/python/python_stub_template.txt](https://github.com/bazelbuild/bazel/commit/9d0163002ca63f4cdbaff9420380d72e2a6e38b3#diff-fa3ae340fea197099f8d4b5101d1d71bccc3ceece3517b4ea9126e2d448eab01) that [gates the deletion](https://github.com/bazelbuild/bazel/commit/9d0163002ca63f4cdbaff9420380d72e2a6e38b3#diff-fa3ae340fea197099f8d4b5101d1d71bccc3ceece3517b4ea9126e2d448eab01R336-R337) of the `Bazel.runfiles_*` directory that is created in `/tmp`. This is because it depends on workspace being set, and workspace is only set [when `RUN_UNDER_RUNFILES` is set](https://github.com/bazelbuild/bazel/commit/9d0163002ca63f4cdbaff9420380d72e2a6e38b3#diff-fa3ae340fea197099f8d4b5101d1d71bccc3ceece3517b4ea9126e2d448eab01R501-R507). This is a regression from the previous behavior where the directory placed in `/tmp` would be deleted as long as it was [running under a zip](https://github.com/bazelbuild/bazel/commit/9d0163002ca63f4cdbaff9420380d72e2a6e38b3#diff-fa3ae340fea197099f8d4b5101d1d71bccc3ceece3517b4ea9126e2d448eab01L296-L303). This should be fixed so that the `Bazel.runfiles_*` directories are cleaned up whenever it is running under a zip.

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


Build a zipapp target on Bazel 6.0.0 (without `RUN_UNDER_RUNFILES` set) and on whatever machine you consume that zipapp there will be a `Bazel.runfiles_<sha>` directory with the zipapp contents left around from execution.

### Which operating system are you running Bazel on?


Linux

### What is the output of `bazel info release`?


release 6.0.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Have you found anything relevant by searching the web?

_No response_

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
Pinging this issue. @rickeylev I noticed you've been making changes around the Python rules here. Would you be able to take a look? This issue is coming up more and more as more projects are upgraded to Bazel 6.
What is this "RUN_UNDER_RUNFILES" environment variable? Is there some doc about this? As near as I can tell it's only set when Bazel runs a target as a test?

Anyways, yeah, bug. I was able to repro.

Posterity/notes to self: This was PR #15590
@rickeylev thank you for getting this done so quickly. Can you merge this into the Bazel 6.1 branch so that it can go out as soon as possible?
I suppose so. I've never done or requested a backport, so I'll have to ask around how to do that.
@rickeylev this mentions how: https://github.com/bazelbuild/bazel/issues/17212#issuecomment-1439198224
Hi all, can this wait till the next release (6.2)? We've already created the third release candidate for 6.1 and plan to do the final release on Monday, so we won't be able to include this.
Hi,

Yes, if it's too late for 6.1, then 6.2 is fine.
@bazel-io fork 6.2.0
I was seeing this on 6.3.2 and tried upgrading to 7.1.1 to see if it fixed it, but it still happens.

Anyone else still seeing this?

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
index bd387ebdd9bd86..19514526104b4c 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
@@ -202,6 +202,8 @@ def ExtractZip(zip_path, dest_dir):
 def CreateModuleSpace():
   temp_dir = tempfile.mkdtemp('', 'Bazel.runfiles_')
   ExtractZip(os.path.dirname(__file__), temp_dir)
+  # IMPORTANT: Later code does `rm -fr` on dirname(module_space) -- it's
+  # important that deletion code be in sync with this directory structure
   return os.path.join(temp_dir, 'runfiles')
 
 # Returns repository roots to add to the import path.
@@ -301,7 +303,7 @@ def UnresolveSymlinks(output_filename):
     os.unlink(unfixed_file)
 
 def ExecuteFile(python_program, main_filename, args, env, module_space,
-                coverage_entrypoint, workspace):
+                coverage_entrypoint, workspace, delete_module_space):
   # type: (str, str, list[str], dict[str, str], str, str|None, str|None) -> ...
   """Executes the given Python file using the various environment settings.
 
@@ -316,8 +318,9 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
     module_space: (str) Path to the module space/runfiles tree directory
     coverage_entrypoint: (str|None) Path to the coverage tool entry point file.
     workspace: (str|None) Name of the workspace to execute in. This is expected to be a
-        directory under the runfiles tree, and will recursively delete the
-        runfiles directory if set.
+        directory under the runfiles tree.
+    delete_module_space: (bool), True if the module space should be deleted
+        after a successful (exit code zero) program run, False if not.
   """
   # We want to use os.execv instead of subprocess.call, which causes
   # problems with signal passing (making it difficult to kill
@@ -327,16 +330,15 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
   # - On Windows, os.execv doesn't handle arguments with spaces
   #   correctly, and it actually starts a subprocess just like
   #   subprocess.call.
-  # - When running in a workspace (i.e., if we're running from a zip),
-  #   we need to clean up the workspace after the process finishes so
-  #   control must return here.
+  # - When running in a workspace or zip file, we need to clean up the
+  #   workspace after the process finishes so control must return here.
   # - If we may need to emit a host config warning after execution, we
   #   can't execv because we need control to return here. This only
   #   happens for targets built in the host config.
   # - For coverage targets, at least coveragepy requires running in
   #   two invocations, which also requires control to return here.
   #
-  if not (IsWindows() or workspace or coverage_entrypoint):
+  if not (IsWindows() or workspace or coverage_entrypoint or delete_module_space):
     _RunExecv(python_program, main_filename, args, env)
 
   if coverage_entrypoint is not None:
@@ -349,7 +351,10 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
       cwd=workspace
     )
 
-  if workspace:
+  if delete_module_space:
+    # NOTE: dirname() is called because CreateModuleSpace() creates a
+    # sub-directory within a temporary directory, and we want to remove the
+    # whole temporary directory.
     shutil.rmtree(os.path.dirname(module_space), True)
   sys.exit(ret_code)
 
@@ -438,8 +443,10 @@ def Main():
 
   if IsRunningFromZip():
     module_space = CreateModuleSpace()
+    delete_module_space = True
   else:
     module_space = FindModuleSpace(main_rel_path)
+    delete_module_space = False
 
   python_imports = '%imports%'
   python_path_entries = CreatePythonPathEntries(python_imports, module_space)
@@ -524,9 +531,11 @@ def Main():
 
   try:
     sys.stdout.flush()
+    # NOTE: ExecuteFile may call execve() and lines after this will never run.
     ExecuteFile(
       python_program, main_filename, args, new_env, module_space,
-      cov_tool, workspace
+      cov_tool, workspace,
+      delete_module_space = delete_module_space,
     )
 
   except EnvironmentError:


Test patch:
diff --git a/src/test/shell/bazel/python_version_test.sh b/src/test/shell/bazel/python_version_test.sh
index c7a44708432bc8..485707a67bf7a8 100755
--- a/src/test/shell/bazel/python_version_test.sh
+++ b/src/test/shell/bazel/python_version_test.sh
@@ -201,6 +201,32 @@ EOF
   expect_log "I am mockpy!"
 }
 
+# Test that running a zip app without RUN_UNDER_RUNFILES=1 removes the
+# temporary directory it creates
+function test_build_python_zip_cleans_up_temporary_module_space() {
+
+  mkdir test
+  cat > test/BUILD << EOF
+py_binary(
+  name = "pybin",
+  srcs = ["pybin.py"],
+)
+EOF
+  cat > test/pybin.py << EOF
+print(__file__)
+EOF
+
+  bazel build //test:pybin --build_python_zip &> $TEST_log || fail "bazel build failed"
+  pybin_location=$(bazel-bin/test/pybin)
+
+  # The pybin location is "<ms root>/runfiles/<workspace>/test/pybin.py",
+  # so we have to go up 4 directories to get to the module space root
+  module_space_dir=$(dirname $(dirname $(dirname $(dirname "$pybin_location"))))
+  if [[ -d "$module_space_dir" ]]; then
+    fail "expected module space directory to be deleted, but $module_space_dir still exists"
+  fi
+}
+
 function test_get_python_zip_file_via_output_group() {
   touch foo.py
   cat > BUILD <<'EOF'