Warn in more cases of possible Python version mismatch in host config

We already emit a diagnostic message when the host config's Python version overrides a Python tool's normal version. This helps users know to add --host_force_python=PY2 to their bazelrc. Without it, all they'd see is a confusing Python stack trace.

With this change, we also emit a diagnostic message when the target's python_version attribute is omitted and the user has not set --host_force_python=PY2. This covers the case where both the target and the host config default to PY3, but the target was really intended to be a Python 2 target. Previously, this case sometimes worked anyway due to bazelbuild#4815, but with Python toolchains enabled it breaks and we need to help the user understand the breakage.

In all cases the diagnostic is still only printed when the target has non-zero exit code and is built in host config.

See also bazelbuild#7899. Fixes bazelbuild#8549.

RELNOTES: None
PiperOrigin-RevId: 251496135

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java

@@ -17,6 +17,7 @@
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -132,10 +133,15 @@ public Artifact getPythonIntermediateStubArtifact(RuleContext ruleContext, Artif
     return ruleContext.getRelatedArtifact(executable.getRootRelativePath(), ".temp");
   }
 
-  private static String booleanToPythonLiteral(boolean value) {
+  private static String boolToLiteral(boolean value) {
     return value ? "True" : "False";
   }
 
+  private static String versionToLiteral(PythonVersion version) {
+    Preconditions.checkArgument(version.isTargetValue());
+    return version == PythonVersion.PY3 ? "\"3\"" : "\"2\"";
+  }
+
   @Override
   public void createExecutable(
       RuleContext ruleContext, PyCommon common, CcInfo ccInfo, Runfiles.Builder runfilesBuilder)
@@ -177,6 +183,12 @@ public void createExecutable(
     // first-stage.
     String pythonBinary = getPythonBinary(ruleContext, common, bazelConfig);
 
+    // Version information for host config diagnostic warning.
+    PythonVersion attrVersion = PyCommon.readPythonVersionFromAttributes(ruleContext.attributes());
+    boolean attrVersionSpecifiedExplicitly = attrVersion != null;
+    if (!attrVersionSpecifiedExplicitly) {
+      attrVersion = config.getDefaultPythonVersion();
+    }
     // Create the stub file.
     ruleContext.registerAction(
         new TemplateExpansionAction(
@@ -189,14 +201,20 @@ public void createExecutable(
                 Substitution.of("%python_binary%", pythonBinary),
                 Substitution.of("%imports%", Joiner.on(":").join(common.getImports())),
                 Substitution.of("%workspace_name%", ruleContext.getWorkspaceName()),
-                Substitution.of("%is_zipfile%", booleanToPythonLiteral(buildPythonZip)),
+                Substitution.of("%is_zipfile%", boolToLiteral(buildPythonZip)),
                 Substitution.of(
-                    "%import_all%", booleanToPythonLiteral(bazelConfig.getImportAllRepositories())),
+                    "%import_all%", boolToLiteral(bazelConfig.getImportAllRepositories())),
                 Substitution.of(
                     "%enable_host_version_warning%",
-                    booleanToPythonLiteral(common.shouldWarnAboutHostVersionUponFailure())),
+                    boolToLiteral(common.shouldWarnAboutHostVersionUponFailure())),
+                Substitution.of(
+                    "%target%", ruleContext.getRule().getLabel().getDefaultCanonicalForm()),
+                Substitution.of(
+                    "%python_version_from_config%", versionToLiteral(common.getVersion())),
+                Substitution.of("%python_version_from_attr%", versionToLiteral(attrVersion)),
                 Substitution.of(
-                    "%python_version%", common.getVersion() == PythonVersion.PY3 ? "'3'" : "'2'")),
+                    "%python_version_specified_explicitly%",
+                    boolToLiteral(attrVersionSpecifiedExplicitly))),
             true));
 
     // Create the zip file if requested. On unix, copy it from the intermediate artifact to the

src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt

