From 72a0f9ed13e4b1f561aee4268e4919b84ac04395 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tristan=20Dani=C3=ABl=20Maat?= <tm@tlater.net>
Date: Mon, 31 Jan 2022 15:33:10 +0000
Subject: [PATCH 01/18] rules/python: Rework built-in coveragepy support

[Coveragepy recently gained support for lcov
output][nedbat/coveragepy#1289], which allows implementing full
support for python coverage without relying on a downstream fork of
that project

Coveragepy actually must be invoked twice; One generating a
`.coverage` database file, the other exporting the data. This means
that it is incompatible with the old implementation.

The fork of coveragepy previously intended for use with bazel
circumvented this by just changing how coveragepy works, and never
outputting that database - just outputting the lcov directly
instead. If we'd like to use upstream coveragepy, this is of course
not possible.

The stub_template seems to be written with the idea of supporting
other coverage tooling in mind, however it still hard-codes arguments
specific to coveragepy. Instead, we think it makes sense to properly
support one of them for now, and to rethink a more generic interface
later - it will probably take specific scripting for each
implementation of coverage in python anyway.

As such, this patch rewrites the python stub template to fully support
upstream coveragepy as a coverage tool, and reworks some of the logic
around invoking python to do so more cleanly.

Additional notes:

  - Python coverage will only work with Python 3.7+ with upstream
    coveragepy, since the first release with lcov support does not
    support earlier Python versions - this is unfortunate, but there
    is not much we can do downstream short of forking to resolve
    that. The stub template itself should still work with Python 2.4+.

  - Comments in the code claim to use `os.execv` for performance
    reasons. There may be a small overhead to `subprocess.call`, but
    it shouldn't be too impactful, especially considering the overhead
    in logic (written in Python) this involves - if this is indeed for
    performance reasons, this is probably a somewhat premature
    optimization.

    A colleauge helped dig through some history, finding
    3bed4af4f27bbd22b5531454095f0eda30bfac9f as the source of this -
    if that commit is to believed, this is actually to resolve issues
    with signal handling, however that seems odd as well, since this
    calls arbitrary python applications, which in turn may use
    subprocesses again as well, and therefore break what that commit
    seems to attempt to fix.

    It's completely opaque to me why we put so much effort into trying
    to ensure we use `os.execv`. I've replicated the behavior and
    comments assuming it was correct previously, but the patch
    probably shouldn't land as-is - the comment explaining the use of
    `os.execv` is most likely misleading.

---

[nedbat/coveragepy#1289]: https://github.com/nedbat/coveragepy/pull/1289

Co-authored-by: Bradley Burns <bradley.burns@codethink.co.uk>
---
 .../rules/python/python_stub_template.txt     | 130 +++++++++++++-----
 1 file changed, 96 insertions(+), 34 deletions(-)

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 c389af01c2df1f..78b2a09fe138d3 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
@@ -281,6 +281,71 @@ def Deduplicate(items):
           seen.add(it)
           yield it
 
+def ExecuteFile(python_program, main_filename, args, env, module_space,
+                coverage_tool=None, workspace=None):
+  """Executes the given python file using the various environment settings.
+
+  This will not return, and acts much like os.execv, except is much
+  more restricted, and handles bazel-related edge cases.
+
+  Args:
+    python_program: Path to the python binary to use for execution
+    main_filename: The python file to execute
+    args: Additional args to pass to the python file
+    env: A dict of environment variables to set for the execution
+    module_space: The module space/runfiles tree
+    coverage_tool: The coverage tool to execute with
+    workspace: 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.
+  """
+  # We want to use os.execv instead of subprocess.call, which causes
+  # problems with signal passing (making it difficult to kill
+  # bazel). However, these conditions force us to run via
+  # subprocess.call instead:
+  #
+  # - 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.
+  # - 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 %enable_host_version_warning% or coverage_tool):
+    os.environ.update(env)
+    os.execv(python_program, [python_program, main_filename] + args)
+
+  if coverage_tool is not None:
+    # Coveragepy wants to frst create a .coverage database file, from
+    # which we can then export lcov.
+    subprocess.call(
+      [python_program, coverage_tool, "run", "--append", "--branch", main_filename] + args,
+      env=env,
+      cwd=workspace
+    )
+    output_filename = os.environ.get('COVERAGE_DIR') + '/pylcov.dat'
+    ret_code = subprocess.call(
+      [python_program, coverage_tool, "lcov", "-o", output_filename] + args,
+      env=env,
+      cwd=workspace
+    )
+  else:
+    ret_code = subprocess.call(
+      [python_program, main_filename] + args,
+      env=env,
+      cwd=workspace
+    )
+
+  if workspace:
+    shutil.rmtree(os.path.dirname(module_space), True)
+  MaybeEmitHostVersionWarning(ret_code)
+  sys.exit(ret_code)
+
 def Main():
   args = sys.argv[1:]
 
@@ -332,13 +397,21 @@ def Main():
   if python_program is None:
     raise AssertionError('Could not find python binary: ' + PYTHON_BINARY)
 
-  cov_tool = os.environ.get('PYTHON_COVERAGE')
-  if cov_tool:
-    # Inhibit infinite recursion:
-    del os.environ['PYTHON_COVERAGE']
+  # COVERAGE_DIR is set iff the instrumentation is configured for the
+  # file and coverage is enabled.
+  if os.environ.get('COVERAGE_DIR'):
+    if 'PYTHON_COVERAGE' in os.environ:
+      cov_tool = os.environ.get('PYTHON_COVERAGE')
+    else:
+      raise EnvironmentError(
+        'No python coverage tool set, '
+        'set PYTHON_COVERAGE '
+        'to configure the coverage tool'
+      )
+
     if not os.path.exists(cov_tool):
       raise EnvironmentError('Python coverage tool %s not found.' % cov_tool)
-    args = [python_program, cov_tool, 'run', '-a', '--branch', main_filename] + args
+
     # coverage library expects sys.path[0] to contain the library, and replaces
     # it with the directory of the program it starts. Our actual sys.path[0] is
     # the runfiles directory, which must not be replaced.
@@ -346,40 +419,29 @@ def Main():
     #
     # Update sys.path such that python finds the coverage package. The coverage
     # entry point is coverage.coverage_main, so we need to do twice the dirname.
-    new_env['PYTHONPATH'] = \
-        new_env['PYTHONPATH'] + ':' + os.path.dirname(os.path.dirname(cov_tool))
-    new_env['PYTHON_LCOV_FILE'] = os.environ.get('COVERAGE_DIR') + '/pylcov.dat'
+    new_env['PYTHONPATH'] = (
+      new_env['PYTHONPATH'] + ':' + os.path.dirname(os.path.dirname(cov_tool))
+    )
   else:
-    args = [python_program, main_filename] + args
+    cov_tool = None
 
-  os.environ.update(new_env)
+  new_env.update((key, val) for key, val in os.environ.items() if key not in new_env)
+
+  workspace = None
+  if IsRunningFromZip():
+    # If RUN_UNDER_RUNFILES equals 1, it means we need to
+    # change directory to the right runfiles directory.
+    # (So that the data files are accessible)
+    if os.environ.get('RUN_UNDER_RUNFILES') == '1':
+      workspace = os.path.join(module_space, '%workspace_name%')
 
   try:
     sys.stdout.flush()
-    if IsRunningFromZip():
-      # If RUN_UNDER_RUNFILES equals 1, it means we need to
-      # change directory to the right runfiles directory.
-      # (So that the data files are accessible)
-      if os.environ.get('RUN_UNDER_RUNFILES') == '1':
-        os.chdir(os.path.join(module_space, '%workspace_name%'))
-      ret_code = subprocess.call(args)
-      shutil.rmtree(os.path.dirname(module_space), True)
-      MaybeEmitHostVersionWarning(ret_code)
-      sys.exit(ret_code)
-    else:
-      # On Windows, os.execv doesn't handle arguments with spaces correctly,
-      # and it actually starts a subprocess just like subprocess.call.
-      #
-      # If we may need to emit a host config warning after execution, don't
-      # execv because we need control to return here. This only happens for
-      # targets built in the host config, so other targets still get to take
-      # advantage of the performance benefits of execv.
-      if IsWindows() or %enable_host_version_warning%:
-        ret_code = subprocess.call(args)
-        MaybeEmitHostVersionWarning(ret_code)
-        sys.exit(ret_code)
-      else:
-        os.execv(args[0], args)
+    ExecuteFile(
+      python_program, main_filename, args, new_env, module_space,
+      cov_tool, workspace
+    )
+
   except EnvironmentError:
     # This works from Python 2.4 all the way to 3.x.
     e = sys.exc_info()[1]

From 80a6cb9645a79fe1bca42c25004343d181935713 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Fri, 27 May 2022 14:29:35 -0700
Subject: [PATCH 02/18] Add a coverage_tool attribute to py_runtime target.

If set, and coverage is enabled, the path to this target is used
instead of the PYTHON_COVERAGE environment variable.  This permits
use of in-build versions of the coverage tool, closing #14436.
---
 site/en/docs/coverage.md                      | 11 ++-
 .../rules/python/BazelPythonSemantics.java    | 36 ++++++++++
 .../rules/python/python_stub_template.txt     | 34 +++++++---
 .../build/lib/rules/python/PyRuntime.java     | 31 ++++++++-
 .../build/lib/rules/python/PyRuntimeInfo.java | 68 ++++++++++++++++++-
 .../build/lib/rules/python/PyRuntimeRule.java | 10 +++
 .../build/lib/starlarkbuildapi/python/BUILD   |  1 +
 .../python/PyRuntimeInfoApi.java              | 47 +++++++++++++
 .../lib/rules/python/PyRuntimeInfoTest.java   |  5 +-
 9 files changed, 226 insertions(+), 17 deletions(-)

diff --git a/site/en/docs/coverage.md b/site/en/docs/coverage.md
index 76828f435bd313..3c75cb8945f087 100644
--- a/site/en/docs/coverage.md
+++ b/site/en/docs/coverage.md
@@ -188,7 +188,16 @@ py_test(
     ],
 )
 ```
-<!-- TODO: Allow specifying a target for `PYTHON_COVERAGE`, instead of having to use `$(location)` -->
+
+If you are using a hermetic python toolchain, you can instead simply add
+
+```starlark
+    coverage_tool = ":coverage",
+```
+
+to your `py_runtime` target.  This ensures that coverage is available for all
+`py_test` and `py_binary` targets, and prevents those targets from incurring a
+dependency on `coverage` except when building with coverage enabled.
 
 
 [lcov]: https://github.com/linux-test-project/lcov
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
index 5f94e4b114451c..6b158bad001f2b 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
@@ -92,12 +92,18 @@ public boolean prohibitHyphensInPackagePaths() {
   public void collectRunfilesForBinary(
       RuleContext ruleContext, Runfiles.Builder builder, PyCommon common, CcInfo ccInfo) {
     addRuntime(ruleContext, common, builder);
+    if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
+      addCoverageSupport(ruleContext, common, builder);
+    }
   }
 
   @Override
   public void collectDefaultRunfilesForBinary(
       RuleContext ruleContext, PyCommon common, Runfiles.Builder builder) {
     addRuntime(ruleContext, common, builder);
+    if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
+      addCoverageSupport(ruleContext, common, builder);
+    }
   }
 
   @Override
@@ -154,6 +160,9 @@ private static void createStubFile(
     // first-stage.
     String pythonBinary = getPythonBinary(ruleContext, common, bazelConfig);
 
+    // The python code coverage tool to use, if any.
+    String coverageTool = getCoverageTool(ruleContext, common, bazelConfig);
+
     // Version information for host config diagnostic warning.
     PythonVersion attrVersion = PyCommon.readPythonVersionFromAttribute(ruleContext.attributes());
     boolean attrVersionSpecifiedExplicitly = attrVersion != null;
@@ -172,6 +181,7 @@ private static void createStubFile(
                 Substitution.of(
                     "%main%", common.determineMainExecutableSource(/*withWorkspaceName=*/ true)),
                 Substitution.of("%python_binary%", pythonBinary),
+                Substitution.of("%coverage_tool%", coverageTool == null ? "" : coverageTool),
                 Substitution.of("%imports%", Joiner.on(":").join(common.getImports().toList())),
                 Substitution.of("%workspace_name%", ruleContext.getWorkspaceName()),
                 Substitution.of("%is_zipfile%", boolToLiteral(isForZipFile)),
@@ -461,6 +471,32 @@ private static String getPythonBinary(
     return pythonBinary;
   }
 
+  private static void addCoverageSupport(
+      RuleContext ruleContext, PyCommon common, Runfiles.Builder builder) {
+    PyRuntimeInfo provider = getRuntime(ruleContext, common);
+    if (provider != null && provider.getCoverageTool() != null) {
+      builder.addArtifact(provider.getCoverageTool());
+      builder.addTransitiveArtifacts(provider.getCoverageToolFiles());
+    }
+  }
+
+  @Nullable
+  private static String getCoverageTool(
+      RuleContext ruleContext, PyCommon common, BazelPythonConfiguration bazelConfig) {
+    if (!ruleContext.getConfiguration().isCodeCoverageEnabled()) {
+      return null;
+    }
+    String coverageTool = null;
+    PyRuntimeInfo provider = getRuntime(ruleContext, common);
+    if (provider != null && provider.getCoverageTool() != null) {
+      PathFragment workspaceName =
+          PathFragment.create(ruleContext.getRule().getPackage().getWorkspaceName());
+      coverageTool =
+          workspaceName.getRelative(provider.getCoverageTool().getRunfilesPath()).getPathString();
+    }
+    return coverageTool;
+  }
+
   private static String getStubShebang(RuleContext ruleContext, PyCommon common) {
     PyRuntimeInfo provider = getRuntime(ruleContext, common);
     if (provider != null) {
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 eb31c8dd1e4a48..12e50d4144da6d 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
@@ -86,20 +86,33 @@ def SearchPath(name):
 
 def FindPythonBinary(module_space):
   """Finds the real Python binary if it's not a normal absolute path."""
-  if PYTHON_BINARY.startswith('//'):
+  return FindBinary(module_space, PYTHON_BINARY)
+
+def FindCoverageTool(module_space):
+  cov_tool = '%coverage_tool%' or os.environ.get('PYTHON_COVERAGE')
+  if cov_tool:
+    return FindBinary(module_space, cov_tool)
+  return None
+
+def FindBinary(module_space, bin_name):
+  """Finds the real binary if it's not a normal absolute path."""
+  if not bin_name:
+    return None
+  if bin_name.startswith("//"):
     # Case 1: Path is a label. Not supported yet.
     raise AssertionError(
-      'Bazel does not support execution of Python interpreters via labels yet')
-  elif os.path.isabs(PYTHON_BINARY):
+        "Bazel does not support execution of Python interpreters via labels yet"
+    )
+  elif os.path.isabs(bin_name):
     # Case 2: Absolute path.
-    return PYTHON_BINARY
+    return bin_name
   # Use normpath() to convert slashes to os.sep on Windows.
-  elif os.sep in os.path.normpath(PYTHON_BINARY):
+  elif os.sep in os.path.normpath(bin_name):
     # Case 3: Path is relative to the repo root.
-    return os.path.join(module_space, PYTHON_BINARY)
+    return os.path.join(module_space, bin_name)
   else:
     # Case 4: Path has to be looked up in the search path.
-    return SearchPath(PYTHON_BINARY)
+    return SearchPath(bin_name)
 
 def CreatePythonPathEntries(python_imports, module_space):
   parts = python_imports.split(':')
@@ -269,11 +282,12 @@ def Main():
   if python_program is None:
     raise AssertionError('Could not find python binary: ' + PYTHON_BINARY)
 
-  cov_tool = os.environ.get('PYTHON_COVERAGE')
+  cov_tool = FindCoverageTool(module_space)
   if cov_tool:
     # Inhibit infinite recursion:
-    del os.environ['PYTHON_COVERAGE']
-    if not os.path.exists(cov_tool):
+    if 'PYTHON_COVERAGE' in os.environ:
+      del os.environ['PYTHON_COVERAGE']
+    if cov_tool is None or not os.path.exists(cov_tool):
       raise EnvironmentError('Python coverage tool %s not found.' % cov_tool)
     args = [python_program, cov_tool, 'run', '-a', '--branch', main_filename] + args
     # coverage library expects sys.path[0] to contain the library, and replaces
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
index fdcad28d301779..3ccdaebf107a66 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
@@ -18,11 +18,14 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FileProvider;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
+import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -39,6 +42,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
     NestedSet<Artifact> files = PrerequisiteArtifacts.nestedSet(ruleContext, "files");
     Artifact interpreter = ruleContext.getPrerequisiteArtifact("interpreter");
+    TransitiveInfoCollection coverageTarget = ruleContext.getPrerequisite("coverage_tool");
     PathFragment interpreterPath =
         PathFragment.create(ruleContext.attributes().get("interpreter_path", Type.STRING));
     PythonVersion pythonVersion =
@@ -61,6 +65,18 @@ public ConfiguredTarget create(RuleContext ruleContext)
       ruleContext.attributeError("interpreter_path", "must be an absolute path.");
     }
 
+    Artifact coverageTool = null;
+    NestedSet<Artifact> coverageFiles = null;
+    if (coverageTarget != null) {
+      FilesToRunProvider filesToRun = coverageTarget.getProvider(FilesToRunProvider.class);
+      if (filesToRun == null) {
+        ruleContext.attributeError("coverage_tool", "must be an executable target.");
+      } else {
+        coverageTool = filesToRun.getExecutable();
+      }
+      coverageFiles = PrerequisiteArtifacts.nestedSet(ruleContext, "coverage_tool");
+    }
+
     if (pythonVersion == PythonVersion._INTERNAL_SENTINEL) {
       if (pyConfig.useToolchains()) {
         ruleContext.attributeError(
@@ -83,8 +99,19 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
     PyRuntimeInfo provider =
         hermetic
-            ? PyRuntimeInfo.createForInBuildRuntime(interpreter, files, pythonVersion, stubShebang)
-            : PyRuntimeInfo.createForPlatformRuntime(interpreterPath, pythonVersion, stubShebang);
+            ? PyRuntimeInfo.createForInBuildRuntime(
+                interpreter,
+                files,
+                coverageTool,
+                coverageFiles,
+                pythonVersion,
+                stubShebang)
+            : PyRuntimeInfo.createForPlatformRuntime(
+                interpreterPath,
+                coverageTool,
+                coverageFiles,
+                pythonVersion,
+                stubShebang);
 
     return new RuleConfiguredTargetBuilder(ruleContext)
         .setFilesToBuild(files)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfo.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfo.java
index 2e2f1ad0eeb69c..2d69c42856d777 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfo.java
@@ -18,6 +18,8 @@
 
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.FileProvider;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -56,6 +58,8 @@ public final class PyRuntimeInfo implements Info, PyRuntimeInfoApi<Artifact> {
   @Nullable private final Artifact interpreter;
   // Validated on initialization to contain Artifact
   @Nullable private final Depset files;
+  @Nullable private final Artifact coverageTool;
+  @Nullable private final Depset coverageFiles;
   /** Invariant: either PY2 or PY3. */
   private final PythonVersion pythonVersion;
 
@@ -66,15 +70,20 @@ private PyRuntimeInfo(
       @Nullable PathFragment interpreterPath,
       @Nullable Artifact interpreter,
       @Nullable Depset files,
+      @Nullable Artifact coverageTool,
+      @Nullable Depset coverageFiles,
       PythonVersion pythonVersion,
       @Nullable String stubShebang) {
     Preconditions.checkArgument((interpreterPath == null) != (interpreter == null));
     Preconditions.checkArgument((interpreter == null) == (files == null));
+    Preconditions.checkArgument((coverageTool == null) == (coverageFiles == null));
     Preconditions.checkArgument(pythonVersion.isTargetValue());
     this.location = location != null ? location : Location.BUILTIN;
     this.files = files;
     this.interpreterPath = interpreterPath;
     this.interpreter = interpreter;
+    this.coverageTool = coverageTool;
+    this.coverageFiles = coverageFiles;
     this.pythonVersion = pythonVersion;
     if (stubShebang != null && !stubShebang.isEmpty()) {
       this.stubShebang = stubShebang;
@@ -97,6 +106,8 @@ public Location getCreationLocation() {
   public static PyRuntimeInfo createForInBuildRuntime(
       Artifact interpreter,
       NestedSet<Artifact> files,
+      @Nullable Artifact coverageTool,
+      @Nullable NestedSet<Artifact> coverageFiles,
       PythonVersion pythonVersion,
       @Nullable String stubShebang) {
     return new PyRuntimeInfo(
@@ -104,18 +115,26 @@ public static PyRuntimeInfo createForInBuildRuntime(
         /*interpreterPath=*/ null,
         interpreter,
         Depset.of(Artifact.TYPE, files),
+        coverageTool,
+        coverageFiles == null ? null : Depset.of(Artifact.TYPE, coverageFiles),
         pythonVersion,
         stubShebang);
   }
 
   /** Constructs an instance from native rule logic (built-in location) for a platform runtime. */
   public static PyRuntimeInfo createForPlatformRuntime(
-      PathFragment interpreterPath, PythonVersion pythonVersion, @Nullable String stubShebang) {
+      PathFragment interpreterPath,
+      @Nullable Artifact coverageTool,
+      @Nullable NestedSet<Artifact> coverageFiles,
+      PythonVersion pythonVersion,
+      @Nullable String stubShebang) {
     return new PyRuntimeInfo(
         /*location=*/ null,
         interpreterPath,
         /*interpreter=*/ null,
         /*files=*/ null,
+        coverageTool,
+        coverageFiles == null ? null : Depset.of(Artifact.TYPE, coverageFiles),
         pythonVersion,
         stubShebang);
   }
@@ -131,12 +150,21 @@ public boolean equals(Object other) {
     return (this.interpreterPath.equals(otherInfo.interpreterPath)
         && this.interpreter.equals(otherInfo.interpreter)
         && this.files.equals(otherInfo.files)
+        && this.coverageTool.equals(otherInfo.coverageTool)
+        && this.coverageFiles.equals(otherInfo.coverageFiles)
         && this.stubShebang.equals(otherInfo.stubShebang));
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(PyRuntimeInfo.class, interpreterPath, interpreter, files, stubShebang);
+    return Objects.hash(
+      PyRuntimeInfo.class,
+      interpreterPath,
+      interpreter,
+      coverageTool,
+      coverageFiles,
+      files,
+      stubShebang);
   }
 
   /**
@@ -191,6 +219,28 @@ public Depset getFilesForStarlark() {
     return files;
   }
 
+  @Override
+  @Nullable
+  public Artifact getCoverageTool() {
+    return coverageTool;
+  }
+
+  @Nullable
+  public NestedSet<Artifact> getCoverageToolFiles() {
+    try {
+      return coverageFiles == null ? null : coverageFiles.getSet(Artifact.class);
+    } catch (Depset.TypeException ex) {
+      throw new IllegalStateException("for coverage_runfiles, " + ex.getMessage());
+    }
+  }
+
+  @Override
+  @Nullable
+  public Depset getCoverageToolFilesForStarlark() {
+    return coverageFiles;
+  }
+
+
   public PythonVersion getPythonVersion() {
     return pythonVersion;
   }
@@ -213,6 +263,8 @@ public PyRuntimeInfo constructor(
         Object interpreterPathUncast,
         Object interpreterUncast,
         Object filesUncast,
+        Object coverageToolUncast,
+        Object coverageFilesUncast,
         String pythonVersion,
         String stubShebang,
         StarlarkThread thread)
@@ -226,6 +278,14 @@ public PyRuntimeInfo constructor(
         Depset.cast(filesUncast, Artifact.class, "files");
         filesDepset = (Depset) filesUncast;
       }
+      Artifact coverageTool =
+          coverageToolUncast == NONE ? null : (Artifact) coverageToolUncast;
+      Depset coverageDepset = null;
+      if (coverageFilesUncast != NONE) {
+        // Validate type of filesDepset.
+        Depset.cast(coverageFilesUncast, Artifact.class, "coverage_files");
+        coverageDepset = (Depset) coverageFilesUncast;
+      }
 
       if ((interpreter == null) == (interpreterPath == null)) {
         throw Starlark.errorf(
@@ -253,6 +313,8 @@ public PyRuntimeInfo constructor(
             /*interpreterPath=*/ null,
             interpreter,
             filesDepset,
+            coverageTool,
+            coverageDepset,
             parsedPythonVersion,
             stubShebang);
       } else {
@@ -261,6 +323,8 @@ public PyRuntimeInfo constructor(
             PathFragment.create(interpreterPath),
             /*interpreter=*/ null,
             /*files=*/ null,
+            coverageTool,
+            coverageDepset,
             parsedPythonVersion,
             stubShebang);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
index aceca7b76b412c..ddbec1eaf8cc28 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
@@ -55,6 +55,16 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("interpreter_path", STRING))
 
+        /* <!-- #BLAZE_RULE(py_runtime).ATTRIBUTE(coverage_tool) -->
+        This is a target to use for collecting code coverage information from <code>py_binary</code>
+        and <code>py_test</code> targets.
+
+        <p>If set, the path to this target determines the value for the <code>PYTHON_COVERAGE</code>
+        environment variable, and the target and its runfiles will be added to the runfiles when
+        coverage is enabled.</p>
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("coverage_tool", LABEL).allowedFileTypes(FileTypeSet.NO_FILE).exec())
+
         /* <!-- #BLAZE_RULE(py_runtime).ATTRIBUTE(python_version) -->
         Whether this runtime is for Python major version 2 or 3. Valid values are <code>"PY2"</code>
         and <code>"PY3"</code>.
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD
index 01b7b2c3daa789..1cfc3866ce4412 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/BUILD
@@ -21,6 +21,7 @@ java_library(
     srcs = glob(["*.java"]),
     deps = [
         "//src/main/java/com/google/devtools/build/docgen/annot",
+        "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyRuntimeInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyRuntimeInfoApi.java
index 743bb888624bd7..6b871a2d956bf7 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyRuntimeInfoApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/python/PyRuntimeInfoApi.java
@@ -16,8 +16,10 @@
 
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.docgen.annot.StarlarkConstructor;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.starlarkbuildapi.FileApi;
+import com.google.devtools.build.lib.starlarkbuildapi.FilesToRunProviderApi;
 import com.google.devtools.build.lib.starlarkbuildapi.core.ProviderApi;
 import javax.annotation.Nullable;
 import net.starlark.java.annot.Param;
@@ -82,6 +84,26 @@ public interface PyRuntimeInfoApi<FileT extends FileApi> extends StarlarkValue {
   @Nullable
   Depset getFilesForStarlark();
 
+  @StarlarkMethod(
+      name = "coverage_tool",
+      structField = true,
+      allowReturnNones = true,
+      doc =
+          "If set, this field is a <code>File</code> representing tool used for collecting code "
+              + "coverage information from python tests. Otherwise, this is <code>None</code>.")
+  @Nullable
+  FileT getCoverageTool();
+
+  @StarlarkMethod(
+      name = "coverage_files",
+      structField = true,
+      allowReturnNones = true,
+      doc =
+          "The files required at runtime for using <code>coverage_tool</code>. "
+              + "Will be <code>None</code> if no <code>coverage_tool</code> was provided.")
+  @Nullable
+  Depset getCoverageToolFilesForStarlark();
+
   @StarlarkMethod(
       name = "python_version",
       structField = true,
@@ -145,6 +167,29 @@ interface PyRuntimeInfoProviderApi extends ProviderApi {
                       + "for this argument if you pass in <code>interpreter_path</code>. If "
                       + "<code>interpreter</code> is given and this argument is <code>None</code>, "
                       + "<code>files</code> becomes an empty <code>depset</code> instead."),
+          @Param(
+              name = "coverage_tool",
+              allowedTypes = {
+                @ParamType(type = FileApi.class),
+                @ParamType(type = NoneType.class),
+              },
+              positional = false,
+              named = true,
+              defaultValue = "None",
+              doc = "The value for the new object's <code>coverage_tool</code> field."),
+          @Param(
+              name = "coverage_files",
+              allowedTypes = {
+                @ParamType(type = Depset.class, generic1 = FileApi.class),
+                @ParamType(type = NoneType.class),
+              },
+              positional = false,
+              named = true,
+              defaultValue = "None",
+              doc =
+                  "The value for the new object's <code>coverage_files</code> field. Do not give a "
+                      + "value for this argument if you do not also pass in "
+                      + "<code>coverage_tool</code>."),
           @Param(
               name = "python_version",
               positional = false,
@@ -169,6 +214,8 @@ PyRuntimeInfoApi<?> constructor(
         Object interpreterPathUncast,
         Object interpreterUncast,
         Object filesUncast,
+        Object coverageToolUncast,
+        Object coverageFilesUncast,
         String pythonVersion,
         String stubShebang,
         StarlarkThread thread)
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
index 03a91656267965..3d51090fbb171c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
@@ -58,7 +58,8 @@ private static void assertHasOrderAndContainsExactly(
   public void factoryMethod_InBuildRuntime() throws Exception {
     NestedSet<Artifact> files = NestedSetBuilder.create(Order.STABLE_ORDER, dummyFile);
     PyRuntimeInfo inBuildRuntime =
-        PyRuntimeInfo.createForInBuildRuntime(dummyInterpreter, files, PythonVersion.PY2, null);
+        PyRuntimeInfo.createForInBuildRuntime(
+            dummyInterpreter, files, null, null, PythonVersion.PY2, null);
 
     assertThat(inBuildRuntime.getCreationLocation()).isEqualTo(Location.BUILTIN);
     assertThat(inBuildRuntime.getInterpreterPath()).isNull();
@@ -75,7 +76,7 @@ public void factoryMethod_InBuildRuntime() throws Exception {
   public void factoryMethod_PlatformRuntime() {
     PathFragment path = PathFragment.create("/system/interpreter");
     PyRuntimeInfo platformRuntime =
-        PyRuntimeInfo.createForPlatformRuntime(path, PythonVersion.PY2, null);
+        PyRuntimeInfo.createForPlatformRuntime(path, null, null, PythonVersion.PY2, null);
 
     assertThat(platformRuntime.getCreationLocation()).isEqualTo(Location.BUILTIN);
     assertThat(platformRuntime.getInterpreterPath()).isEqualTo(path);

From de88f81ff00dedf366ed254ae6c7de0be5fb6e90 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Tue, 31 May 2022 11:48:52 -0700
Subject: [PATCH 03/18] Fix up filenames in pycoverage-generated lcov output.

Attempt to configure it to use relative file names (for
future-proofing), but current versions of the tool ignore that
configuration when generating the lcov output.  So, go through each
instrumented file in the coverage manifest and fix it up.

Allow test to pass when no coverage tool is configured.
---
 .../rules/python/python_stub_template.txt     | 110 ++++++++++++++----
 1 file changed, 87 insertions(+), 23 deletions(-)

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 fccefd3704303f..b2dcb4a28b9b3c 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
@@ -226,6 +226,47 @@ def Deduplicate(items):
           seen.add(it)
           yield it
 
+def InstrumentedFilePaths():
+  """Yields tuples of realpath of each instrumented file with the relative path."""
+  manifest_filename = os.environ.get('COVERAGE_MANIFEST')
+  if not manifest_filename:
+    return
+  with open(manifest_filename, "r") as manifest:
+    for line in manifest:
+      filename = line.strip()
+      if not filename:
+        continue
+      try:
+        realpath = os.path.realpath(filename)
+      except OSError:
+        print(
+          "Could not find instrumented file {}".format(filename),
+          file=sys.stderr)
+        continue
+      if realpath != filename:
+        print("Fixing up {} -> {}".format(realpath, filename), file=sys.stderr)
+        yield (realpath, filename)
+
+def UnresolveSymlinks(output_filename):
+  """Replace realpath of instrumented files with the relative path in the lcov output.
+  
+  Though we are asking coveragepy to use relative file names, current
+  ignore that for purposes of generating the lcov report, so we need to go
+  and fix up the report.
+  """
+  substitutions = list(InstrumentedFilePaths())
+  if substitutions:
+    unfixed_file = output_filename + '.tmp'
+    os.rename(output_filename, unfixed_file)
+    with open(unfixed_file, "r") as unfixed:
+      with open(output_filename, "w") as output_file:
+        for line in unfixed:
+          if line.startswith('SF:'):
+            for (realpath, filename) in substitutions:
+              line = line.replace(realpath, filename)
+          output_file.write(line)
+    os.unlink(unfixed_file)
+
 def ExecuteFile(python_program, main_filename, args, env, module_space,
                 coverage_tool=None, workspace=None):
   """Executes the given python file using the various environment settings.
@@ -266,19 +307,44 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
     os.execv(python_program, [python_program, main_filename] + args)
 
   if coverage_tool is not None:
+    # We need for coveragepy to use relative paths.  This can only be configured
+    # via an rc file, so we need to make one.
+    rcfile_name = os.path.join(os.environ['COVERAGE_DIR'], '.coveragerc')
+    with open(rcfile_name, "w") as rcfile:
+      rcfile.write('''[run]
+relative_files = True
+''')
     # Coveragepy wants to frst create a .coverage database file, from
     # which we can then export lcov.
     subprocess.call(
-      [python_program, coverage_tool, "run", "--append", "--branch", main_filename] + args,
+      [
+        python_program,
+        coverage_tool,
+        "run",
+        "--rcfile=" + rcfile_name,
+        "--append",
+        "--branch",
+        main_filename
+      ] + args,
       env=env,
       cwd=workspace
     )
-    output_filename = os.environ.get('COVERAGE_DIR') + '/pylcov.dat'
+    output_filename = os.environ['COVERAGE_DIR'] + '/pylcov.dat'
     ret_code = subprocess.call(
-      [python_program, coverage_tool, "lcov", "-o", output_filename] + args,
+      [
+        python_program,
+        coverage_tool,
+        "lcov",
+        "--rcfile=" + rcfile_name,
+        "-o",
+        output_filename
+      ],
       env=env,
       cwd=workspace
     )
+    os.unlink(rcfile_name)
+    if os.path.isfile(output_filename):
+      UnresolveSymlinks(output_filename)
   else:
     ret_code = subprocess.call(
       [python_program, main_filename] + args,
@@ -352,27 +418,25 @@ def Main():
   if os.environ.get('COVERAGE_DIR'):
     cov_tool = FindCoverageTool(module_space)
     if cov_tool is None:
-      raise EnvironmentError(
-        'No python coverage tool set, '
-        'set PYTHON_COVERAGE '
-        'to configure the coverage tool'
+      print('Coverage was requested, but not python coverage tool was configured.', file=sys.stderr)
+    else:
+      # Inhibit infinite recursion:
+      if 'PYTHON_COVERAGE' in os.environ:
+        del os.environ['PYTHON_COVERAGE']
+
+      if not os.path.exists(cov_tool):
+        raise EnvironmentError('Python coverage tool %s not found.' % cov_tool)
+
+      # coverage library expects sys.path[0] to contain the library, and replaces
+      # it with the directory of the program it starts. Our actual sys.path[0] is
+      # the runfiles directory, which must not be replaced.
+      # CoverageScript.do_execute() undoes this sys.path[0] setting.
+      #
+      # Update sys.path such that python finds the coverage package. The coverage
+      # entry point is coverage.coverage_main, so we need to do twice the dirname.
+      new_env['PYTHONPATH'] = (
+        new_env['PYTHONPATH'] + ':' + os.path.dirname(os.path.dirname(cov_tool))
       )
-    # Inhibit infinite recursion:
-    if 'PYTHON_COVERAGE' in os.environ:
-      del os.environ['PYTHON_COVERAGE']
-    if not os.path.exists(cov_tool):
-      raise EnvironmentError('Python coverage tool %s not found.' % cov_tool)
-
-    # coverage library expects sys.path[0] to contain the library, and replaces
-    # it with the directory of the program it starts. Our actual sys.path[0] is
-    # the runfiles directory, which must not be replaced.
-    # CoverageScript.do_execute() undoes this sys.path[0] setting.
-    #
-    # Update sys.path such that python finds the coverage package. The coverage
-    # entry point is coverage.coverage_main, so we need to do twice the dirname.
-    new_env['PYTHONPATH'] = (
-      new_env['PYTHONPATH'] + ':' + os.path.dirname(os.path.dirname(cov_tool))
-    )
   else:
     cov_tool = None
 

From 437934e3d275cef3f0f9f68d2ee057a577902599 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Mon, 6 Jun 2022 17:39:02 -0700
Subject: [PATCH 04/18] Properly add runfiles for coverage target.

---
 .../devtools/build/lib/rules/python/PyRuntime.java       | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
index 3ccdaebf107a66..1857989bf81283 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import javax.annotation.Nullable;
@@ -74,7 +75,13 @@ public ConfiguredTarget create(RuleContext ruleContext)
       } else {
         coverageTool = filesToRun.getExecutable();
       }
-      coverageFiles = PrerequisiteArtifacts.nestedSet(ruleContext, "coverage_tool");
+      NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
+      result.addTransitive(coverageTarget.getProvider(FileProvider.class).getFilesToBuild());
+      RunfilesProvider runfilesProvider = coverageTarget.getProvider(RunfilesProvider.class);
+      if (runfilesProvider != null) {
+        result.addTransitive(runfilesProvider.getDefaultRunfiles().getArtifacts());
+      }
+      coverageFiles = result.build();
     }
 
     if (pythonVersion == PythonVersion._INTERNAL_SENTINEL) {

From 37b4286ce8227e99185d508f000ece190a0ba45c Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Mon, 6 Jun 2022 18:36:31 -0700
Subject: [PATCH 05/18] Add integration test for python coverage.

---
 src/test/shell/bazel/BUILD                    |   9 +
 .../bazel/bazel_coverage_hermetic_py_test.sh  | 183 ++++++++++++++++++
 2 files changed, 192 insertions(+)
 create mode 100755 src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh

diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index 1fa58ae560e74d..82f26112d0037b 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -523,6 +523,15 @@ sh_test(
     ],
 )
 
+sh_test(
+    name = "bazel_coverage_hermetic_py_test",
+    srcs = ["bazel_coverage_hermetic_py_test.sh"],
+    data = [":test-deps"],
+    tags = [
+        "no_windows",
+    ],
+)
+
 sh_test(
     name = "bazel_coverage_sh_test",
     srcs = ["bazel_coverage_sh_test.sh"],
diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
new file mode 100755
index 00000000000000..8ec01a19f3d741
--- /dev/null
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -0,0 +1,183 @@
+#!/bin/bash
+#
+# Copyright 2015 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+set -eu
+
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+# Fetch hermetic python and register toolchain.
+function set_up() {
+    cat >>WORKSPACE <<EOF
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+
+http_archive(
+    name = "rules_python",
+    sha256 = "cdf6b84084aad8f10bf20b46b77cb48d83c319ebe6458a18e9d2cebf57807cdd",
+    strip_prefix = "rules_python-0.8.1",
+    url = "https://github.com/bazelbuild/rules_python/archive/refs/tags/0.8.1.tar.gz",
+)
+
+load("@rules_python//python:repositories.bzl", "python_register_toolchains")
+
+# Load hermetic python toolchain.
+python_register_toolchains(
+    name = "python3_9",
+    python_version = "3.9",
+    # Don't register it, though, because we need to set up coverage support.
+    register_toolchains = False,
+)
+
+http_archive(
+    name = "coverage_linux_x64",
+    build_file_content = """
+filegroup(
+    name = "coverage",
+    srcs = ["coverage/__main__.py"],
+    data = glob(["coverage/*", "coverage/**/*.py"]),
+    visibility = ["//visibility:public"],
+)
+""",
+    sha256 = "84631e81dd053e8a0d4967cedab6db94345f1c36107c71698f746cb2636c63e3",
+    type = "zip",
+    urls = [
+        "https://files.pythonhosted.org/packages/74/0d/0f3c522312fd27c32e1abe2fb5c323b583a5c108daf2c26d6e8dfdd5a105/coverage-6.4.1-cp39-cp39-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl",
+    ],
+)
+
+register_toolchains(
+    "//:python_toolchain",
+)
+EOF
+}
+
+# Asserts if the given expected coverage result is included in the given output
+# file.
+#
+# - expected_coverage The expected result that must be included in the output.
+# - output_file       The location of the coverage output file.
+function assert_coverage_result() {
+    local expected_coverage="${1}"; shift
+    local output_file="${1}"; shift
+
+    # Replace newlines with commas to facilitate the assertion.
+    local expected_coverage_no_newlines="$( echo "$expected_coverage" | tr '\n' ',' )"
+    local output_file_no_newlines="$( cat "$output_file" | tr '\n' ',' )"
+
+    (echo "$output_file_no_newlines" \
+        | grep -F "$expected_coverage_no_newlines") \
+        || fail "Expected coverage result
+<$expected_coverage>
+was not found in actual coverage report:
+<$( cat "$output_file" )>"
+}
+
+# Returns the path of the code coverage report that was generated by Bazel by
+# looking at the current $TEST_log. The method fails if TEST_log does not
+# contain any coverage report for a passed test.
+function get_coverage_file_path_from_test_log() {
+  local ending_part="$(sed -n -e '/PASSED/,$p' "$TEST_log")"
+
+  local coverage_file_path=$(grep -Eo "/[/a-zA-Z0-9\.\_\-]+\.dat$" <<< "$ending_part")
+  [[ -e "$coverage_file_path" ]] || fail "Coverage output file does not exist!"
+  echo "$coverage_file_path"
+}
+
+function set_up_py_test_coverage() {
+  # Set up python toolchain.
+  cat <<EOF > BUILD
+load("@bazel_tools//tools/python:toolchain.bzl", "py_runtime_pair")
+
+py_runtime(
+    name = "py3_runtime",
+    coverage_tool = "@coverage_linux_x64//:coverage",
+    files = ["@python3_9_x86_64-unknown-linux-gnu//:files"],
+    interpreter = "@python3_9_x86_64-unknown-linux-gnu//:bin/python3",
+    python_version = "PY3",
+)
+
+py_runtime_pair(
+    name = "python_runtimes",
+    py2_runtime = None,
+    py3_runtime = ":py3_runtime",
+)
+
+toolchain(
+    name = "python_toolchain",
+    exec_compatible_with = [
+        "@platforms//os:linux",
+        "@platforms//cpu:x86_64",
+    ],
+    toolchain = ":python_runtimes",
+    toolchain_type = "@bazel_tools//tools/python:toolchain_type",
+)
+EOF
+  # Add a py_library and test.
+  cat <<EOF >> BUILD
+py_library(
+    name = "hello",
+    srcs = ["hello.py"],
+)
+
+py_test(
+    name = "hello_test",
+    srcs = ["hello_test.py"],
+    deps = [":hello"],
+)
+EOF
+  cat <<EOF > hello.py
+def Hello():
+  print("Hello, world!")
+
+def Goodbye():
+  print("Goodbye, world!")
+EOF
+  cat <<EOF > hello_test.py
+import unittest
+import hello
+
+class Tests(unittest.TestCase):
+  def testHello(self):
+    hello.Hello()
+
+if __name__ == "__main__":
+  unittest.main()
+EOF
+  cat <<EOF > expected.dat
+SF:hello.py
+FNF:0
+FNH:0
+DA:1,1,fi+A0ud2xABMExsbhdW38w
+DA:2,1,3qA2I6CcUyJmcd1vpeVcRA
+DA:4,1,nFnrj5CwYCqkvbVhPUFVVw
+DA:5,0,RmWioilSA3bI5NbLlwiuSA
+LH:3
+LF:4
+end_of_record
+EOF
+}
+
+function test_py_test_coverage() {
+  set_up_py_test_coverage
+  bazel coverage --test_output=all //:hello_test &>$TEST_log || fail "Coverage for //:hello_test failed"
+  local coverage_file_path="$( get_coverage_file_path_from_test_log )"
+  diff expected.dat "$coverage_file_path" >> $TEST_log
+  cmp expected.dat "$coverage_file_path" || fail "Coverage output file is different than the expected file for py_library."
+}
+
+run_suite "test tests"
\ No newline at end of file

From b472c21f0a7c7dc13c45ef67a1f5f24caeb9fd9c Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Mon, 6 Jun 2022 18:46:45 -0700
Subject: [PATCH 06/18] Update documentation.

---
 site/en/docs/coverage.md                      | 53 +++++++++++++++++--
 .../bazel/bazel_coverage_hermetic_py_test.sh  |  4 +-
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/site/en/docs/coverage.md b/site/en/docs/coverage.md
index 3c75cb8945f087..cf2f4bfff36672 100644
--- a/site/en/docs/coverage.md
+++ b/site/en/docs/coverage.md
@@ -189,16 +189,59 @@ py_test(
 )
 ```
 
-If you are using a hermetic python toolchain, you can instead simply add
+If you are using a hermetic python toolchain, instead of adding an attribute to
+every `py_test` rule you can instead add the coverage tool to the toolchain
+configuration.
+
+Because the `pip_install` rule depends on the python toolchain, you cannot use
+it to fetch the `coverage` module.  Instead, add in your `WORKSPACE` e.g.
 
 ```starlark
-    coverage_tool = ":coverage",
+http_archive(
+    name = "coverage_linux_x86_64"",
+    build_file_content = """
+filegroup(
+    name = "coverage",
+    srcs = ["coverage/__main__.py"],
+    data = glob(["coverage/*", "coverage/**/*.py"]),
+    visibility = ["//visibility:public"],
+)
+""",
+    sha256 = "84631e81dd053e8a0d4967cedab6db94345f1c36107c71698f746cb2636c63e3",
+    type = "zip",
+    urls = [
+        "https://files.pythonhosted.org/packages/74/0d/0f3c522312fd27c32e1abe2fb5c323b583a5c108daf2c26d6e8dfdd5a105/coverage-6.4.1-cp39-cp39-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl",
+    ],
+)
 ```
 
-to your `py_runtime` target.  This ensures that coverage is available for all
-`py_test` and `py_binary` targets, and prevents those targets from incurring a
-dependency on `coverage` except when building with coverage enabled.
+Then configure your python toolchain as e.g.
 
+```starlark
+py_runtime(
+    name = "py3_runtime_linux_x86_64",
+    coverage_tool = "@coverage_linux_x86_64//:coverage",
+    files = ["@python3_9_x86_64-unknown-linux-gnu//:files"],
+    interpreter = "@python3_9_x86_64-unknown-linux-gnu//:bin/python3",
+    python_version = "PY3",
+)
+
+py_runtime_pair(
+    name = "python_runtimes_linux_x86_64",
+    py2_runtime = None,
+    py3_runtime = ":py3_runtime_linux_x86_64",
+)
+
+toolchain(
+    name = "python_toolchain_linux_x86_64",
+    exec_compatible_with = [
+        "@platforms//os:linux",
+        "@platforms//cpu:x86_64",
+    ],
+    toolchain = ":python_runtimes_linux_x86_64",
+    toolchain_type = "@bazel_tools//tools/python:toolchain_type",
+)
+```
 
 [lcov]: https://github.com/linux-test-project/lcov
 [rules_python]: https://github.com/bazelbuild/rules_python
diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
index 8ec01a19f3d741..38a299f8eb02b3 100755
--- a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -44,7 +44,7 @@ python_register_toolchains(
 )
 
 http_archive(
-    name = "coverage_linux_x64",
+    name = "coverage_linux_x86_64",
     build_file_content = """
 filegroup(
     name = "coverage",
@@ -105,7 +105,7 @@ load("@bazel_tools//tools/python:toolchain.bzl", "py_runtime_pair")
 
 py_runtime(
     name = "py3_runtime",
-    coverage_tool = "@coverage_linux_x64//:coverage",
+    coverage_tool = "@coverage_linux_x86_64//:coverage",
     files = ["@python3_9_x86_64-unknown-linux-gnu//:files"],
     interpreter = "@python3_9_x86_64-unknown-linux-gnu//:bin/python3",
     python_version = "PY3",

From f2c9cd2a37ce4e885fc8a9c8b191ef672431ae73 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Mon, 6 Jun 2022 21:49:48 -0700
Subject: [PATCH 07/18] Add requires-network to
 bazel_coverage_hermetic_py_test.

Irony aside, this is required so the test can download the hermetic
python toolchain and coverage wheel.
---
 src/test/shell/bazel/BUILD | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index 82f26112d0037b..f8e72b68468996 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -529,6 +529,7 @@ sh_test(
     data = [":test-deps"],
     tags = [
         "no_windows",
+        "requires-network",
     ],
 )
 

From 57d88fbd31326698e99340a872d7903a13c75f77 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Thu, 11 Aug 2022 14:28:40 -0700
Subject: [PATCH 08/18] Deduplicate cov_tool path entry.

---
 .../build/lib/bazel/rules/python/python_stub_template.txt   | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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 b2dcb4a28b9b3c..f5730edb495aaf 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
@@ -434,9 +434,9 @@ def Main():
       #
       # Update sys.path such that python finds the coverage package. The coverage
       # entry point is coverage.coverage_main, so we need to do twice the dirname.
-      new_env['PYTHONPATH'] = (
-        new_env['PYTHONPATH'] + ':' + os.path.dirname(os.path.dirname(cov_tool))
-      )
+      python_path_entries = new_env['PYTHONPATH'].split(os.pathsep)
+      python_path_entries.append(os.path.dirname(os.path.dirname(cov_tool)))
+      new_env['PYTHONPATH'] = os.pathsep.join(Deduplicate(python_path_entries))
   else:
     cov_tool = None
 

From e9d5582c6653829fac2c9502dc454023fd59c51b Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Thu, 11 Aug 2022 14:45:31 -0700
Subject: [PATCH 09/18] Fix bad merge conflict resolution.

---
 .../build/lib/bazel/rules/python/python_stub_template.txt      | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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 f5730edb495aaf..3be4586295dd6a 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
@@ -302,7 +302,7 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
   # - For coverage targets, at least coveragepy requires running in
   #   two invocations, which also requires control to return here.
   #
-  if not (IsWindows() or workspace or %enable_host_version_warning% or coverage_tool):
+  if not (IsWindows() or workspace or coverage_tool):
     os.environ.update(env)
     os.execv(python_program, [python_program, main_filename] + args)
 
@@ -354,7 +354,6 @@ relative_files = True
 
   if workspace:
     shutil.rmtree(os.path.dirname(module_space), True)
-  MaybeEmitHostVersionWarning(ret_code)
   sys.exit(ret_code)
 
 def Main():

From 9bb352d52c64f6bd2c5f00df2877c993f110a1fc Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Fri, 19 Aug 2022 13:06:56 -0700
Subject: [PATCH 10/18] Use a mock coverage tool in unit test.

---
 .../rules/python/python_stub_template.txt     |   6 +-
 src/test/shell/bazel/BUILD                    |   1 -
 .../bazel/bazel_coverage_hermetic_py_test.sh  | 103 ++++++++----------
 3 files changed, 46 insertions(+), 64 deletions(-)

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 3be4586295dd6a..35e85bdb9d9c42 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
@@ -316,7 +316,7 @@ relative_files = True
 ''')
     # Coveragepy wants to frst create a .coverage database file, from
     # which we can then export lcov.
-    subprocess.call(
+    ret_code = subprocess.call(
       [
         python_program,
         coverage_tool,
@@ -341,7 +341,7 @@ relative_files = True
       ],
       env=env,
       cwd=workspace
-    )
+    ) or ret_code
     os.unlink(rcfile_name)
     if os.path.isfile(output_filename):
       UnresolveSymlinks(output_filename)
@@ -417,7 +417,7 @@ def Main():
   if os.environ.get('COVERAGE_DIR'):
     cov_tool = FindCoverageTool(module_space)
     if cov_tool is None:
-      print('Coverage was requested, but not python coverage tool was configured.', file=sys.stderr)
+      print('Coverage was requested, but no python coverage tool was configured.', file=sys.stderr)
     else:
       # Inhibit infinite recursion:
       if 'PYTHON_COVERAGE' in os.environ:
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index f8e72b68468996..82f26112d0037b 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -529,7 +529,6 @@ sh_test(
     data = [":test-deps"],
     tags = [
         "no_windows",
-        "requires-network",
     ],
 )
 
diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
index 38a299f8eb02b3..0d755e94677f75 100755
--- a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -24,69 +24,12 @@ source "${CURRENT_DIR}/../integration_test_setup.sh" \
 # Fetch hermetic python and register toolchain.
 function set_up() {
     cat >>WORKSPACE <<EOF
-load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
-
-http_archive(
-    name = "rules_python",
-    sha256 = "cdf6b84084aad8f10bf20b46b77cb48d83c319ebe6458a18e9d2cebf57807cdd",
-    strip_prefix = "rules_python-0.8.1",
-    url = "https://github.com/bazelbuild/rules_python/archive/refs/tags/0.8.1.tar.gz",
-)
-
-load("@rules_python//python:repositories.bzl", "python_register_toolchains")
-
-# Load hermetic python toolchain.
-python_register_toolchains(
-    name = "python3_9",
-    python_version = "3.9",
-    # Don't register it, though, because we need to set up coverage support.
-    register_toolchains = False,
-)
-
-http_archive(
-    name = "coverage_linux_x86_64",
-    build_file_content = """
-filegroup(
-    name = "coverage",
-    srcs = ["coverage/__main__.py"],
-    data = glob(["coverage/*", "coverage/**/*.py"]),
-    visibility = ["//visibility:public"],
-)
-""",
-    sha256 = "84631e81dd053e8a0d4967cedab6db94345f1c36107c71698f746cb2636c63e3",
-    type = "zip",
-    urls = [
-        "https://files.pythonhosted.org/packages/74/0d/0f3c522312fd27c32e1abe2fb5c323b583a5c108daf2c26d6e8dfdd5a105/coverage-6.4.1-cp39-cp39-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl",
-    ],
-)
-
 register_toolchains(
     "//:python_toolchain",
 )
 EOF
 }
 
-# Asserts if the given expected coverage result is included in the given output
-# file.
-#
-# - expected_coverage The expected result that must be included in the output.
-# - output_file       The location of the coverage output file.
-function assert_coverage_result() {
-    local expected_coverage="${1}"; shift
-    local output_file="${1}"; shift
-
-    # Replace newlines with commas to facilitate the assertion.
-    local expected_coverage_no_newlines="$( echo "$expected_coverage" | tr '\n' ',' )"
-    local output_file_no_newlines="$( cat "$output_file" | tr '\n' ',' )"
-
-    (echo "$output_file_no_newlines" \
-        | grep -F "$expected_coverage_no_newlines") \
-        || fail "Expected coverage result
-<$expected_coverage>
-was not found in actual coverage report:
-<$( cat "$output_file" )>"
-}
-
 # Returns the path of the code coverage report that was generated by Bazel by
 # looking at the current $TEST_log. The method fails if TEST_log does not
 # contain any coverage report for a passed test.
@@ -105,9 +48,8 @@ load("@bazel_tools//tools/python:toolchain.bzl", "py_runtime_pair")
 
 py_runtime(
     name = "py3_runtime",
-    coverage_tool = "@coverage_linux_x86_64//:coverage",
-    files = ["@python3_9_x86_64-unknown-linux-gnu//:files"],
-    interpreter = "@python3_9_x86_64-unknown-linux-gnu//:bin/python3",
+    coverage_tool = ":mock_coverage",
+    interpreter_path = "$(which python3)",
     python_version = "PY3",
 )
 
@@ -134,11 +76,52 @@ py_library(
     srcs = ["hello.py"],
 )
 
+filegroup(
+    name = "mock_coverage",
+    srcs = ["mock_coverage.py"],
+)
+
 py_test(
     name = "hello_test",
     srcs = ["hello_test.py"],
     deps = [":hello"],
 )
+EOF
+  cat <<EOF > mock_coverage.py
+#!/usr/bin/env python3
+import argparse
+import os
+import subprocess
+import sys
+parser = argparse.ArgumentParser()
+parser.add_argument("mode", choices=["run", "lcov"])
+parser.add_argument("--rcfile", type=str)
+parser.add_argument("--append", action="store_true")
+parser.add_argument("--branch", action="store_true")
+parser.add_argument("--output", "-o", type=str)
+parser.add_argument("target", nargs="*")
+args = parser.parse_intermixed_args()
+tmp_cov_file = os.path.join(os.environ["COVERAGE_DIR"], "tmp.out")
+if args.mode == "run":
+  subprocess.check_call([sys.executable]+args.target)
+  with open(tmp_cov_file, "a") as tmp:
+    tmp.write("TN:\nSF:")
+    tmp.write(os.path.join(os.path.dirname(os.path.realpath(args.target[0])), "hello.py"))
+    tmp.write("""
+FNF:0
+FNH:0
+DA:1,1,fi+A0ud2xABMExsbhdW38w
+DA:2,1,3qA2I6CcUyJmcd1vpeVcRA
+DA:4,1,nFnrj5CwYCqkvbVhPUFVVw
+DA:5,0,RmWioilSA3bI5NbLlwiuSA
+LH:3
+LF:4
+end_of_record
+""")
+else:
+  with open(args.output, "w") as out_file:
+    with open(tmp_cov_file, "r") as in_file:
+      out_file.write(in_file.read())
 EOF
   cat <<EOF > hello.py
 def Hello():

From 174643773713bf010c6753cd42f83e32b4a306a4 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Fri, 19 Aug 2022 14:01:12 -0700
Subject: [PATCH 11/18] Don't use parse_intermixed_args().

It won't work on python < 3.7.
---
 src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
index 0d755e94677f75..54e441c36c7714 100755
--- a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -94,15 +94,16 @@ import os
 import subprocess
 import sys
 parser = argparse.ArgumentParser()
-parser.add_argument("mode", choices=["run", "lcov"])
+mode = sys.argv[1]
+del(sys.argv[1])
 parser.add_argument("--rcfile", type=str)
 parser.add_argument("--append", action="store_true")
 parser.add_argument("--branch", action="store_true")
 parser.add_argument("--output", "-o", type=str)
 parser.add_argument("target", nargs="*")
-args = parser.parse_intermixed_args()
+args = parser.parse_args()
 tmp_cov_file = os.path.join(os.environ["COVERAGE_DIR"], "tmp.out")
-if args.mode == "run":
+if mode == "run":
   subprocess.check_call([sys.executable]+args.target)
   with open(tmp_cov_file, "a") as tmp:
     tmp.write("TN:\nSF:")

From fed8618f3c5ce5fd44e4439be145dfc9b7f5f7b3 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Fri, 19 Aug 2022 14:26:50 -0700
Subject: [PATCH 12/18] Make mock_coverage.py executable.

---
 src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
index 54e441c36c7714..eb1ec3549d7dbc 100755
--- a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -124,6 +124,7 @@ else:
     with open(tmp_cov_file, "r") as in_file:
       out_file.write(in_file.read())
 EOF
+  chmod +x mock_coverage.py
   cat <<EOF > hello.py
 def Hello():
   print("Hello, world!")
@@ -164,4 +165,4 @@ function test_py_test_coverage() {
   cmp expected.dat "$coverage_file_path" || fail "Coverage output file is different than the expected file for py_library."
 }
 
-run_suite "test tests"
\ No newline at end of file
+run_suite "test tests"

From baed9a21707bb20c976a3ecf82c23814147b3f03 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Fri, 19 Aug 2022 14:40:42 -0700
Subject: [PATCH 13/18] Remove platform constraints from test python_toolchain.

---
 src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
index eb1ec3549d7dbc..a355086084333c 100755
--- a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -61,10 +61,6 @@ py_runtime_pair(
 
 toolchain(
     name = "python_toolchain",
-    exec_compatible_with = [
-        "@platforms//os:linux",
-        "@platforms//cpu:x86_64",
-    ],
     toolchain = ":python_runtimes",
     toolchain_type = "@bazel_tools//tools/python:toolchain_type",
 )

From 337f9286d4ecfd22ea16c123f68c47cca824b44b Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Tue, 30 Aug 2022 21:41:40 -0700
Subject: [PATCH 14/18] Address comments from code review.

Do not require coverage_tool to be executable, so long as it presents a
single file as an entry point.

Fix up some comments, including making the fact that UnresolveSymlinks
is a bit of a hack more explicit.

Further refactor ExecuteFile to break out _RunExecv and _RunForCoverage
into separate functions, to make the control flow more obvious.

Produce more debugging output when VERBOSE_COVERAGE is enabled.
---
 site/en/configure/coverage.md                 |   7 +-
 .../rules/python/python_stub_template.txt     | 178 +++++++++++-------
 .../build/lib/rules/python/PyRuntime.java     |  19 +-
 .../build/lib/rules/python/PyRuntimeRule.java |   7 +-
 .../bazel/bazel_coverage_hermetic_py_test.sh  |  20 +-
 5 files changed, 150 insertions(+), 81 deletions(-)

diff --git a/site/en/configure/coverage.md b/site/en/configure/coverage.md
index cf2f4bfff36672..721c8c7ac43716 100644
--- a/site/en/configure/coverage.md
+++ b/site/en/configure/coverage.md
@@ -193,14 +193,15 @@ If you are using a hermetic python toolchain, instead of adding an attribute to
 every `py_test` rule you can instead add the coverage tool to the toolchain
 configuration.
 
-Because the `pip_install` rule depends on the python toolchain, you cannot use
-it to fetch the `coverage` module.  Instead, add in your `WORKSPACE` e.g.
+Because the [pip_install][pip_install_rule] rule depends on the python
+toolchain, it cannot be used to fetch the `coverage` module.
+Instead, add in your `WORKSPACE` e.g.
 
 ```starlark
 http_archive(
     name = "coverage_linux_x86_64"",
     build_file_content = """
-filegroup(
+py_library(
     name = "coverage",
     srcs = ["coverage/__main__.py"],
     data = glob(["coverage/*", "coverage/**/*.py"]),
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 35e85bdb9d9c42..681f73381ba7c6 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
@@ -88,8 +88,15 @@ def FindPythonBinary(module_space):
   """Finds the real Python binary if it's not a normal absolute path."""
   return FindBinary(module_space, PYTHON_BINARY)
 
-def FindCoverageTool(module_space):
-  cov_tool = '%coverage_tool%' or os.environ.get('PYTHON_COVERAGE')
+def FindCoverageEntryPoint(module_space):
+  cov_tool = '%coverage_tool%'
+  if cov_tool:
+    if os.environ.get("VERBOSE_COVERAGE"):
+      print('Using toolchain coverage_tool %r' % cov_tool)
+  else:
+   cov_tool = os.environ.get('PYTHON_COVERAGE')
+   if cov_tool and os.environ.get("VERBOSE_COVERAGE"):
+     print('PYTHON_COVERAGE: %r' % cov_tool)
   if cov_tool:
     return FindBinary(module_space, cov_tool)
   return None
@@ -248,11 +255,17 @@ def InstrumentedFilePaths():
         yield (realpath, filename)
 
 def UnresolveSymlinks(output_filename):
+  # type: (str) -> None
   """Replace realpath of instrumented files with the relative path in the lcov output.
   
-  Though we are asking coveragepy to use relative file names, current
-  ignore that for purposes of generating the lcov report, so we need to go
-  and fix up the report.
+  Though we are asking coveragepy to use relative file names, currently
+  ignore that for purposes of generating the lcov report (and other reports
+  which are not the XML report), so we need to go and fix up the report.
+
+  This function is a workaround for that issue. Once that issue is fixed
+  upstream and the updated version is widely in use, this should be removed.
+
+  See https://github.com/nedbat/coveragepy/issues/963.
   """
   substitutions = list(InstrumentedFilePaths())
   if substitutions:
@@ -268,22 +281,23 @@ def UnresolveSymlinks(output_filename):
     os.unlink(unfixed_file)
 
 def ExecuteFile(python_program, main_filename, args, env, module_space,
-                coverage_tool=None, workspace=None):
+                coverage_entrypoint, workspace):
+  # type: (str, str, list[str], dict[str, str], str, str|None, str|None) -> ...
   """Executes the given python file using the various environment settings.
 
   This will not return, and acts much like os.execv, except is much
   more restricted, and handles bazel-related edge cases.
 
   Args:
-    python_program: Path to the python binary to use for execution
-    main_filename: The python file to execute
-    args: Additional args to pass to the python file
-    env: A dict of environment variables to set for the execution
-    module_space: The module space/runfiles tree
-    coverage_tool: The coverage tool to execute with
-    workspace: 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.
+    python_program (str): Path to the python binary to use for execution
+    main_filename (str): The python file to execute
+    args (list[str]): Additional args to pass to the python file
+    env (dict[str, str]): A dict of environment variables to set for the execution
+    module_space (str): The module space/runfiles tree
+    coverage_entrypoint (str): The coverage tool to execute with.
+    workspace (str): 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.
   """
   # We want to use os.execv instead of subprocess.call, which causes
   # problems with signal passing (making it difficult to kill
@@ -302,49 +316,12 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
   # - 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_tool):
-    os.environ.update(env)
-    os.execv(python_program, [python_program, main_filename] + args)
-
-  if coverage_tool is not None:
-    # We need for coveragepy to use relative paths.  This can only be configured
-    # via an rc file, so we need to make one.
-    rcfile_name = os.path.join(os.environ['COVERAGE_DIR'], '.coveragerc')
-    with open(rcfile_name, "w") as rcfile:
-      rcfile.write('''[run]
-relative_files = True
-''')
-    # Coveragepy wants to frst create a .coverage database file, from
-    # which we can then export lcov.
-    ret_code = subprocess.call(
-      [
-        python_program,
-        coverage_tool,
-        "run",
-        "--rcfile=" + rcfile_name,
-        "--append",
-        "--branch",
-        main_filename
-      ] + args,
-      env=env,
-      cwd=workspace
-    )
-    output_filename = os.environ['COVERAGE_DIR'] + '/pylcov.dat'
-    ret_code = subprocess.call(
-      [
-        python_program,
-        coverage_tool,
-        "lcov",
-        "--rcfile=" + rcfile_name,
-        "-o",
-        output_filename
-      ],
-      env=env,
-      cwd=workspace
-    ) or ret_code
-    os.unlink(rcfile_name)
-    if os.path.isfile(output_filename):
-      UnresolveSymlinks(output_filename)
+  if not (IsWindows() or workspace or coverage_entrypoint):
+    _RunExecv(python_program, main_filename, args, env)
+
+  if coverage_entrypoint is not None:
+    ret_code = _RunForCoverage(python_program, main_filename, args, env,
+                               coverage_entrypoint, workspace)
   else:
     ret_code = subprocess.call(
       [python_program, main_filename] + args,
@@ -356,6 +333,74 @@ relative_files = True
     shutil.rmtree(os.path.dirname(module_space), True)
   sys.exit(ret_code)
 
+def _RunExecv(python_program, main_filename, args, env):
+  # type: (str, str, list[str], dict[str, str]) -> ...
+  """Executes the given python file using the various environment settings."""
+  os.environ.update(env)
+  os.execv(python_program, [python_program, main_filename] + args)
+
+def _RunForCoverage(python_program, main_filename, args, env,
+                    coverage_entrypoint, workspace):
+  # type: (str, str, list[str], dict[str, str], str, str|None) -> int
+  """Collects coverage infomration for the given python file.
+
+  Args:
+    python_program (str): Path to the python binary to use for execution
+    main_filename (str): The python file to execute
+    args (list[str]): Additional args to pass to the python file
+    env (dict[str, str]): A dict of environment variables to set for the execution
+    coverage_entrypoint (str): The coverage tool to execute with.
+    workspace (str): 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.
+  """
+  # We need for coveragepy to use relative paths.  This can only be configured
+  # via an rc file, so we need to make one.
+  rcfile_name = os.path.join(os.environ['COVERAGE_DIR'], '.coveragerc')
+  with open(rcfile_name, "w") as rcfile:
+    rcfile.write('''[run]
+relative_files = True
+''')
+  verbose_coverage = os.environ.get("VERBOSE_COVERAGE")
+  if verbose_coverage:
+    print('Coverage entrypoint:', coverage_entrypoint, file=sys.stderr)
+  # First run the target python file via coveragepy to create a .coverage
+  # database file, from which we can later export lcov.
+  ret_code = subprocess.call(
+    [
+      python_program,
+      coverage_entrypoint,
+      "run",
+      "--rcfile=" + rcfile_name,
+      "--append",
+      "--branch",
+      main_filename
+    ] + args,
+    env=env,
+    cwd=workspace
+  )
+  output_filename = os.path.join(os.environ['COVERAGE_DIR'], 'pylcov.dat')
+
+  if verbose_coverage:
+    print('Converting coveragepy database to lcov:', output_filename, file=sys.stderr)
+  # Run coveragepy again to convert its .coverage database file into lcov.
+  ret_code = subprocess.call(
+    [
+      python_program,
+      coverage_entrypoint,
+      "lcov",
+      "--rcfile=" + rcfile_name,
+      "-o",
+      output_filename
+    ],
+    env=env,
+    cwd=workspace
+  ) or ret_code
+  os.unlink(rcfile_name)
+  if os.path.isfile(output_filename):
+    UnresolveSymlinks(output_filename)
+  return ret_code
+
 def Main():
   args = sys.argv[1:]
 
@@ -412,19 +457,26 @@ def Main():
   if python_program is None:
     raise AssertionError('Could not find python binary: ' + PYTHON_BINARY)
 
-  # COVERAGE_DIR is set iff the instrumentation is configured for the
-  # file and coverage is enabled.
+  # COVERAGE_DIR is set if coverage is enabled and instrumentation is configured
+  # for something, though it could be another program executing this one or
+  # one executed by this one (e.g. an extension module).
   if os.environ.get('COVERAGE_DIR'):
-    cov_tool = FindCoverageTool(module_space)
+    cov_tool = FindCoverageEntryPoint(module_space)
     if cov_tool is None:
-      print('Coverage was requested, but no python coverage tool was configured.', file=sys.stderr)
+      if os.environ.get("VERBOSE_COVERAGE"):
+        print('Coverage was enabled, but python coverage tool was not configured.',
+              file=sys.stderr)
     else:
       # Inhibit infinite recursion:
       if 'PYTHON_COVERAGE' in os.environ:
         del os.environ['PYTHON_COVERAGE']
 
       if not os.path.exists(cov_tool):
-        raise EnvironmentError('Python coverage tool %s not found.' % cov_tool)
+        raise EnvironmentError(
+          'Python coverage tool %r not found. '
+          'Try running with VERBOSE_COVERAGE=1 to collect more information.'
+          % cov_tool
+        )
 
       # coverage library expects sys.path[0] to contain the library, and replaces
       # it with the directory of the program it starts. Our actual sys.path[0] is
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
index 1857989bf81283..2830f8ce24e1d4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java
@@ -43,7 +43,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
     NestedSet<Artifact> files = PrerequisiteArtifacts.nestedSet(ruleContext, "files");
     Artifact interpreter = ruleContext.getPrerequisiteArtifact("interpreter");
-    TransitiveInfoCollection coverageTarget = ruleContext.getPrerequisite("coverage_tool");
     PathFragment interpreterPath =
         PathFragment.create(ruleContext.attributes().get("interpreter_path", Type.STRING));
     PythonVersion pythonVersion =
@@ -68,15 +67,23 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
     Artifact coverageTool = null;
     NestedSet<Artifact> coverageFiles = null;
+    TransitiveInfoCollection coverageTarget = ruleContext.getPrerequisite("coverage_tool");
     if (coverageTarget != null) {
-      FilesToRunProvider filesToRun = coverageTarget.getProvider(FilesToRunProvider.class);
-      if (filesToRun == null) {
-        ruleContext.attributeError("coverage_tool", "must be an executable target.");
+      NestedSet<Artifact> coverageToolFiles =
+          coverageTarget.getProvider(FileProvider.class).getFilesToBuild();
+      if (coverageToolFiles.isSingleton()) {
+        coverageTool = coverageToolFiles.getSingleton();
       } else {
-        coverageTool = filesToRun.getExecutable();
+        FilesToRunProvider filesToRun = coverageTarget.getProvider(FilesToRunProvider.class);
+        if (filesToRun == null) {
+          ruleContext.attributeError("coverage_tool",
+              "must be an executable target or must produce exactly one file.");
+        } else {
+          coverageTool = filesToRun.getExecutable();
+        }
       }
       NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
-      result.addTransitive(coverageTarget.getProvider(FileProvider.class).getFilesToBuild());
+      result.addTransitive(coverageToolFiles);
       RunfilesProvider runfilesProvider = coverageTarget.getProvider(RunfilesProvider.class);
       if (runfilesProvider != null) {
         result.addTransitive(runfilesProvider.getDefaultRunfiles().getArtifacts());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
index ddbec1eaf8cc28..e5c1bdda45dd0c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
@@ -59,11 +59,10 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         This is a target to use for collecting code coverage information from <code>py_binary</code>
         and <code>py_test</code> targets.
 
-        <p>If set, the path to this target determines the value for the <code>PYTHON_COVERAGE</code>
-        environment variable, and the target and its runfiles will be added to the runfiles when
-        coverage is enabled.</p>
+        <p>If set, the path to this target determines the entry point for the python coverage tool,
+        and the target and its runfiles will be added to the runfiles when coverage is enabled.</p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("coverage_tool", LABEL).allowedFileTypes(FileTypeSet.NO_FILE).exec())
+        .add(attr("coverage_tool", LABEL).allowedFileTypes(FileTypeSet.NO_FILE))
 
         /* <!-- #BLAZE_RULE(py_runtime).ATTRIBUTE(python_version) -->
         Whether this runtime is for Python major version 2 or 3. Valid values are <code>"PY2"</code>
diff --git a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
index a355086084333c..0da772c7678769 100755
--- a/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
+++ b/src/test/shell/bazel/bazel_coverage_hermetic_py_test.sh
@@ -34,9 +34,11 @@ EOF
 # looking at the current $TEST_log. The method fails if TEST_log does not
 # contain any coverage report for a passed test.
 function get_coverage_file_path_from_test_log() {
-  local ending_part="$(sed -n -e '/PASSED/,$p' "$TEST_log")"
+  local ending_part
+  ending_part="$(sed -n -e '/PASSED/,$p' "$TEST_log")"
 
-  local coverage_file_path=$(grep -Eo "/[/a-zA-Z0-9\.\_\-]+\.dat$" <<< "$ending_part")
+  local coverage_file_path
+  coverage_file_path=$(grep -Eo "/[/a-zA-Z0-9\.\_\-]+\.dat$" <<< "$ending_part")
   [[ -e "$coverage_file_path" ]] || fail "Coverage output file does not exist!"
   echo "$coverage_file_path"
 }
@@ -72,9 +74,15 @@ py_library(
     srcs = ["hello.py"],
 )
 
-filegroup(
+py_library(
     name = "mock_coverage",
     srcs = ["mock_coverage.py"],
+    deps = [":coverage_support"],
+)
+
+py_library(
+    name = "coverage_support",
+    srcs = ["coverage_support.py"],
 )
 
 py_test(
@@ -83,12 +91,14 @@ py_test(
     deps = [":hello"],
 )
 EOF
+  echo "# fake dependency" > coverage_support.py
   cat <<EOF > mock_coverage.py
 #!/usr/bin/env python3
 import argparse
 import os
 import subprocess
 import sys
+import coverage_support
 parser = argparse.ArgumentParser()
 mode = sys.argv[1]
 del(sys.argv[1])
@@ -120,7 +130,6 @@ else:
     with open(tmp_cov_file, "r") as in_file:
       out_file.write(in_file.read())
 EOF
-  chmod +x mock_coverage.py
   cat <<EOF > hello.py
 def Hello():
   print("Hello, world!")
@@ -156,7 +165,8 @@ EOF
 function test_py_test_coverage() {
   set_up_py_test_coverage
   bazel coverage --test_output=all //:hello_test &>$TEST_log || fail "Coverage for //:hello_test failed"
-  local coverage_file_path="$( get_coverage_file_path_from_test_log )"
+  local coverage_file_path
+  coverage_file_path="$( get_coverage_file_path_from_test_log )"
   diff expected.dat "$coverage_file_path" >> $TEST_log
   cmp expected.dat "$coverage_file_path" || fail "Coverage output file is different than the expected file for py_library."
 }

From 7c9b34f9dc60d3503084d14ca5162748eaa93bbc Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Wed, 31 Aug 2022 18:00:58 -0700
Subject: [PATCH 15/18] Ignore failures unlinking the rcfile.

---
 .../build/lib/bazel/rules/python/python_stub_template.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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 681f73381ba7c6..faa18804726f44 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
@@ -396,7 +396,13 @@ relative_files = True
     env=env,
     cwd=workspace
   ) or ret_code
-  os.unlink(rcfile_name)
+  try:
+    os.unlink(rcfile_name)
+  except OSError:
+    # It's possible that the profiled program might execute another python
+    # binary through a wrapper that would then delete the rcfile.  Not much
+    # we can do about that, besides ignore the failure here.
+    pass
   if os.path.isfile(output_filename):
     UnresolveSymlinks(output_filename)
   return ret_code

From 4550f7044d9bfa810118dd7a201a33d76a5294f4 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Wed, 31 Aug 2022 22:55:13 -0700
Subject: [PATCH 16/18] Fix docstring style.

---
 .../rules/python/python_stub_template.txt     | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

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 faa18804726f44..09ef345abbcbf6 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
@@ -289,13 +289,13 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
   more restricted, and handles bazel-related edge cases.
 
   Args:
-    python_program (str): Path to the python binary to use for execution
-    main_filename (str): The python file to execute
-    args (list[str]): Additional args to pass to the python file
-    env (dict[str, str]): A dict of environment variables to set for the execution
-    module_space (str): The module space/runfiles tree
-    coverage_entrypoint (str): The coverage tool to execute with.
-    workspace (str): The workspace to execute in. This is expected to be a
+    python_program: (str) Path to the python binary to use for execution
+    main_filename: (str) The python file to execute
+    args: (list[str]) Additional args to pass to the python file
+    env: (dict[str, str]) A dict of environment variables to set for the execution
+    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.
   """
@@ -345,12 +345,12 @@ def _RunForCoverage(python_program, main_filename, args, env,
   """Collects coverage infomration for the given python file.
 
   Args:
-    python_program (str): Path to the python binary to use for execution
-    main_filename (str): The python file to execute
-    args (list[str]): Additional args to pass to the python file
-    env (dict[str, str]): A dict of environment variables to set for the execution
-    coverage_entrypoint (str): The coverage tool to execute with.
-    workspace (str): The workspace to execute in. This is expected to be a
+    python_program: (str) Path to the python binary to use for execution
+    main_filename: (str) The python file to execute
+    args: (list[str]) Additional args to pass to the python file
+    env: (dict[str, str]) A dict of environment variables to set for the execution
+    coverage_entrypoint: (str|None) Path to the coverage entry point to execute with.
+    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.
   """

From ec50345ab77154bb00b28fa6f5a213aa7016cce4 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Sun, 4 Sep 2022 23:36:26 -0700
Subject: [PATCH 17/18] Detail the requrements for the coverage_tool.

---
 .../build/lib/rules/python/PyRuntimeRule.java         | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
index e5c1bdda45dd0c..1ec90325df7965 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntimeRule.java
@@ -59,8 +59,15 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         This is a target to use for collecting code coverage information from <code>py_binary</code>
         and <code>py_test</code> targets.
 
-        <p>If set, the path to this target determines the entry point for the python coverage tool,
-        and the target and its runfiles will be added to the runfiles when coverage is enabled.</p>
+        <p>If set, the target must either produce a single file or be and executable target.
+        The path to the single file, or the executable if the target is executable,
+        determines the entry point for the python coverage tool.  The target and its
+        runfiles will be added to the runfiles when coverage is enabled.</p>
+
+        <p>The entry point for the tool must be loadable by a python interpreter (e.g. a
+        <code>.py</code> or <code>.pyc</code> file).  It must accept the command line arguments
+        of <a href="https://coverage.readthedocs.io/">coverage.py</a>, at least including
+        the <code>run</code> and <code>lcov</code> subcommands.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("coverage_tool", LABEL).allowedFileTypes(FileTypeSet.NO_FILE))
 

From 3b892de4fe723dc0364b5cf87370f6ac939a0982 Mon Sep 17 00:00:00 2001
From: Adam Azarchs <adam.azarchs@10xgenomics.com>
Date: Tue, 6 Sep 2022 12:52:44 -0700
Subject: [PATCH 18/18] Factor out VERBOSE_COVERAGE handling in stub.

Also fix some minor issues in documentation.
---
 site/en/configure/coverage.md                 |  8 +--
 .../rules/python/BazelPythonSemantics.java    |  2 +
 .../rules/python/python_stub_template.txt     | 57 +++++++++----------
 3 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/site/en/configure/coverage.md b/site/en/configure/coverage.md
index 721c8c7ac43716..4f546df03c3d3e 100644
--- a/site/en/configure/coverage.md
+++ b/site/en/configure/coverage.md
@@ -189,11 +189,11 @@ py_test(
 )
 ```
 
-If you are using a hermetic python toolchain, instead of adding an attribute to
-every `py_test` rule you can instead add the coverage tool to the toolchain
-configuration.
+If you are using a hermetic Python toolchain, instead of adding the coverage
+dependency to every `py_test` target you can instead add the coverage tool to
+the toolchain configuration.
 
-Because the [pip_install][pip_install_rule] rule depends on the python
+Because the [pip_install][pip_install_rule] rule depends on the Python
 toolchain, it cannot be used to fetch the `coverage` module.
 Instead, add in your `WORKSPACE` e.g.
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
index 6b158bad001f2b..c26c7b9ef79c34 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
@@ -92,6 +92,8 @@ public boolean prohibitHyphensInPackagePaths() {
   public void collectRunfilesForBinary(
       RuleContext ruleContext, Runfiles.Builder builder, PyCommon common, CcInfo ccInfo) {
     addRuntime(ruleContext, common, builder);
+    // select() and build configuration should ideally remove coverage as
+    // as dependency, but guard against including it at runtime just in case.
     if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
       addCoverageSupport(ruleContext, common, builder);
     }
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 09ef345abbcbf6..480261fad5b285 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
@@ -88,15 +88,19 @@ def FindPythonBinary(module_space):
   """Finds the real Python binary if it's not a normal absolute path."""
   return FindBinary(module_space, PYTHON_BINARY)
 
+def PrintVerboseCoverage(*args):
+  """Print output if VERBOSE_COVERAGE is non-empty in the environment."""
+  if os.environ.get("VERBOSE_COVERAGE"):
+    print(*args, file=sys.stderr)
+
 def FindCoverageEntryPoint(module_space):
   cov_tool = '%coverage_tool%'
   if cov_tool:
-    if os.environ.get("VERBOSE_COVERAGE"):
-      print('Using toolchain coverage_tool %r' % cov_tool)
+    PrintVerboseCoverage('Using toolchain coverage_tool %r' % cov_tool)
   else:
-   cov_tool = os.environ.get('PYTHON_COVERAGE')
-   if cov_tool and os.environ.get("VERBOSE_COVERAGE"):
-     print('PYTHON_COVERAGE: %r' % cov_tool)
+    cov_tool = os.environ.get('PYTHON_COVERAGE')
+    if cov_tool:
+      PrintVerboseCoverage('PYTHON_COVERAGE: %r' % cov_tool)
   if cov_tool:
     return FindBinary(module_space, cov_tool)
   return None
@@ -251,7 +255,7 @@ def InstrumentedFilePaths():
           file=sys.stderr)
         continue
       if realpath != filename:
-        print("Fixing up {} -> {}".format(realpath, filename), file=sys.stderr)
+        PrintVerboseCoverage("Fixing up {} -> {}".format(realpath, filename))
         yield (realpath, filename)
 
 def UnresolveSymlinks(output_filename):
@@ -283,15 +287,15 @@ def UnresolveSymlinks(output_filename):
 def ExecuteFile(python_program, main_filename, args, env, module_space,
                 coverage_entrypoint, workspace):
   # type: (str, str, list[str], dict[str, str], str, str|None, str|None) -> ...
-  """Executes the given python file using the various environment settings.
+  """Executes the given Python file using the various environment settings.
 
   This will not return, and acts much like os.execv, except is much
-  more restricted, and handles bazel-related edge cases.
+  more restricted, and handles Bazel-related edge cases.
 
   Args:
-    python_program: (str) Path to the python binary to use for execution
-    main_filename: (str) The python file to execute
-    args: (list[str]) Additional args to pass to the python file
+    python_program: (str) Path to the Python binary to use for execution
+    main_filename: (str) The Python file to execute
+    args: (list[str]) Additional args to pass to the Python file
     env: (dict[str, str]) A dict of environment variables to set for the execution
     module_space: (str) Path to the module space/runfiles tree directory
     coverage_entrypoint: (str|None) Path to the coverage tool entry point file.
@@ -301,7 +305,7 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
   """
   # We want to use os.execv instead of subprocess.call, which causes
   # problems with signal passing (making it difficult to kill
-  # bazel). However, these conditions force us to run via
+  # Bazel). However, these conditions force us to run via
   # subprocess.call instead:
   #
   # - On Windows, os.execv doesn't handle arguments with spaces
@@ -335,19 +339,19 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
 
 def _RunExecv(python_program, main_filename, args, env):
   # type: (str, str, list[str], dict[str, str]) -> ...
-  """Executes the given python file using the various environment settings."""
+  """Executes the given Python file using the various environment settings."""
   os.environ.update(env)
   os.execv(python_program, [python_program, main_filename] + args)
 
 def _RunForCoverage(python_program, main_filename, args, env,
                     coverage_entrypoint, workspace):
   # type: (str, str, list[str], dict[str, str], str, str|None) -> int
-  """Collects coverage infomration for the given python file.
+  """Collects coverage infomration for the given Python file.
 
   Args:
-    python_program: (str) Path to the python binary to use for execution
-    main_filename: (str) The python file to execute
-    args: (list[str]) Additional args to pass to the python file
+    python_program: (str) Path to the Python binary to use for execution
+    main_filename: (str) The Python file to execute
+    args: (list[str]) Additional args to pass to the Python file
     env: (dict[str, str]) A dict of environment variables to set for the execution
     coverage_entrypoint: (str|None) Path to the coverage entry point to execute with.
     workspace: (str|None) Name of the workspace to execute in. This is expected to be a
@@ -361,10 +365,8 @@ def _RunForCoverage(python_program, main_filename, args, env,
     rcfile.write('''[run]
 relative_files = True
 ''')
-  verbose_coverage = os.environ.get("VERBOSE_COVERAGE")
-  if verbose_coverage:
-    print('Coverage entrypoint:', coverage_entrypoint, file=sys.stderr)
-  # First run the target python file via coveragepy to create a .coverage
+  PrintVerboseCoverage('Coverage entrypoint:', coverage_entrypoint)
+  # First run the target Python file via coveragepy to create a .coverage
   # database file, from which we can later export lcov.
   ret_code = subprocess.call(
     [
@@ -381,8 +383,7 @@ relative_files = True
   )
   output_filename = os.path.join(os.environ['COVERAGE_DIR'], 'pylcov.dat')
 
-  if verbose_coverage:
-    print('Converting coveragepy database to lcov:', output_filename, file=sys.stderr)
+  PrintVerboseCoverage('Converting coveragepy database to lcov:', output_filename)
   # Run coveragepy again to convert its .coverage database file into lcov.
   ret_code = subprocess.call(
     [
@@ -398,11 +399,11 @@ relative_files = True
   ) or ret_code
   try:
     os.unlink(rcfile_name)
-  except OSError:
-    # It's possible that the profiled program might execute another python
+  except OSError as err:
+    # It's possible that the profiled program might execute another Python
     # binary through a wrapper that would then delete the rcfile.  Not much
     # we can do about that, besides ignore the failure here.
-    pass
+    PrintVerboseCoverage('Error removing temporary coverage rc file:', err)
   if os.path.isfile(output_filename):
     UnresolveSymlinks(output_filename)
   return ret_code
@@ -469,9 +470,7 @@ def Main():
   if os.environ.get('COVERAGE_DIR'):
     cov_tool = FindCoverageEntryPoint(module_space)
     if cov_tool is None:
-      if os.environ.get("VERBOSE_COVERAGE"):
-        print('Coverage was enabled, but python coverage tool was not configured.',
-              file=sys.stderr)
+      PrintVerboseCoverage('Coverage was enabled, but python coverage tool was not configured.')
     else:
       # Inhibit infinite recursion:
       if 'PYTHON_COVERAGE' in os.environ: