Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaks with Python toolchains (to be flipped in Bazel 0.27) #456

Closed
brandjon opened this issue May 31, 2019 · 18 comments
Closed

Breaks with Python toolchains (to be flipped in Bazel 0.27) #456

brandjon opened this issue May 31, 2019 · 18 comments

Comments

@brandjon
Copy link
Member

Bazel 0.27 flips --incompatible_use_python_toolchains, which breaks rules_apple according to a presubmit. The flag causes Bazel to actually run Python targets with the version of Python that was requested at analysis time, instead of whatever the python command happens to be. You likely need to add python_version = "PY2" to your targets, and/or enable --host_force_python=PY2. Note that the default version for both host and target config is Python 3 as of Bazel 0.25.

@thomasvl
Copy link
Member

Is that alone going to fix things? The catch we've had in the past is we use py_binary as tool for an action, and that action then is tagged to always run on a mac, but in a mix/remote execution env, the problem is no transitions happen related to that, so the py_binary was built for the host env, not actually for the mac where it gets run. So how to we ensure the py_binary will always work?

@brandjon
Copy link
Member Author

Right, there can only be one host configuration, so unfortunately there can only be one Python version used for all Python targets built in the host configuration. This has always been true, so it is not a regression.

What has changed is that now Bazel enforces that the version you ask for (even implicitly by default) is the version you get. The default Python version is Python 3, and it seems you were expecting to get Python 2. You can tell Bazel to use Python 2 for the host configuration with --host_force_python=PY2.

@thomasvl
Copy link
Member

A global flag doesn't help, we're a rule set, we need to work (with some python tools along the way) no matter what flags the end developer ends up needing to pass.

@brandjon
Copy link
Member Author

For the end user there can only be one version used in the host config. If you have Python tools that require PY2, then so do your users. Both you and they will have to set --host_force_python=PY2.

Again, this has always been the case, it's just more apparent now that the default has changed to PY3 (as of Bazel 0.25) and Bazel is actually enforcing this default (as of Bazel 0.27).

@thomasvl
Copy link
Member

There should be a way to make this "just work", wether they have python 2 or 3 installed, what's the path to that? Anything outside of that is basically going to be hostile to Mac users.

@brandjon
Copy link
Member Author

brandjon commented Jun 3, 2019

Spoke with @sergiocampama on Friday and put my plan in bazelbuild/bazel#8547.

To the point you raise, I think there are two separate cases:

  1. A tool that is built in the host config and legitimately requires a specific version of Python.

  2. A tool that is built in the host config and is source compatible with both versions of Python, and also doesn't have any version-dependent analysis-time constraints (srcs_version or select()s on the Python version).

For the first case, if you have a host tool that legitimately is PY2, then the combination of fixing bazelbuild/bazel#4815 along with the fact that PY3 is now the default means that you had better tell Bazel to use PY3 in the host config with --host_force_python. I don't see a way around this that doesn't regress w.r.t. either bazelbuild/bazel#4815 or PY3-as-default.

For the second case, it seems there's a real need for a way to define a py_binary target to use whichever version of Python the target system has available, when the target itself has no intrinsic constraints. For the host config we really can't do anything and are blocked on replacing host with the exec config. So let's just consider non-host configs.

One idea is to add a new allowed value python_version = "PY2AND3" (reusing the similar concept from srcs_version) to mean "Pick either PY2 or PY3 depending on the current platform". I think this should be possible but will require some implementation effort, and I'd just as soon hold off on it until rules are migrated to Starlark if possible.

The other idea is to have users indirect their dependencies, so that instead of depending on a py_binary that has a fixed version PY2 or PY3, you'd depend on a target that uses select() on the current platform to choose either a PY2 binary or a PY3 binary. This can be made more elegant with macros.