@@ -179,44 +179,78 @@ def RunfilesEnvvar(module_space):
 def MaybeEmitHostVersionWarning(ret_code):
   """Warn the user if a failure may be due to the host config's version.
 
-  This emits a message to stderr if 1) ret_code is non-zero, 2) a mismatch
-  between this target's declared version and its configured version was
-  detected during analysis (see #6443), and 3) toolchains are enabled (see
-  #7899).
+  This emits a message to stderr if
+    1) ret_code is non-zero,
+    2) the target was built in the host config and with toolchains enabled, and
+    3) at analysis time we detected a mismatch between the host config's version
+       and this target's explicitly declared version, or else this target did
+       not explicitly declare its version. (The former diagnoses targets
+       affected by #6443, and the latter diagnoses targets that are broken by
+       fixing #4815.)
+
+  See also #7899, #8549, and PyCommon#shouldWarnAboutHostVersionUponFailure.
 
   Since this warning is emitted here in the stub script and not in Bazel itself,
-  it will be present in all failing runs of host-configured targets built with
-  the wrong Python version, even when executed directly and not via `bazel run`.
-  However, note that this warning is never added to non-host-configured targets,
-  and that it can be disabled by ensuring the correct Python version is passed
-  to --host_force_python.
+  it will be present in all failing runs of affected targets, even when executed
+  directly and not via `bazel run`. However, note that this warning is never
+  added to non-host-configured targets, and that it can be disabled by ensuring
+  the correct Python version is passed to --host_force_python and declared in
+  tools' python_version attributes.
 
   Args:
       ret_code: The exit code of the payload user program
   """
