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 bazelbuild#7899.

RELNOTES: None
PiperOrigin-RevId: 249263849
  • Loading branch information
brandjon authored and copybara-github committed May 21, 2019
1 parent 0ff19c6 commit 7f49531
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 9 deletions.
66 changes: 57 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 @@ -654,4 +648,58 @@ configuration, which uses Python 3"
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 @@ -15,6 +15,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 7f49531

Please sign in to comment.