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 1be0a2470bc72d..b719f297dd379d 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 @@ -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 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 2b22666af0c654..c9d5d3b837036a 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 @@ -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) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java index 295a944c7c3e88..0c31dbf1d291d7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java @@ -655,20 +655,33 @@ public PythonVersion getSourcesVersion() { * *

This method should only be called for executable Python rules. * - *

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. + *

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. * - *

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. + *

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. + * + *

This method returns true when all of the following hold: + * + *

    + *
  1. 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.) + *
  2. 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. + *
  3. 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). + *
* * @throws IllegalArgumentException if there is a problem parsing the Python version from the * attributes; see {@link #readPythonVersionFromAttributes}. @@ -676,16 +689,25 @@ public PythonVersion getSourcesVersion() { // 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; } /** diff --git a/src/test/shell/bazel/python_version_test.sh b/src/test/shell/bazel/python_version_test.sh index 73b173530da5c6..d6f401998ede33 100755 --- a/src/test/shell/bazel/python_version_test.sh +++ b/src/test/shell/bazel/python_version_test.sh @@ -598,74 +598,117 @@ function test_default_output_root_is_bazel_bin() { } # Tests that we get a warning when host tools fail due to being built for the -# wrong Python version. See #7899 for context. +# wrong Python version. See #7899 and #8549 for context, as well as +# {@link PyCommon#shouldWarnAboutHostVersionUponFailure}. # TODO(#6443): Delete this once we no longer have the host configuration. function test_host_version_mismatch_warning() { mkdir -p test cat > test/BUILD << 'EOF' py_binary( - name = "success_tool", - srcs = ["success_tool.py"], + name = "passing_tool_using_py2_explicitly", + srcs = ["success.py"], + main = "success.py", python_version = "PY2", ) py_binary( - name = "fail_tool", - srcs = ["fail_tool.py"], + name = "failing_tool_using_py2_explicitly", + srcs = ["fail.py"], + main = "fail.py", python_version = "PY2", ) +py_binary( + name = "failing_tool_using_py3_explicitly", + srcs = ["fail.py"], + main = "fail.py", + python_version = "PY3", +) + +py_binary( + name = "failing_tool_using_py3_implicitly", + srcs = ["fail.py"], + main = "fail.py", +) + +# For each tool, a genrule target that uses it. + +genrule( + name = "invoke_passing_tool_using_py2_explicitly", + cmd = "$(location :passing_tool_using_py2_explicitly) > $@", + tools = [":passing_tool_using_py2_explicitly"], + outs = ["passing_tool_using_py2_explicitly.txt"], +) + genrule( - name = "success_gen", - cmd = "$(location :success_tool) > $@", - tools = [":success_tool"], - outs = ["success_out.txt"], + name = "invoke_failing_tool_using_py2_explicitly", + cmd = "$(location :failing_tool_using_py2_explicitly) > $@", + tools = [":failing_tool_using_py2_explicitly"], + outs = ["failing_tool_using_py2_explicitly.txt"], ) genrule( - name = "fail_gen", - cmd = "$(location :fail_tool) > $@", - tools = [":fail_tool"], - outs = ["fail_out.txt"], + name = "invoke_failing_tool_using_py3_explicitly", + cmd = "$(location :failing_tool_using_py3_explicitly) > $@", + tools = [":failing_tool_using_py3_explicitly"], + outs = ["failing_tool_using_py3_explicitly.txt"], +) + +genrule( + name = "invoke_failing_tool_using_py3_implicitly", + cmd = "$(location :failing_tool_using_py3_implicitly) > $@", + tools = [":failing_tool_using_py3_implicitly"], + outs = ["failing_tool_using_py3_implicitly.txt"], ) EOF - cat > test/success_tool.py << EOF + cat > test/success.py << EOF print("Successfully did nothing.") EOF - cat > test/fail_tool.py << EOF + cat > test/fail.py << EOF import sys sys.exit(1) EOF + # Relies on --incompatible_py3_is_default being true (flipped in Bazel 0.25). + # The warning should be present if the tool - # 1) was built in the host config, - # 2) with the wrong version, - # 3) with toolchains enabled, + # 1) was built with toolchains enabled, + # 2) in the host config, + # 3) with a mismatched version, or a version set implicitly, # 4) and it failed during execution. - bazel build //test:fail_gen \ + + # Warning should be present due to explicit version mismatch with host config. + bazel build //test:invoke_failing_tool_using_py2_explicitly \ --incompatible_use_python_toolchains=true --host_force_python=PY3 \ &> $TEST_log && fail "bazel build succeeded (expected failure)" expect_log "it is a Python 2 program that was built in the host \ configuration, which uses Python 3" - # No warning if there was no version mismatch. - bazel build //test:fail_gen \ + # Warning should be present due to implicitly set version, even though it + # matches host config. + bazel build //test:invoke_failing_tool_using_py3_implicitly \ + --incompatible_use_python_toolchains=true --host_force_python=PY3 \ + &> $TEST_log && fail "bazel build succeeded (expected failure)" + expect_log "it is running under Python 3 instead of Python 2" + + # No warning if version was set explicitly and matches host config. + bazel build //test:invoke_failing_tool_using_py2_explicitly \ --incompatible_use_python_toolchains=true --host_force_python=PY2 \ &> $TEST_log && fail "bazel build succeeded (expected failure)" - expect_not_log "program that was built in the host configuration" + expect_not_log "Note: The above Python target's failure" # No warning if toolchains are disabled. - bazel build //test:fail_gen \ - --incompatible_use_python_toolchains=false --host_force_python=PY2 \ + bazel build //test:invoke_failing_tool_using_py2_explicitly \ + --incompatible_use_python_toolchains=false --host_force_python=PY3 \ &> $TEST_log && fail "bazel build succeeded (expected failure)" - expect_not_log "program that was built in the host configuration" + expect_not_log "Note: The above Python target's failure" # No warning if it succeeded during execution. - bazel build //test:success_gen \ - --incompatible_use_python_toolchains=true --host_force_python=PY2 \ + bazel build //test:invoke_passing_tool_using_py2_explicitly \ + --incompatible_use_python_toolchains=true --host_force_python=PY3 \ &> $TEST_log || fail "bazel build failed (expected success)" - expect_not_log "program that was built in the host configuration" + expect_not_log "Note: The above Python target's failure" } # Regression test for (bazelbuild/continuous-integration#578): Ensure that a