-  if ret_code != 0 and %enable_host_version_warning%:
-      host_version = %python_version%
-      target_version = "2" if host_version == "3" else "3"
-      # TODO(brandjon): Change the wording "You are likely seeing this message
-      # because" to something less strong after a few releases from 0.27. By
-      # that point, migration for toolchains won't be the main reason this error
-      # is seen by users.
-      print("""\
-----------------
-Note: The above Python target's failure (exit code {ret_code}) may have been \
+  if ret_code == 0:
+    return
+  if not %enable_host_version_warning%:
+    return
+
+  host_version = %python_version_from_config%
+  target_version = %python_version_from_attr%
+  opposite_of_host_version = "2" if host_version == "3" else "3"
+
+  if %python_version_specified_explicitly%:
+    # Mismatch with explicitly declared version.
+    diagnostic = """\
+Note: The failure of target {target} (with exit code {ret_code}) may have been \
 caused by the fact that it is a Python {target_version} program that was built \
 in the host configuration, which uses Python {host_version}. You can change \
 the host configuration (for the entire build) to instead use Python \
-{target_version} by setting --host_force_python=PY{target_version}.
+{target_version} by setting --host_force_python=PY{target_version}.\
+""".format(
+    target="%target%",
+    ret_code=ret_code,
+    target_version=target_version,
+    host_version=host_version)
+  else:
+    diagnostic = """\
+Note: The failure of target {target} (with exit code {ret_code}) may have been \
+caused by the fact that it is running under Python {host_version} instead of \
+Python {opposite_of_host_version}. Examine the error to determine if that \
+appears to be the problem. Since this target is built in the host \
+configuration, the only way to change its version is to set \
+--host_force_python=PY{opposite_of_host_version}, which affects the entire \
+build.\
+""".format(
+    target="%target%",
+    ret_code=ret_code,
+    host_version=host_version,
+    opposite_of_host_version=opposite_of_host_version)
+
+  # TODO(brandjon): Change the wording "You are likely seeing this message
+  # because" to something less strong after a few releases from 0.27. By that
+  # point, migration for toolchains won't be the main reason this error is seen
+  # by users.
+  message = """\
+----------------
+{diagnostic}
 
-You are likely seeing this message because the way Bazel locates the Python \
-interpreter changed in 0.27. See \
+If this error started occurring in Bazel 0.27 and later, it may be because the \
+Python toolchain now enforces that targets analyzed as PY2 and PY3 run under a \
+Python 2 and Python 3 interpreter, respectively. See \
 https://github.com/bazelbuild/bazel/issues/7899 for more information.
-----------------""".format(
-                ret_code=ret_code,
-                target_version=target_version,
-                host_version=host_version),
-            file=sys.stderr)
+----------------""".format(diagnostic=diagnostic)
+  print(message, file=sys.stderr)
 
 def Main():
   args = sys.argv[1:]
@@ -272,7 +306,7 @@ def Main():
     sys.stdout.flush()
     if IsRunningFromZip():
       # If RUN_UNDER_RUNFILES equals 1, it means we need to
-      # change directory to the right runfiles directory.S
+      # 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%"))
@@ -283,8 +317,11 @@ def Main():
     else:
       # On Windows, os.execv doesn't handle arguments with spaces correctly,
       # and it actually starts a subprocess just like subprocess.call.
-      # If we have a host config version mismatch, don't execv because we may
-      # need to inject a diagnostic message if the user code exits abnormally.
+      #
+      # 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)

src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java

@@ -655,37 +655,59 @@ public PythonVersion getSourcesVersion() {
    *
    * <p>This method should only be called for executable Python rules.
    *
-   * <p>Background: Executable Python rules have a rule transition that sets the version to the one
-   * declared by the target's attributes ({@code python_version} / {@code default_python_version}).
-   * If there's any discrepancy, that means the rule transition didn't actually have any effect.
-   * This can only happen in the host configuration, when the target's version is the opposite of
-   * the value of {@code --host_force_python}. Running a program under the wrong Python interpreter
-   * version can lead to confusing tracebacks, therefore we try to be helpful by appending a
-   * diagnostic message to stderr. See #7899 for context.
+   * <p>Background: Historically, Bazel did not necessarily launch a Python interpreter whose
+   * version corresponded to the one determined by the analysis phase (#4815). Enabling Python
+   * toolchains fixed this bug. However, this caused some builds to break due to targets that
+   * contained Python-2-only code yet got analyzed for (and now run with) Python 3. This is
+   * particularly problematic for the host configuration, where the value of {@code
+   * --host_force_python} overrides the declared or implicit Python version of the target.
    *
-   * <p>This method only returns true when 1) a version mismatch is detected, and 2) Python
-   * toolchains are enabled. Toolchains make it more likely that the Python runtime invoked at
-   * execution time matches the version decided at analysis time (fixing #4815). Therefore, when
-   * toolchains are enabled the warning is 1) more important, because many builds start failing due
-   * to getting the "correct" interpreter for the first time (see #7899), and 2) more accurate,
-   * because it correctly describes which interpreter version was actually used.
+   * <p>Our mitigation for this is to warn users when a Python target has a non-zero exit code and
+   * the failure could be due to a bad Python version in the host configuration. In this case,
+   * instead of just giving the user a confusing traceback of a PY2 vs PY3 error, we append a
+   * diagnostic message to stderr. See #7899 and especially #8549 for context.
+   *
+   * <p>This method returns true when all of the following hold:
+   *
+   * <ol>
+   *   <li>Python toolchains are enabled. (The warning is needed the most when toolchains are
+   *       enabled, since that's an incompatible change likely to cause breakages. At the same time,
+   *       warning when toolchains are disabled could be misleading, since we don't actually know
+   *       whether the interpreter invoked at runtime is correct.)
+   *   <li>The target is built in the host configuration. This avoids polluting stderr with spurious
+   *       warnings for non-host-configured targets, while covering the most problematic case.
+   *   <li>Either the value of {@code --host_force_python} overrode the target's normal Python
+   *       version to a different value (in which case we know a mismatch occurred), or else {@code
+   *       --host_force_python} is in agreement with the target's version but the target's version
+   *       was set by default instead of explicitly (in which case we suspect the target may have
+   *       been defined incorrectly).
+   * </ol>
    *
    * @throws IllegalArgumentException if there is a problem parsing the Python version from the
    *     attributes; see {@link #readPythonVersionFromAttributes}.
    */
   // TODO(#6443): Remove this logic and the corresponding stub script logic once we no longer have
   // the possibility of Python binaries appearing in the host configuration.
   public boolean shouldWarnAboutHostVersionUponFailure() {
+    // Only warn when toolchains are used.
     PythonConfiguration config = ruleContext.getFragment(PythonConfiguration.class);
     if (!config.useToolchains()) {
       return false;
     }
+    // Only warn in the host config.
+    if (!ruleContext.getConfiguration().isHostConfiguration()) {
+      return false;
+    }
+
     PythonVersion configVersion = config.getPythonVersion();
     PythonVersion attrVersion = readPythonVersionFromAttributes(ruleContext.attributes());
     if (attrVersion == null) {
-      attrVersion = config.getDefaultPythonVersion();
+      // Warn if the version wasn't set explicitly.
+      return true;
+    } else {
+      // Warn if the explicit version is different from the host config's version.
+      return configVersion != attrVersion;
     }
-    return configVersion != attrVersion;
   }
 
   /**