From 33e571939085dd158422e1b3503cfc738e0a3165 Mon Sep 17 00:00:00 2001 From: brandjon Date: Mon, 4 Mar 2019 06:37:01 -0800 Subject: [PATCH] Fix the Python version select() mechanism to handle PY3-as-default 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) #7615. RELNOTES: None PiperOrigin-RevId: 236642947 --- .../devtools/build/lib/rules/python/BUILD | 1 - .../rules/python/PythonVersionSelectTest.java | 15 ++++++++----- tools/python/python_version.bzl | 22 ++++++++++++++----- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD index 8c62fd54a2e6b4..330b2c9ba35c2b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD @@ -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", diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java index c9541183bb3d3a..3ff0e537e7e03b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java @@ -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; @@ -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. @@ -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"); diff --git a/tools/python/python_version.bzl b/tools/python/python_version.bzl index 05260e124b4f74..8bb45b86c0dce4 100644 --- a/tools/python/python_version.bzl +++ b/tools/python/python_version.bzl @@ -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: # @@ -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 " + @@ -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), }, ) @@ -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"}, @@ -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, @@ -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"], )