Update clients of PythonOptions to respect old/new version semantics

Clients that care about the Python version mode should use getters/setters that respect `--experimental_better_python_version_mixing`. This ensures that the correct field is updated and the correct transition semantics are used.

select()s that examine "force_python" will still see the raw value of that flag. For partial compatibility we still update this flag when transitioning the Python version, but targets that are not underneath py_binary/py_test targets may still observe "force_python" values that are inconsistent with the actual Python mode. The future fix will be to use a new feature flag instead of any native flag.

Work toward #6583, #6868. Design doc for new semantics here: https://github.com/bazelbuild/rules_python/blob/master/proposals/2018-10-25-selecting-between-python-2-and-3.md

RELNOTES: None
PiperOrigin-RevId: 226043392

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

@@ -145,15 +145,6 @@ public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOption
     public String getOutputDirectoryName() {
       return null;
     }
-
-    /**
-     * Returns { 'option name': 'alternative default' } entries for options where the
-     * "real default" should be something besides the default specified in the {@link Option}
-     * declaration.
-     */
-    public Map<String, Object> lateBoundOptionDefaults() {
-      return ImmutableMap.of();
-    }
   }
 
   public static final Label convertOptionsLabel(String input) throws OptionsParsingException {

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

@@ -96,8 +96,7 @@ public PyCommon(RuleContext ruleContext) {
 
   public void initCommon(PythonVersion defaultVersion) {
     this.sourcesVersion = getSrcsVersionAttr(ruleContext);
-    this.version = ruleContext.getFragment(PythonConfiguration.class)
-        .getPythonVersion(defaultVersion);
+    this.version = ruleContext.getFragment(PythonConfiguration.class).getPythonVersion();
     this.transitivePythonSources = collectTransitivePythonSources();
     checkSourceIsCompatible(this.version, this.sourcesVersion, ruleContext.getLabel());
   }
@@ -116,10 +115,6 @@ public void initBinary(List<Artifact> srcs) {
     } else {
       executable = ruleContext.createOutputArtifact();
     }
-    if (this.version == PythonVersion.PY2AND3) {
-      // TODO(bazel-team): we need to create two actions
-      ruleContext.ruleError("PY2AND3 is not yet implemented");
-    }
 
     NestedSetBuilder<Artifact> filesToBuildBuilder =
         NestedSetBuilder.<Artifact>stableOrder().addAll(srcs).add(executable);
@@ -194,6 +189,10 @@ public static StructImpl createSourceProvider(
         "No such attribute '%s'");
   }
 
+  /**
+   * Returns the parsed value of the "default_python_version" attribute, or null if it is not
+   * defined for this rule class.
+   */
   public PythonVersion getDefaultPythonVersion() {
     return ruleContext.getRule().isAttrDefined("default_python_version", Type.STRING)
         ? getPythonVersionAttr(ruleContext)

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

@@ -11,9 +11,11 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
 package com.google.devtools.build.lib.rules.python;
 
-import com.google.common.base.Joiner;
+import com.google.common.base.Ascii;
+import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -24,7 +26,6 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.common.options.TriState;
-import java.util.List;
 
 /**
  * The configuration fragment containing information about the various pieces of infrastructure
@@ -33,59 +34,63 @@
 @Immutable
 @SkylarkModule(
     name = "py",
-    doc = "A configuration fragment for SWIG.",
+    doc = "A configuration fragment for Python.",
     category = SkylarkModuleCategory.CONFIGURATION_FRAGMENT)
 public class PythonConfiguration extends BuildConfiguration.Fragment {
-  private final boolean ignorePythonVersionAttribute;
-  private final PythonVersion defaultPythonVersion;
+
+  private final PythonVersion version;
   private final TriState buildPythonZip;
   private final boolean buildTransitiveRunfilesTrees;
 
   PythonConfiguration(
       PythonVersion defaultPythonVersion,
-      boolean ignorePythonVersionAttribute,
       TriState buildPythonZip,
       boolean buildTransitiveRunfilesTrees) {
-    this.ignorePythonVersionAttribute = ignorePythonVersionAttribute;
-    this.defaultPythonVersion = defaultPythonVersion;
+    this.version = defaultPythonVersion;
     this.buildPythonZip = buildPythonZip;
     this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees;
   }
 
   /**
-   * Returns the Python version to use. Command-line flag --force_python overrides
-   * the rule default, given as argument.
+   * Returns the Python version to use.
+   *
+   * <p>Specified using either the {@code --python_version} flag and {@code python_version} rule
+   * attribute (new API), or the {@code --force_python} flag and {@code default_python_version} rule
+   * attribute (old API).
    */
-  public PythonVersion getPythonVersion(PythonVersion attributeVersion) {
-    return ignorePythonVersionAttribute || attributeVersion == null
-        ? defaultPythonVersion
-        : attributeVersion;
+  public PythonVersion getPythonVersion() {
+    return version;
   }
 
   @Override
   public String getOutputDirectoryName() {
-    List<PythonVersion> allowedVersions = PythonVersion.TARGET_VALUES;
+    // TODO(brandjon): Implement alternative semantics for controlling which python version(s) get
+    // suffixed roots.
+    Preconditions.checkState(version.isTargetValue());
+    // The only possible Python target version values are PY2 and PY3. For now, PY2 gets the normal
+    // output directory name, and PY3 gets a "-py3" suffix.
     Verify.verify(
-        allowedVersions.size() == 2, // If allowedVersions.size() == 1, we don't need this method.
-        ">2 possible defaultPythonVersion values makes output directory clashes possible");
-    // Skip this check if --force_python is set. That's because reportInvalidOptions reports
-    // bad --force_python settings with a clearer user error (and Bazel's configuration
-    // initialization logic calls reportInvalidOptions after this method).
-    if (!ignorePythonVersionAttribute && !allowedVersions.contains(defaultPythonVersion)) {
-      throw new IllegalStateException(
-          String.format("defaultPythonVersion=%s not allowed: must be in %s to prevent output "
-              + "directory clashes", defaultPythonVersion, Joiner.on(", ").join(allowedVersions)));
+        PythonVersion.TARGET_VALUES.size() == 2, // If there is only 1, we don't need this method.
+        "Detected a change in PythonVersion.TARGET_VALUES so that there are no longer two Python "
+            + "versions. Please check that PythonConfiguration#getOutputDirectoryName() is still "
+            + "needed and is still able to avoid output directory clashes, then update this "
+            + "canary message.");
+    if (version.equals(PythonVersion.PY2)) {
+      return null;
+    } else {
+      return Ascii.toLowerCase(version.toString());
     }
-    return (defaultPythonVersion == PythonVersion.PY3) ? "py3" : null;
   }
 
   @Override
   public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {
     PythonOptions pythonOptions = buildOptions.get(PythonOptions.class);
-    if (pythonOptions.forcePython != null
-        && pythonOptions.forcePython != PythonVersion.PY2
-        && pythonOptions.forcePython != PythonVersion.PY3) {
-      reporter.handle(Event.error("'--force_python' argument must be 'PY2' or 'PY3'"));
+    if (pythonOptions.pythonVersion != null
+        && !pythonOptions.experimentalBetterPythonVersionMixing) {
+      reporter.handle(
+          Event.error(
+              "`--python_version` is only allowed with "
+                  + "`--experimental_better_python_version_mixing`"));
     }
   }
 

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

@@ -33,11 +33,9 @@ public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() {
   public PythonConfiguration create(BuildOptions buildOptions)
       throws InvalidConfigurationException {
     PythonOptions pythonOptions = buildOptions.get(PythonOptions.class);
-    boolean ignorePythonVersionAttribute = pythonOptions.forcePython != null;
     PythonVersion pythonVersion = pythonOptions.getPythonVersion();
     return new PythonConfiguration(
         pythonVersion,
-        ignorePythonVersionAttribute,
         pythonOptions.buildPythonZip,
         pythonOptions.buildTransitiveRunfilesTrees);
   }

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

@@ -143,14 +143,6 @@ public String getTypeDescription() {
               + " Can be \"PY2\" or \"PY3\".")
   public PythonVersion hostForcePython;
 
-  public PythonVersion getPythonVersion() {
-    return getPythonVersion(PythonVersion.DEFAULT_TARGET_VALUE);
-  }
-
-  public PythonVersion getPythonVersion(PythonVersion defaultVersion) {
-    return (forcePython == null) ? defaultVersion : forcePython;
-  }
-
   /**
    * Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for.
    *
@@ -164,9 +156,7 @@ public PythonVersion getPythonVersion(PythonVersion defaultVersion) {
    *       except {@code --python_version} is ignored.
    * </ul>
    */
-  // TODO(brandjon): Eliminate the NEW prefix, make public, use this to replace the above
-  // getPythonVersion() overloads.
-  PythonVersion NEWgetPythonVersion() {
+  public PythonVersion getPythonVersion() {
     if (experimentalBetterPythonVersionMixing) {
       if (pythonVersion != null) {
         return pythonVersion;
@@ -180,7 +170,10 @@ PythonVersion NEWgetPythonVersion() {
    *
    * <p>Under the new semantics ({@link #experimentalBetterPythonVersionMixing} is true), version
    * transitions are always allowed, so this just returns whether the new version is different from
-   * the existing one.
+   * the existing one. However, as a compatibility measure for {@code select()}s that depend on
+   * {@code "force_python"}, transitioning is still done when {@code forcePython} is not in
+   * agreement with {@link #getPythonVersion}, even if {@code #getPythonVersion}'s value would be
+   * unaffected.
    *
    * <p>Under the old semantics ({@link #experimentalBetterPythonVersionMixing} is false), version
    * transitions are not allowed once the version has already been set ({@link #forcePython} is
@@ -191,10 +184,10 @@ PythonVersion NEWgetPythonVersion() {
    * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
    */
   public boolean canTransitionPythonVersion(PythonVersion version) {
-    Preconditions.checkArgument(PythonVersion.TARGET_VALUES.contains(version));
+    Preconditions.checkArgument(version.isTargetValue());
     if (experimentalBetterPythonVersionMixing) {
-      PythonVersion currentVersion = NEWgetPythonVersion();
-      return !version.equals(currentVersion);
+      PythonVersion currentVersion = getPythonVersion();
+      return !version.equals(currentVersion) || !version.equals(forcePython);
     } else {
       return forcePython == null && !version.equals(PythonVersion.DEFAULT_TARGET_VALUE);
     }
@@ -212,12 +205,21 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
    * false), after this method is called {@code transitionPythonVersion} will not be able to change
    * the version ({@code forcePython} will be non-null).
    *
+   * <p>To help avoid breaking {@code select()} expressions that check the value of {@code
+   * "force_python"}, under the new semantics both {@code pythonVersion} and {@code forcePython} are
+   * updated. Note that it is still not guaranteed that all instances of {@code PythonOptions} that
+   * use the new semantics have {@code forcePython} equal {@code pythonVersion} -- in particular,
+   * this might not be the case for targets that have not gone through a {@link
+   * PythonVersionTransition}.
+   *
    * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
    */
   public void setPythonVersion(PythonVersion version) {
-    Preconditions.checkArgument(PythonVersion.TARGET_VALUES.contains(version));
+    Preconditions.checkArgument(version.isTargetValue());
     if (experimentalBetterPythonVersionMixing) {
       this.pythonVersion = version;
+      // Meaningless to getPythonVersion(), but read by select()s that depend on "force_python".
+      this.forcePython = version;
     } else {
       this.forcePython = version;
     }

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

@@ -102,6 +102,11 @@ private static ImmutableList<String> convertToStrings(List<PythonVersion> values
 
   public static final PythonVersion DEFAULT_SRCS_VALUE = PY2AND3;
 
+  /** Returns whether or not this value is a distinct Python version. */
+  public boolean isTargetValue() {
+    return TARGET_VALUES.contains(this);
+  }
+
   /**
    * Converts the string to a target {@code PythonVersion} value (case-sensitive).
    *

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

@@ -14,49 +14,46 @@
 
 package com.google.devtools.build.lib.rules.python;
 
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 
-/**
- * A configuration transition that sets the Python version by setting {@link
- * PythonOptions#forcePython}.
- */
+/** A configuration transition that sets the Python version. */
 @AutoCodec
 public class PythonVersionTransition implements PatchTransition {
-  private final PythonVersion defaultVersion;
+
+  private final PythonVersion version;
 
   /**
-   * Creates a new transition that sets the given version if not already specified by
-   * {@link PythonOptions#forcePython}.
+   * Creates a new transition that sets the version to the given value, if transitioning is allowed.
+   *
+   * <p>See {@link PythonOptions#canTransitionPythonVersion} for information on when transitioning
+   * is allowed.
+   *
+   * <p>{@code version} must be a target version (either {@code PY2} or {@code PY3}), or else null,
+   * which means use the default value ({@link PythonVersion#DEFAULT_TARGET_VALUE}).
+   *
+   * @throws IllegalArgumentException if {@code version} is non-null and not {@code PY2} or {@code
+   *     PY3}
    */
-  PythonVersionTransition(PythonVersion defaultVersion) {
-    this.defaultVersion = defaultVersion;
+  PythonVersionTransition(PythonVersion version) {
+    if (version == null) {
+      version = PythonVersion.DEFAULT_TARGET_VALUE;
+    }
+    Preconditions.checkArgument(version.isTargetValue());
+    this.version = version;
   }
 
   @Override
-   public BuildOptions patch(BuildOptions options) {
-    PythonOptions pyOptions = options.get(PythonOptions.class);
-    // The current Python version is either explicitly set by --force_python or a
-    // build-wide default.
-    PythonVersion currentVersion = pyOptions.getPythonVersion();
-    // The new Python version is either explicitly set by --force_python or this transition's
-    // default.
-    PythonVersion newVersion = pyOptions.getPythonVersion(defaultVersion);
-    if (currentVersion == newVersion) {
+  public BuildOptions patch(BuildOptions options) {
+    PythonOptions opts = options.get(PythonOptions.class);
+    if (!opts.canTransitionPythonVersion(version)) {
       return options;
     }
-
-    // forcePython must be one of PY2 or PY3 because these are the only values Blaze's output
-    // directories can safely distinguish. In other words, a configuration with forcePython=PY2
-    // would have the same output directory prefix as another with forcePython=PY2AND3, which is a
-    // major correctness failure.
-    //
-    // Even though this transition doesn't enforce the above, it only gets called on
-    // "default_python_version" attribute values, which happen to honor this. Proper enforcement is
-    // done in PythonConfiguration#getOutputDirectoryName.
     BuildOptions newOptions = options.clone();
-    newOptions.get(PythonOptions.class).forcePython = newVersion;
+    PythonOptions newOpts = newOptions.get(PythonOptions.class);
+    newOpts.setPythonVersion(version);
     return newOptions;
   }
 }

src/test/java/com/google/devtools/build/lib/rules/python/BUILD

@@ -21,6 +21,16 @@ test_suite(
     ],
 )
 
+java_library(
+    name = "PythonTestUtils",
+    srcs = ["PythonTestUtils.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib:python-rules",
+        "//third_party:junit4",
+        "//third_party:truth",
+    ],
+)
+
 java_test(
     name = "BazelPythonConfigurationTest",
     srcs = ["BazelPythonConfigurationTest.java"],
@@ -39,6 +49,7 @@ java_test(
     name = "PythonConfigurationTest",
     srcs = ["PythonConfigurationTest.java"],
     deps = [
+        ":PythonTestUtils",
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:python-rules",
         "//src/main/java/com/google/devtools/common/options",
@@ -53,6 +64,7 @@ java_library(
     name = "PyBaseTestBase",
     srcs = ["PyBaseConfiguredTargetTestBase.java"],
     deps = [
+        ":PythonTestUtils",
         "//src/main/java/com/google/devtools/build/lib:bazel-rules",
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:python-rules",
@@ -67,9 +79,11 @@ java_library(
     srcs = ["PyExecutableConfiguredTargetTestBase.java"],
     deps = [
         ":PyBaseTestBase",
+        ":PythonTestUtils",
         "//src/main/java/com/google/devtools/build/lib:bazel-rules",
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:python-rules",
+        "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
     ],
@@ -101,7 +115,9 @@ java_test(
     srcs = ["PyLibraryConfiguredTargetTest.java"],
     deps = [
         ":PyBaseTestBase",
+        ":PythonTestUtils",
         "//src/main/java/com/google/devtools/build/lib:build-base",
+        "//src/main/java/com/google/devtools/build/lib:python-rules",
         "//src/test/java/com/google/devtools/build/lib:actions_testutil",
         "//src/test/java/com/google/devtools/build/lib:analysis_testutil",
         "//third_party:junit4",