In any case, my main goal now is to be less hostile to all users by advising them of the need to set --host_force_python=PY2, and to be less hostile to mac users specifically by adding the option of a non-strict Python toolchain (bazelbuild/bazel#8547).

@brandjon
Copy link
Member Author

brandjon commented Jun 4, 2019

The solution I outlined for mac users in bazelbuild/bazel#8547 (break them once with instructions on how to opt out of strict version checking) is marked as a Bazel release blocker.

In the meantime, can we add the --host_force_python=PY2 to the bazelrc to fix CI?

@thomasvl
Copy link
Member

thomasvl commented Jun 5, 2019

CI should be fixed up (just the one on head bazel, the latest bazel doesn't have the flag set yet, waiting to see if there is better resolution that forcing all developers to also set the flag)

@brandjon
Copy link
Member Author

brandjon commented Jun 6, 2019

I'm still seeing failures in this run from last night (~8pm EST).

I suspect that e385296 and fac34c5 only set the flag in rules_apple's own CI pipeline, not the collective Bazel@HEAD + downstream projects pipeline. That would require setting --host_force_python in the top-level .bazelrc file (+@philwo to confirm).

I understand your reluctance to add a bazelrc flag in so far as it signifies that users are supposed to update their own bazelrcs. But they'll have to do that anyway, regardless of whether the downstream projects pipeline turns green for rules_apple or not.

Bazel does not currently provide a built-in way to automatically select Python 2 or 3 mode for binaries based on the user's platform, and never has (although one can probably be written using macros as I mentioned above). The fact that it happened to work before was a bug (bazelbuild/bazel#4815 specifically).

@thomasvl
Copy link
Member

thomasvl commented Jun 6, 2019

The rc file won't help the integration tests, it is failing in the workspaces created within those tests. fac34c5 went in an hour ago to hopefully finish catching those issues.

@brandjon
Copy link
Member Author

brandjon commented Jun 6, 2019

Oh, I see. Thanks!

+@laurentlb, can we get a rerun of either the RC's or the bazel@head+downstream pipeline to verify this fix?

@thomasvl
Copy link
Member

thomasvl commented Jun 6, 2019

There's atleast one more test that isn't channeling thru our common plumbing and hitting a python issue. I've got some cls in the pipeline fixing things that need to land first, once that's in, I'll send out another CL to get the last place I know of hitting python differences.

@laurentlb
Copy link

laurentlb commented Jun 6, 2019

@thomasvl
Copy link
Member

thomasvl commented Jun 7, 2019

@brandjon is this flag still going to flip in 0.27 meaning you will force all devs to use the python flag or is it going to get flipped back of pending resolution for things like us where we use python within the rules can shouldn't require global flags?

@brandjon
Copy link
Member Author

brandjon commented Jun 7, 2019

The plan is to flip in 0.27 and force people to add --host_force_python=PY2 to their bazelrc if they need it now. The alternative is to not flip Python toolchains until 1.0, which I think prolongs confusion for everyone and defers this complexity to a release that's already going to include a number of other changes.

There's a few different use cases here:

  1. People who need to use host-configured Python tools that require Python 2 rather than 3. Here it makes sense to require the flag in their bazelrc, because it declares something that is already true of their build -- that they require PY2 for the host config in place of PY3 (the default).

  2. Mac users who do not have PY3 installed on their system, but have gotten by because "PY3"-analyzed targets (host or otherwise) are in reality compatible with PY2. For these users I created an opt-out for version checking in the form of the non-strict toolchain (Improve mac Python 2 toolchain story bazel#8547).

  3. Rule authors who want to write a Python binary target that is compatible with either Python version automatically. If users have both Python 2 and Python 3 installed, then it doesn't matter what happens here. For Mac users who don't have Python 3, well that's covered by the above case. For other platforms I'd expect Python 3 (the default in Bazel) to be available. Furthermore, if we want to support the case of only one Python version available without requiring the non-strict toolchain, we can do the indirecting-through-select() trick I mentioned, though that doesn't work in the host config (which is eventually going away).

Edit: I should add, the non-strict toolchain also works on non-mac platforms, should it be needed.

swiple-rules-gardener pushed a commit that referenced this issue Jun 10, 2019
Working toward #456.

- Add a wrap to test under python 2 and 3.
- Minor tweaks to improve support for both pythons.

RELNOTES: None.
PiperOrigin-RevId: 252104076
swiple-rules-gardener pushed a commit that referenced this issue Jun 10, 2019
Working toward #456.

- Add a wrap to test under python 2 and 3.
- Minor tweaks to improve support for both pythons.
- Tweak the tools a little for dual support.

RELNOTES: None.
PiperOrigin-RevId: 252104076
swiple-rules-gardener pushed a commit that referenced this issue Jun 10, 2019
Working toward #456.

- Add a wrap to test under python 2 and 3.
- Minor tweaks to improve support for both pythons.
- Tweak the tools a little for dual support.

RELNOTES: None.
PiperOrigin-RevId: 252104076
swiple-rules-gardener pushed a commit that referenced this issue Jun 10, 2019
Working toward #456.

- Add a wrap to test under python 2 and 3.
- Minor tweaks to improve support for both pythons.
- Tweak the tools a little for dual support.

RELNOTES: None.
PiperOrigin-RevId: 252428068
@thomasvl
Copy link
Member

#472 removes forcing the flag, and we seem to have things passing now like this. Hopefully that also means it "just works" no matter what python a user has installed.

@brandjon anything else we should do for this at the moment, or should we close this as hopefully fixed?

@brandjon
Copy link
Member Author

If the target is truly compatible with both, in its source code and in the target attributes, transitively, then it should be fine. (Also discussed this use case here.)

@brandjon
Copy link
Member Author

brandjon commented Jun 11, 2019

Verified in buildkite. Thanks.

Do keep in mind that users may still have to add --extra_toolchains=@bazel_tools//tools/python:autodetecting_toolchain_nonstrict, if they don't have a Python 3 interpreter installed. This is unrelated to your project, and is just the way that Bazel 0.27 will allow people to opt out of strict version checking. Users are instructed to do this by the error message that results if a Python 3 interpreter cannot be found. (This error condition is triggered by attempting to run a target analyzed as PY3, even if it doesn't actually contain any real Python 3-specific source code.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants