Build fails with bazel 0.25 --incompatible_use_python_toolchains

[tmc]

It seems like the first line in the par has an incomplete path to the wrapper script:

$ head bazel-bin/compiler/compiler.par
#!bazel_tools/tools/python/py2wrapper.sh
PK

[brandjon]

Seems like this is the code at fault. Relative paths get embedded directly whereas the old default "python" interpreter command was passed to /usr/bin/env.

[brandjon]

Possible solutions:

1. Fail-fast whenever the subpar compiler attempts to generate a par file embedding a relative path to an interpreter. The user must somehow ensure that the wrapped py_binary is built with a Python toolchain that refers to a system interpreter.

2. Use /usr/bin/env python as the shebang for the par file, ignoring the interpreter specified by the toolchain. This is what the py_binary stub script does.

[brandjon]

Argument for (2): This is already the default behavior for everyone who doesn't manipulate the python runtime. Who would we be breaking by making this the unconditional par shebang interpreter? Only users who override the python runtime to an absolute path (e.g. with --python_path) AND who need that custom runtime to be used for the stub script of their par files (but not of non-par py_binary stubs in their builds).

[depthwise]

Wouldn't /usr/bin/env python resolve to python2 on some systems though? Or is that not an issue?

user@carbon Dev/work % cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=18.04
DISTRIB_CODENAME=bionic
DISTRIB_DESCRIPTION="Ubuntu 18.04.2 LTS"
user@carbon Dev/work % /usr/bin/env python
Python 2.7.15rc1 (default, Nov 12 2018, 14:31:15) 
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>>

[brandjon]

Yes, but it's only used for the stub script (unless I'm seriously misunderstanding how subpar works). So the stub script needs to be compatible with both Python 2 and Python 3, but the payload gets run with whatever PYTHON_BINARY = ... interpreter path is determined by Bazel during the analysis phase (whether by toolchains or the legacy --python_top / --python_path).

This is how it works for standard py_binary rules. I'm not sure why par files should deviate from this.

[depthwise]

I'm not saying it should deviate, not at all. I was just wondering if that could be problematic. Looks like it won't be: py_binary works fine in 0.25

[tmc]

Would a relative path ever be valid?

[brandjon]

I'm not saying it should deviate

Sure, I was just thinking aloud as I'm newly considering this issue. :)

Would a relative path ever be valid?

Sometimes, and I noticed this case in my own testing to reproduce this issue. For instance, when referencing an in-build runtime defined in the main repo, it so happens that the execroot directory structure allowed it to find the runtime executable. But change it to a runtime defined in an external repo and it no longer works out that way. I think it'd also fail if you invoked it directly without bazel run from an arbitrary current working directory.

The reason not to allow relative paths in the shebang is that the par file ends up being non-relocatable even within the same system. For in-build runtimes, you typically would want to refer to them via workspace-relative paths, so that's one reason not to use them as the shebang. But even if you referred to it via an absolute path, you'd still end up with par files that are sensitive to changes to your source tree or output tree, which happens during the course of normal development workflows.

So IMO the solution should be to only use absolute paths to platform-defined interpreters in the shebang (whether for par files or the standard py_binary stub), and for now specifically to just use #!/usr/bin/env python. Will write a doc on this soon.

[tmc]

That sounds reasonable -- can/should someone open a diff here to make that change?

[aaliddell]

From what I can see, #!/usr/bin/env python would fail if you wanted Python 3, as the contents of the subpar binary are run directly with the specified python, rather than doing the re-exec we see with py_binary stub. So perhaps the rules for interpreter could be:

• If absolute path, use directly (which should handle when people specify their own python toolchain, rather than the default one)
• If py3wrapper.sh, use #!/usr/bin/env python3
• If py2wrapper.sh use #!/usr/bin/env python (or python2?)
• Else, fail

Would you consider a PR with this change? Otherwise, the version finding and re-exec would have to be moved into the par executable, rather than at compile time.

[brandjon]

@aaliddell Thanks for raising that point -- it caused me to look a little deeper into subpar's inner workings.

As best as I can tell, the shebang will choose what Python version executes the zip file, which under Python's semantics means running the __main__.py file. This file is basically the same as the standard stub file. In particular, it too launches a separate interpreter (whose path is saved in the PYTHON_BINARY constant) to run the payload, independent of the stub / zipfile's launching Python process.

I tried this out by patching cli.py to use the standard shebang. This makes the following command pass where it previously failed:

bazel build //tests/package_a:a.par --incompatible_use_python_toolchains

The tests run into some assertion errors because the new runtime wrapper script needs to be added to their expected file manifests. I also expect some PY2 vs PY3 problems since toolchains fix bazelbuild/bazel#4815. But this seems to solve the actual bug.

[brandjon]

Gah, I misread it, the generated __main__.py file is a marked up version of the user payload entry point, not the stub script. I think the stub script is only present as a byproduct of it being in runfiles, and that it's not actually ever executed. That makes sense, because the stub script would extract runfiles and that's clearly redundant with what subpar does.

My previous test only worked because it stopped the shebang from being malformed. But it still chooses the wrong Python version, as was uncovered when trying to run //tests:version_test.

So I think @aaliddell's workaround is the way to go. It bans local paths while still carving out a loophole for people who just want the default Python interpreter.

[aaliddell]

I’ve written that up as #99, although I didn’t add the ‘or fail’ bit there but instead left the existing behaviour for relative paths. Happy to make it fail though if desired.

[brandjon]

Tests still need updating, I'll look.

[depthwise]

Are you guys going to cut a release when this is done? I think it's warranted, since Subpar is broken with the Bazel 0.25 for everyone right now.

[brandjon]

Hm? Only with the incompatible change flag enabled, unless there's a more serious breakage I was unaware of.

Cutting a release sounds warranted. Looks like there's no workflow around it (e.g. auto-generated release notes) other than tagging it as such in github.

[brandjon]

Are you guys going to cut a release when this is done?

I'll double check that rules_python's tests pass and tag a release.

I think it has to be a new major release, i.e. 2.0.0, if we're following semantic versioning, since we drop support for Bazels earlier than 0.23. I'm not sure whether previous releases used this standard but we may as well do it now since I think semantic versioning is strongly encouraged all around the bazel ecosystem.

[brandjon]

Tagged 2.0.0.

[thundergolfer]

Correct me if I'm worng, but I think this is still somewhat broken.

Because there's currently a bug in rules_python, I set --python_top when running tests like so: test --python_top=//tools/build/python:py_tests_runtime. This ensures that when I do bazel test //... I don't have Python2 used and get a bunch of SyntaxErrors.

But this causes problems with subpar, because the above py_runtime is a label location-type like this:

py_runtime(
    name = "my_py3_runtime",
    interpreter = "@python3//:interpreter",
    python_version = "PY3",
)

so you get this error:

subpar.compiler.error.Error: par files require a Python runtime that is installed on the system, not defined inside the workspace. Use a `py_runtime` with an absolute path, not a label.

Because of how bazel test //... works, it's trying to run subpar targets and they're breaking.

For the purposes of tests I can specify a different py_runtime with a hardcoded path to the interpreter, but this doesn't work well with Nix.

---

How do you recommend I sort this out?