Prohibit select()ing on certain Python flags

The proper way to select on the python version is to have a config_setting depend (via the flag_values attribute) on @bazel_tools//tools/python:python_version. Reading the value of the native flags --python_version, --force_python, or --host_force_python is not guaranteed to give the actual Python version mode, and can lead to action conflicts.

This CL makes it an error to select on any of these three native flags when --experimental_better_python_version_mixing is enabled. For --python_version, which is currently experimental, this CL also disallows it even when the experimental flag is not enabled.

Work toward #6583 and #6501.

RELNOTES: None
PiperOrigin-RevId: 227046222

src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java

@@ -16,14 +16,18 @@
 
 import com.google.common.base.Ascii;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionMetadataTag;
+import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
+import java.util.Map;
 
 /**
  * Python-related command-line options.
@@ -106,6 +110,9 @@ public String getTypeDescription() {
               + "explicitly), so there is usually not much reason to supply this flag.")
   public PythonVersion pythonVersion;
 
+  private static final OptionDefinition PYTHON_VERSION_DEFINITION =
+      OptionsParser.getOptionDefinitionByName(PythonOptions.class, "python_version");
+
   /**
    * This field should be either null, {@code PY2}, or {@code PY3}. Other {@code PythonVersion}
    * values do not represent distinct Python versions and are not allowed.
@@ -126,6 +133,9 @@ public String getTypeDescription() {
       help = "Overrides default_python_version attribute. Can be \"PY2\" or \"PY3\".")
   public PythonVersion forcePython;
 
+  private static final OptionDefinition FORCE_PYTHON_DEFINITION =
+      OptionsParser.getOptionDefinitionByName(PythonOptions.class, "force_python");
+
   /**
    * This field should be either null, {@code PY2}, or {@code PY3}. Other {@code PythonVersion}
    * values do not represent distinct Python versions and are not allowed.
@@ -143,6 +153,28 @@ public String getTypeDescription() {
               + " Can be \"PY2\" or \"PY3\".")
   public PythonVersion hostForcePython;
 
+  private static final OptionDefinition HOST_FORCE_PYTHON_DEFINITION =
+      OptionsParser.getOptionDefinitionByName(PythonOptions.class, "host_force_python");
+
+  @Override
+  public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {
+    // TODO(brandjon): Add an error string that references documentation explaining to use
+    // @bazel_tools//tools/python:python_version instead.
+    ImmutableMap.Builder<OptionDefinition, SelectRestriction> restrictions = ImmutableMap.builder();
+    restrictions.put(
+        PYTHON_VERSION_DEFINITION,
+        new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null));
+    if (experimentalBetterPythonVersionMixing) {
+      restrictions.put(
+          FORCE_PYTHON_DEFINITION,
+          new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null));
+      restrictions.put(
+          HOST_FORCE_PYTHON_DEFINITION,
+          new SelectRestriction(/*visibleWithinToolsPackage=*/ false, /*errorMessage=*/ null));
+    }
+    return restrictions.build();
+  }
+
   /**
    * Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for.
    *

src/test/shell/bazel/python_version_test.sh

@@ -98,31 +98,21 @@ function use_system_python_2_3_runtimes() {
 'python2', 'python3'"
   fi
 
+  # Point Python builds at a py_runtime target defined in a //tools package of
+  # the main repo. This is not related to @bazel_tools//tools/python.
   add_to_bazelrc "build --python_top=//tools/python:default_runtime"
 
   mkdir -p tools/python
 
   cat > tools/python/BUILD << EOF
 package(default_visibility=["//visibility:public"])
 
-sh_binary(
-    name = '2to3',
-    srcs = ['2to3.sh']
-)
-
-config_setting(
-    name = "py3_mode",
-    values = {"force_python": "PY3"},
-)
-
-# TODO(brandjon): Replace dependency on "force_python" with a 2-valued feature
-# flag instead
 py_runtime(
     name = "default_runtime",
     files = [],
     interpreter_path = select({
-        "py3_mode": "${PYTHON3_BIN}",
-        "//conditions:default": "${PYTHON2_BIN}",
+        "@bazel_tools//tools/python:PY2": "${PYTHON2_BIN}",
+        "@bazel_tools//tools/python:PY3": "${PYTHON3_BIN}",
     }),
 )
 EOF

src/test/shell/integration/python_test.sh

@@ -239,4 +239,73 @@ EOF
   do_test_select_on_python_version "$EXPFLAG --force_python=PY3 --python_version=PY3" "PY3"
 }
 
+function test_cannot_select_on_python_version_native_flags() {
+  mkdir -p test
+
+  cat > test/BUILD << EOF
+config_setting(
+    name = "badconfig_pv",
+    values = {"python_version": "PY2"},
+)
+
+config_setting(
+    name = "badconfig_fp",
+    values = {"force_python": "PY2"},
+)
+
+config_setting(
+    name = "badconfig_hfp",
+    values = {"host_force_python": "PY2"},
+)
+
+sh_binary(name = "bad_pv",
+    srcs = select({
+        ":badconfig_pv": ["bad.sh"],
+        "//conditions:default": ["bad.sh"],
+    }),
+)
+
+sh_binary(name = "bad_fp",
+    srcs = select({
+        ":badconfig_fp": ["bad.sh"],
+        "//conditions:default": ["bad.sh"],
+    }),
+)
+
+sh_binary(name = "bad_hfp",
+    srcs = select({
+        ":badconfig_hfp": ["bad.sh"],
+        "//conditions:default": ["bad.sh"],
+    }),
+)
+EOF
+
+  touch bad.sh
+  chmod u+x bad.sh
+
+  EXPFLAG="--experimental_better_python_version_mixing=true"
+  NO_EXPFLAG="--experimental_better_python_version_mixing=false"
+
+  # --python_version is always unselectable.
+
+  bazel build //test:bad_pv $NO_EXPFLAG \
+      &> $TEST_log && fail "expected analysis to fail"
+  expect_log "'python_version' cannot be used in a config_setting"
+
+  bazel build //test:bad_pv $EXPFLAG \
+      &> $TEST_log && fail "expected analysis to fail"
+  expect_log "'python_version' cannot be used in a config_setting"
+
+  # --force_python and --host_force_python are only unselectable with the
+  # experimental flag enabled.
+
+  bazel build //test:bad_fp $EXPFLAG \
+      &> $TEST_log && fail "expected analysis to fail"
+  expect_log "'force_python' cannot be used in a config_setting"
+
+  bazel build //test:bad_hfp $EXPFLAG \
+      &> $TEST_log && fail "expected analysis to fail"
+  expect_log "'host_force_python' cannot be used in a config_setting"
+}
+
 run_suite "Tests for the Python rules"

tools/python/BUILD

@@ -36,14 +36,14 @@ test_suite(
 #
 # If you do not need to test any other flags in combination with the Python
 # version, then as a convenience you may use the predefined `config_setting`s
-# `@bazel_tools//python:PY2` and `@bazel_tools//python:PY3`.
+# `@bazel_tools//tools/python:PY2` and `@bazel_tools//tools/python:PY3`.
 #
 # Example usage:
 #
 #     config_setting(
 #         name = "py3_on_arm",
 #         values = {"cpu": "arm"},
-#         flag_values = {"@bazel_tools//python:python_version": "PY3"},
+#         flag_values = {"@bazel_tools//tools/python:python_version": "PY3"},
 #     )
 #
 #     my_target(