Ensure Python transitions do not create action conflicts

This is an extension of 09c6b7a, which turned out to be insufficient for preventing action conflicts (between unshareable actions, e.g. C++ compilation actions). That CL avoided creating a transition where it was not needed; however in cases where it is needed, we still get conflicts due to configurations that differ trivially from the top-level configuration.

This CL avoids the problem by preprocessing the top-level configuration, to avoid creating configured targets with spurious config differences. Specifically, even though --force_python and --python_version admit a "null" state meaning "not set by the user, please use the default", we immediately replace this with the actual default at the time the BuildOptions is created, so no configured target sees a null value for those options. This mechanism may also be useful for other fragments in the future that need smart default values.

Fixes #7808 to unblock #7307.

RELNOTES: None
PiperOrigin-RevId: 240207632

src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java

@@ -471,11 +471,14 @@ public Builder merge(BuildOptions options) {
     }
 
     /**
-     * Adds a new FragmentOptions instance to the builder. Overrides previous instances of the exact
-     * same subclass of FragmentOptions.
+     * Adds a new {@link FragmentOptions} instance to the builder.
+     *
+     * <p>Overrides previous instances of the exact same subclass of {@code FragmentOptions}.
+     *
+     * <p>The options get preprocessed with {@link FragmentOptions#getNormalized}.
      */
     public <T extends FragmentOptions> Builder addFragmentOptions(T options) {
-      fragmentOptions.put(options.getClass(), options);
+      fragmentOptions.put(options.getClass(), options.getNormalized());
       return this;
     }
 

src/main/java/com/google/devtools/build/lib/analysis/config/FragmentOptions.java

@@ -48,20 +48,44 @@ public FragmentOptions clone() {
   }
 
   /**
-   * Creates a new FragmentOptions instance with all flags set to default.
+   * Creates a new instance of this {@code FragmentOptions} with all flags set to their default
+   * values.
    */
   public FragmentOptions getDefault() {
     return Options.getDefaults(getClass());
   }
 
   /**
-   * Creates a new FragmentOptions instance with flags adjusted to host platform.
+   * Creates a new instance of this {@code FragmentOptions} with all flags adjusted as needed to
+   * represent the host platform.
    */
   @SuppressWarnings("unused")
   public FragmentOptions getHost() {
     return getDefault();
   }
 
+  /**
+   * Creates a new instance of this {@code FragmentOptions} with all flags adjusted to be suitable
+   * for forming configurations.
+   *
+   * <p>Motivation: Sometimes a fragment's physical option values, as set by the options parser, do
+   * not correspond to their logical interpretation. For example, an option may need custom code to
+   * determine its logical default value at runtime, but it's limited to a single hard-coded
+   * physical default value in the {@link Option#defaultValue} annotation field. If two instances of
+   * the fragment have the same logical value but different physical values, a redundant
+   * configuration can be created, which results in an action conflict (particularly for unshareable
+   * actions; see #7808).
+   *
+   * <p>To solve this, we can distinguish between "normalized" and "non-normalized" instances of a
+   * fragment type, and preserve the invariant that configured targets only ever see normalized
+   * instances. This requires that 1) the top-level configuration is normalized, and 2) all
+   * transitions preserve normalization. Step 1) is ensured by {@link BuildOptions} calling this
+   * method. Step 2) is the responsibility of each transition implementation.
+   */
+  public FragmentOptions getNormalized() {
+    return clone();
+  }
+
   /** Tracks limitations on referring to an option in a {@code config_setting}. */
   // TODO(bazel-team): There will likely also be a need to customize whether or not an option is
   // visible to users for setting on the command line (or perhaps even in a test of a Starlark

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

@@ -313,17 +313,6 @@ 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) {
@@ -357,9 +346,13 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
   public void setPythonVersion(PythonVersion version) {
     Preconditions.checkArgument(version.isTargetValue());
     this.pythonVersion = version;
-    // Update forcePython, but if the flag to remove the old API is enabled, no one will be able
-    // to tell anyway.
-    this.forcePython = version;
+    // If the old version API is enabled, update forcePython for consistency. If the old API is
+    // disabled, don't update it because 1) no one can read it anyway, and 2) updating it during
+    // normalization would cause analysis-time validation of the flag to spuriously fail (it'd think
+    // the user set the flag).
+    if (!incompatibleRemoveOldPythonVersionApi) {
+      this.forcePython = version;
+    }
   }
 
   @Override
@@ -378,4 +371,16 @@ public FragmentOptions getHost() {
     hostPythonOptions.incompatibleUsePythonToolchains = incompatibleUsePythonToolchains;
     return hostPythonOptions;
   }
+
+  @Override
+  public FragmentOptions getNormalized() {
+    // Under the new version semantics, we want to ensure that options with "null" physical default
+    // values are normalized, to avoid #7808. We don't want to normalize with the old version
+    // semantics because that breaks backwards compatibility (--force_python would always be on).
+    PythonOptions newOptions = (PythonOptions) clone();
+    if (incompatibleAllowPythonVersionTransitions) {
+      newOptions.setPythonVersion(newOptions.getPythonVersion());
+    }
+    return newOptions;
+  }
 }

