From 09c6b7a7a659941e6920902f726660af14f9b176 Mon Sep 17 00:00:00 2001
From: brandjon <brandjon@google.com>
Date: Wed, 13 Mar 2019 12:24:50 -0700
Subject: [PATCH] Do not add dummy py version transitions

These transitions were intended to bring --force_python in sync with the actual Python version to make select() less broken. This ended up causing more problems than it fixed (#7655). It's not really needed anyway since the select() issue becomes moot once --incompatible_remove_old_python_version_api is enabled.

RELNOTES: None
PiperOrigin-RevId: 238281325
---
 .../build/lib/rules/python/PythonOptions.java | 21 ++++++++++++-------
 .../rules/python/PythonConfigurationTest.java |  9 ++++----
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
index 91231f5adcd51b..ef53b5181636a6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
@@ -305,10 +305,7 @@ public PythonVersion getPythonVersion() {
    *
    * <p>Under the new semantics ({@link #incompatibleAllowPythonVersionTransitions} is true),
    * version transitions are always allowed, so this essentially returns whether the new version is
-   * different from the existing one. However, to improve compatibility for unmigrated {@code
-   * select()}s that depend on {@code "force_python"}, if the old API is still enabled then
-   * transitioning is still done whenever {@link #forcePython} is not in agreement with the
-   * requested version, even if {@link #getPythonVersion}'s value would be unaffected.
+   * different from the existing one.
    *
    * <p>Under the old semantics ({@link #incompatibleAllowPythonVersionTransitions} is false),
    * version transitions are not allowed once the version has already been set ({@link #forcePython}
@@ -316,15 +313,23 @@ public PythonVersion getPythonVersion() {
    * transition the version to the hard-coded default value. Under these constraints, there is only
    * one transition possible, from null to the non-default value, and it is never a no-op.
    *
+   * <p>Previously this method also allowed transitioning under the new semantics in cases where the
+   * transition would have no impact on {@link #getPythonVersion} but would bring {@link
+   * #forcePython} into agreement with the actual version. The benefit of doing this was supposed to
+   * be that {@code select()}ing on {@code "force_python"} would give the correct result more often,
+   * even though it's still incorrect in general. However, this ended up causing more harm than
+   * good, because this type of transition does not change the output root and therefore caused
+   * action conflicts for unshareable actions (mainly C++ actions); see #7655. The resolution is
+   * that users should just migrate their {@code select()}s to the {@code
+   * //tools/python:python_version} target in the tools repository, as is required when {@code
+   * --incompatible_remove_old_python_version_api} is enabled.
+   *
    * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
    */
   public boolean canTransitionPythonVersion(PythonVersion version) {
     Preconditions.checkArgument(version.isTargetValue());
     if (incompatibleAllowPythonVersionTransitions) {
-      boolean currentVersionNeedsUpdating = !version.equals(getPythonVersion());
-      boolean forcePythonNeedsUpdating =
-          !incompatibleRemoveOldPythonVersionApi && !version.equals(forcePython);
-      return currentVersionNeedsUpdating || forcePythonNeedsUpdating;
+      return !version.equals(getPythonVersion());
     } else {
       boolean currentlyUnset = forcePython == null && pythonVersion == null;
       boolean transitioningToNonDefault = !version.equals(getDefaultPythonVersion());
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
index 3aec314504bdcd..ecb94703bb9e03 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
@@ -177,17 +177,16 @@ public void canTransitionPythonVersion_NewSemantics_NoBecauseSameAsCurrent() thr
   }
 
   @Test
-  public void canTransitionPythonVersion_NewApi_YesBecauseForcePythonDisagrees() throws Exception {
+  public void canTransitionPythonVersion_NewApi_NoEvenWhenForcePythonDisagrees() throws Exception {
     PythonOptions opts =
         parsePythonOptions(
             "--incompatible_allow_python_version_transitions=true",
             "--incompatible_remove_old_python_version_api=false",
-            // Test that even though getPythonVersion() would not be affected by a transition (it is
-            // PY3 before and after), the transition is still considered necessary because
-            // --force_python's value needs to be brought in sync.
+            // Test that even though --force_python's value isn't in sync, we don't transition
+            // because getPythonVersion() would be unaffected by the transition.
             "--force_python=PY2",
             "--python_version=PY3");
-    assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isTrue();
+    assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isFalse();
   }
 
   @Test