Skip to content

Commit

Permalink
Remove ancient PY2 warning messages from python_stub_template.txt
Browse files Browse the repository at this point in the history 
These messages are rarely helpful and often misleading. The associated TODO (#6443) was closed years ago.

Closes #15783.

PiperOrigin-RevId: 462371940
Change-Id: I62f903f66927fdd7fb2e8c1205b1af2b220677a1
  • Loading branch information
@jvolkman
jvolkman authored and Copybara-Service committed Jul 21, 2022
1 parent e603222 commit b3dacd4
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 265 deletions.
3 changes: 0 additions & 3 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,6 @@ private static void createStubFile(
Substitution.of("%is_zipfile%", boolToLiteral(isForZipFile)),
Substitution.of(
"%import_all%", boolToLiteral(bazelConfig.getImportAllRepositories())),
Substitution.of(
"%enable_host_version_warning%",
boolToLiteral(common.shouldWarnAboutHostVersionUponFailure())),
Substitution.of("%target%", ruleContext.getRule().getLabel().getCanonicalForm()),
Substitution.of(
"%python_version_from_config%", versionToLiteral(common.getVersion())),
Expand Down
87 changes: 1 addition & 86 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,84 +205,6 @@ def RunfilesEnvvar(module_space):

return (None, None)

# TODO(#6443): Remove this once there's no longer a host configuration for
# Python targets to appear in.
def MaybeEmitHostVersionWarning(ret_code):
"""Warns 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) 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 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:
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}.\
""".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}

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(diagnostic=diagnostic)
print(message, file=sys.stderr)

def Deduplicate(items):
"""Efficiently filter out duplicates, keeping the first element only."""
seen = set()
Expand Down Expand Up @@ -379,19 +301,12 @@ def Main():
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%:
if IsWindows():
ret_code = subprocess.call(args)
MaybeEmitHostVersionWarning(ret_code)
sys.exit(ret_code)
else:
os.execv(args[0], args)
Expand Down
62 changes: 0 additions & 62 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,68 +580,6 @@ public PythonVersion getSourcesVersion() {
return sourcesVersion;
}

/**
* Returns whether, in the case that a user Python program fails, the stub script should emit a
* warning that the failure may have been caused by the host configuration using the wrong Python
* version.
*
* <p>This method should only be called for executable Python rules.
*
* <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>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 #readPythonVersionFromAttribute}.
*/
// 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().isToolConfiguration()) {
return false;
}

PythonVersion configVersion = config.getPythonVersion();
PythonVersion attrVersion = readPythonVersionFromAttribute(ruleContext.attributes());
if (attrVersion == null) {
// 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;
}
}

/**
* Returns the transitive Python sources collected from the deps attribute, not including sources
* from the srcs attribute (unless they were separately reached via deps).
Expand Down
114 changes: 0 additions & 114 deletions src/test/shell/bazel/python_version_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,120 +601,6 @@ function test_default_output_root_is_bazel_bin() {
expect_not_log "bazel-out/.*-py3.*/bin"
}

# Tests that we get a warning when host tools fail due to being built for the
# 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 = "passing_tool_using_py2_explicitly",
srcs = ["success.py"],
main = "success.py",
python_version = "PY2",
)
py_binary(
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 = "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 = "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.py << EOF
print("Successfully did nothing.")
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 with toolchains enabled,
# 2) in the host config,
# 3) with a mismatched version, or a version set implicitly,
# 4) and it failed during execution.

# 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"

# 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 "Note: The above Python target's failure"

# No warning if toolchains are disabled.
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 "Note: The above Python target's failure"

# No warning if it succeeded during execution.
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 "Note: The above Python target's failure"
}

# Regression test for (bazelbuild/continuous-integration#578): Ensure that a
# py_binary built with the autodetecting toolchain works when used as a tool
# from Starlark rule. In particular, the wrapper script that launches the real
Expand Down

0 comments on commit b3dacd4

Please sign in to comment.