src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java

@@ -190,7 +190,7 @@ public void canTransitionPythonVersion_NewApi_NoEvenWhenForcePythonDisagrees() t
   }
 
   @Test
-  public void setPythonVersion() throws Exception {
+  public void setPythonVersion_OldApiEnabled() throws Exception {
     PythonOptions opts =
         parsePythonOptions(
             "--incompatible_remove_old_python_version_api=false",
@@ -201,6 +201,16 @@ public void setPythonVersion() throws Exception {
     assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3);
   }
 
+  @Test
+  public void setPythonVersion_OldApiDisabled() throws Exception {
+    PythonOptions opts =
+        parsePythonOptions(
+            "--incompatible_remove_old_python_version_api=true", "--python_version=PY2");
+    opts.setPythonVersion(PythonVersion.PY3);
+    assertThat(opts.forcePython).isNull();
+    assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3);
+  }
+
   @Test
   public void getHost_CopiesMostValues() throws Exception {
     PythonOptions opts =
@@ -260,4 +270,20 @@ public void getHost_Py3IsDefaultFlagChangesHost() throws Exception {
     PythonOptions hostOpts = (PythonOptions) opts.getHost();
     assertThat(hostOpts.getPythonVersion()).isEqualTo(PythonVersion.PY3);
   }
+
+  @Test
+  public void getNormalized_OldSemantics() throws Exception {
+    PythonOptions opts =
+        parsePythonOptions("--incompatible_allow_python_version_transitions=false");
+    PythonOptions normalizedOpts = (PythonOptions) opts.getNormalized();
+    assertThat(normalizedOpts.pythonVersion).isNull();
+  }
+
+  @Test
+  public void getNormalized_NewSemantics() throws Exception {
+    ensureDefaultIsPY2();
+    PythonOptions opts = parsePythonOptions("--incompatible_allow_python_version_transitions=true");
+    PythonOptions normalizedOpts = (PythonOptions) opts.getNormalized();
+    assertThat(normalizedOpts.pythonVersion).isEqualTo(PythonVersion.PY2);
+  }
 }

src/test/shell/integration/python_stub_test.sh

@@ -136,4 +136,72 @@ EOF
       &> $TEST_log || fail "bazel build failed"
 }
 
+# Regression test for #7808. We want to ensure that changing the Python version
+# to a value different from the top-level configuration, and then changing it
+# back again, is able to reuse the top-level configuration.
+function test_no_action_conflicts_from_version_transition() {
+  mkdir -p test
+
+  # To repro, we need to build a C++ target in two different ways in the same
+  # build:
+  #
+  #   1) At the top-level, and without any explicit flags passed to control the
+  #      Python version, because the behavior under test involves the internal
+  #      null default value of said flags.
+  #
+  #   2) As a dependency of a target that transitions the Python version to the
+  #      same value as in the top-level configuration.
+  #
+  # We need to use two different Python targets, to transition the version
+  # *away* from the top-level default and then *back* again. Furthermore,
+  # because (as of the writing of this test) the default Python version is in
+  # the process of being migrated from PY2 to PY3, we'll future-proof this test
+  # by using two separate paths that have the versions inverted.
+  #
+  # We use C++ for the repro because it has unshareable actions, so we'll know
+  # if the top-level config isn't being reused.
+
+  cat > test/BUILD << EOF
+cc_binary(
+    name = "cc",
+    srcs = ["cc.cc"],
+)
+
+py_binary(
+    name = "path_A_inner",
+    srcs = ["path_A_inner.py"],
+    data = [":cc"],
+    python_version = "PY2",
+)
+
+py_binary(
+    name = "path_A_outer",
+    srcs = [":path_A_outer.py"],
+    data = [":path_A_inner"],
+    python_version = "PY3",
+)
+
+py_binary(
+    name = "path_B_inner",
+    srcs = [":path_B_inner.py"],
+    data = [":cc"],
+    python_version = "PY3",
+)
+
+py_binary(
+    name = "path_B_outer",
+    srcs = [":path_B_outer.py"],
+    data = [":path_B_inner"],
+    python_version = "PY2",
+)
+EOF
+
+  # Build cc at the top level, along with the outer halves of both paths to cc.
+  # Make sure to use the new version transition semantics.
+  bazel build --nobuild //test:cc //test:path_A_outer //test:path_B_outer \
+      --incompatible_remove_old_python_version_api=true \
+      --incompatible_allow_python_version_transitions=true \
+      &> $TEST_log || fail "bazel run failed"
+}
+
 run_suite "Tests for the Python rules without Python execution"