Skip to content

Commit

Permalink
Fix the Python version select() mechanism to handle PY3-as-default
Browse files Browse the repository at this point in the history
The target @bazel_tools//tools/python:python_version can be used with select() to determine the Python version in the current configuration. However, it was not updated when --incompatible_py3_is_default was added to switch the default to PY3. This meant that if neither --force_python nor --python_version was passed to the build, but --incompatible_py3_is_default was set, it would incorrectly return PY2 when the actual version was PY3. This CL updates this target's definition to take that flag into account.

Fixes the bug in (but does not close) bazelbuild#7615.

RELNOTES: None
PiperOrigin-RevId: 236642947
  • Loading branch information
brandjon authored and copybara-github committed Mar 4, 2019
1 parent 81adec8 commit 33e5719
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ java_test(
name = "PythonVersionSelectTest",
srcs = ["PythonVersionSelectTest.java"],
deps = [
":PythonTestUtils",
"//src/main/java/com/google/devtools/build/lib:build-base",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.rules.python;

import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2;

import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.FileProvider;
Expand Down Expand Up @@ -103,8 +102,6 @@ public void cannotSelectOnForcePythonFlagsWithoutOldApi() throws Exception {
*/
@Test
public void selectOnPythonVersionTarget() throws Exception {
// Expected results assume the default is PY2.
ensureDefaultIsPY2();
// getRuleContext() doesn't populate the information needed to resolve select()s, and
// ConfiguredTarget doesn't allow us to test an end-to-end view of the behavior of a select().
// So this test has the select() control srcs and asserts on which one's in the files to build.
Expand All @@ -120,11 +117,19 @@ public void selectOnPythonVersionTarget() throws Exception {
" }),",
")");

// Default.
doTestSelectOnPythonVersionTarget(py2);
// Neither --python_version nor --force_python, use default value.
doTestSelectOnPythonVersionTarget(py2, "--incompatible_py3_is_default=false");
doTestSelectOnPythonVersionTarget(
py3,
"--incompatible_py3_is_default=true",
// PythonConfiguration has a validation check requiring that the new transition semantics be
// enabled before we're allowed to set the default to PY3.
"--incompatible_allow_python_version_transitions=true");

// No --python_version, trust --force_python.
doTestSelectOnPythonVersionTarget(py2, "--force_python=PY2");
doTestSelectOnPythonVersionTarget(py3, "--force_python=PY3");

// --python_version overrides --force_python.
doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2");
doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2", "--force_python=PY2");
Expand Down
22 changes: 17 additions & 5 deletions tools/python/python_version.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ _TRUE = "TRUE"
_PY2 = "PY2"
_PY3 = "PY3"

# Keep in sync with PythonVersion.DEFAULT_TARGET_VALUE.
_DEFAULT_PYTHON_VERSION = "PY2"

def _python_version_flag_impl(ctx):
# Version is determined using the same logic as in PythonOptions#getPythonVersion:
#
Expand All @@ -34,7 +31,7 @@ def _python_version_flag_impl(ctx):
elif ctx.attr.force_python_flag != _UNSET:
version = ctx.attr.force_python_flag
else:
version = _DEFAULT_PYTHON_VERSION
version = _PY3 if ctx.attr.incompatible_py3_is_default_flag else _PY2

if version not in ["PY2", "PY3"]:
fail("Internal error: _python_version_flag should only be able to " +
Expand All @@ -46,6 +43,7 @@ _python_version_flag = rule(
attrs = {
"force_python_flag": attr.string(mandatory = True, values = [_PY2, _PY3, _UNSET]),
"python_version_flag": attr.string(mandatory = True, values = [_PY2, _PY3, _UNSET]),
"incompatible_py3_is_default_flag": attr.bool(mandatory = True),
},
)

Expand All @@ -60,7 +58,7 @@ def define_python_version_flag(name):
"""

# Config settings for the underlying native flags we depend on:
# --force_python and --python_version.
# --force_python, --python_version, and --incompatible_py3_is_default.
native.config_setting(
name = "_force_python_setting_PY2",
values = {"force_python": "PY2"},
Expand All @@ -81,6 +79,16 @@ def define_python_version_flag(name):
values = {"python_version": "PY3"},
visibility = ["//visibility:private"],
)
native.config_setting(
name = "_incompatible_py3_is_default_setting_false",
values = {"incompatible_py3_is_default": "false"},
visibility = ["//visibility:private"],
)
native.config_setting(
name = "_incompatible_py3_is_default_setting_true",
values = {"incompatible_py3_is_default": "true"},
visibility = ["//visibility:private"],
)

_python_version_flag(
name = name,
Expand All @@ -94,5 +102,9 @@ def define_python_version_flag(name):
":_python_version_setting_PY3": _PY3,
"//conditions:default": _UNSET,
}),
incompatible_py3_is_default_flag = select({
":_incompatible_py3_is_default_setting_false": False,
":_incompatible_py3_is_default_setting_true": True,
}),
visibility = ["//visibility:public"],
)

0 comments on commit 33e5719

Please sign in to comment.