Skip to content

Commit

Permalink
Fix the autodetecting Python toolchain on Mac
Browse files Browse the repository at this point in the history
See bazelbuild/continuous-integration#578 for context. The gist of it is that when PATH is not set (as happens when a binary using this toolchain is used as a tool from Starlark), the shell comes up with its own PATH to run the pywrapper script. However, it does not necessarily export this PATH, causing "which" to misbehave and fail to locate a Python interpreter.

The fix is to add "export PATH" and a regression test.

Fixes bazelbuild/continuous-integration#578, work toward #7899.

RELNOTES: None
PiperOrigin-RevId: 249263849
  • Loading branch information
brandjon authored and aehlig committed May 23, 2019
1 parent 85de4f0 commit 25efdab
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 9 deletions.
137 changes: 128 additions & 9 deletions src/test/shell/bazel/python_version_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,12 @@ case "$(uname -s | tr [:upper:] [:lower:])" in
msys*)
# As of 2018-08-14, Bazel on Windows only supports MSYS Bash.
declare -r is_windows=true
# As of 2019-04-26, this test is disabled on Windows (via "no_windows" tag),
# As of 2019-05-18, this test is disabled on Windows (via "no_windows" tag),
# so this code shouldn't even have run.
# TODO(bazelbuild/continuous-integration#578): Enable this test for Windows.
# TODO(#7844): Enable this test for Windows once our autodetecting toolchain
# works transparently for this platform.
fail "This test does not run on Windows."
;;
darwin*)
# As of 2019-04-26, this test is disabled on mac, but there's no "no_mac" tag
# so we just have to trivially succeed.
# TODO(bazelbuild/continuous-integration#578): Enable this test for Mac.
echo "This test does not run on Mac; exiting early." >&2
exit 0
;;
*)
declare -r is_windows=false
;;
Expand Down Expand Up @@ -583,4 +577,129 @@ 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 for context.
# 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"],
python_version = "PY2",
)
py_binary(
name = "fail_tool",
srcs = ["fail_tool.py"],
python_version = "PY2",
)
genrule(
name = "success_gen",
cmd = "$(location :success_tool) > $@",
tools = [":success_tool"],
outs = ["success_out.txt"],
)
genrule(
name = "fail_gen",
cmd = "$(location :fail_tool) > $@",
tools = [":fail_tool"],
outs = ["fail_out.txt"],
)
EOF
cat > test/success_tool.py << EOF
print("Successfully did nothing.")
EOF
cat > test/fail_tool.py << EOF
import sys
sys.exit(1)
EOF

# The warning should be present if the tool
# 1) was built in the host config,
# 2) with the wrong version,
# 3) with toolchains enabled,
# 4) and it failed during execution.
bazel build //test:fail_gen \
--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 \
--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"

# No warning if toolchains are disabled.
bazel build //test:fail_gen \
--incompatible_use_python_toolchains=false --host_force_python=PY2 \
&> $TEST_log && fail "bazel build succeeded (expected failure)"
expect_not_log "program that was built in the host configuration"

# No warning if it succeeded during execution.
bazel build //test:success_gen \
--incompatible_use_python_toolchains=true --host_force_python=PY2 \
&> $TEST_log || fail "bazel build failed (expected success)"
expect_not_log "program that was built in the host configuration"
}

# 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
# second-stage interpreter must be able to tolerate PATH not being set.
function test_py_binary_with_autodetecting_toolchain_usable_as_tool() {
mkdir -p test

cat > test/BUILD << 'EOF'
load(":tooluser.bzl", "tooluser_rule")
py_binary(
name = "tool",
srcs = ["tool.py"],
)
tooluser_rule(
name = "tooluser",
out = "out.txt",
)
EOF
cat > test/tooluser.bzl << EOF
def _tooluser_rule_impl(ctx):
ctx.actions.run(
inputs = [],
outputs = [ctx.outputs.out],
executable = ctx.executable._tool,
arguments = [ctx.outputs.out.path],
)
tooluser_rule = rule(
implementation = _tooluser_rule_impl,
attrs = {
"_tool": attr.label(
executable = True,
default = "//test:tool",
# cfg param is required but its value doesn't matter
cfg = "target"),
"out": attr.output(),
},
)
EOF
cat > test/tool.py << EOF
import sys
with open(sys.argv[1], 'wt') as out:
print("Tool output", file=out)
EOF

bazel build //test:tooluser \
--incompatible_use_python_toolchains=true \
|| fail "bazel build failed"
cat bazel-bin/test/out.txt &> $TEST_log
expect_log "Tool output"
}

run_suite "Tests for how the Python rules handle Python 2 vs Python 3"
7 changes: 7 additions & 0 deletions tools/python/pywrapper_template.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ die() {
exit 1
}

# Make sure PATH is exported. If we're called with PATH unset, as happens when
# we're invoked as a tool during the build, the shell will initialize its own
# PATH but not necessarily export it. This would break our call to `which`. See
# https://github.com/bazelbuild/continuous-integration/issues/578 for more
# information.
export PATH

# Try the "python%VERSION%" command name first, then fall back on "python".
PYTHON_BIN=`which python%VERSION% || echo ""`
USED_FALLBACK=0
Expand Down

0 comments on commit 25efdab

Please sign in to comment.