Rework flags for Python version migration

This CL implements the bulk of the changes to the migration section of the design doc, described by PR bazelbuild/rules_python#153. A subsequent CL will do the flag rename from --experimental_better_python_version_mixing to --experimental_allow_python_version_transitions.

The main changes in this CL are:

  1. Instead of one experimental/incompatible flag, we have two: one for syntax and one for semantics.

  2. The new API (--python_version flag and python_version attribute) is no longer guarded by an experimental flag, but rather is unconditionally available.

  3. The old API (--force_python flag and default_python_version attribute) is now disabled by an experimental flag. (This was always the plan but this CL implements it.)

The motivation for (1) is so that it's easier for users to migrate and easier for us to roll these changes out. It also simplifies some logic in PythonOptions.java and python_version.bzl, because the fallback from --python_version on to --force_python no longer cares about whether we're using the new or old semantics.

The motivation for (2) is that the current logic for gating an attribute on an experimental/incompatible flag breaks native.existing_rules(). See #7071. In this case the experimental flag would have been very short-lived anyway, since we want these features to be migration ready by February. We still have to face #7071 before we can enable the incompatible change, but at least this CL unblocks further work without hacking up existing_rules() with ad hoc blacklists.

Finally, I took this CL as an opportunity to downsize some of our shell integration tests by turning them into Java unit tests. This required updating the mocks a bit.

Work towards #6583.

RELNOTES: None
PiperOrigin-RevId: 229261233

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

@@ -176,8 +176,7 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
                       "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
                           + " to the configuration"))
           /* <!-- #BLAZE_RULE($base_py_binary).ATTRIBUTE(python_version) -->
-          An experimental replacement for <code>default_python_version</code>. Only available when
-          <code>--experimental_better_python_version_mixing</code> is enabled. If both this and
+          A replacement for <code>default_python_version</code>. If both this and
           <code>default_python_version</code> are supplied, the latter will be ignored.
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .add(

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -897,14 +897,6 @@ private static SkylarkDict<String, Object> targetDict(
         continue;
       }
 
-      // TODO(brandjon): Remove this hack before flipping
-      // --experimental_better_python_version_mixing. Issue #7071.
-      if (attr.getName().equals("python_version")) {
-        // python_version is guarded by experimental flags, which breaks the use case of copying
-        // targets around using native.existing_rules. See #7071 and (Google-internal) b/122596733.
-        continue;
-      }
-
       try {
         Object val = skylarkifyValue(cont.getAttr(attr.getName()), target.getPackage());
         if (val == null) {

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

@@ -179,22 +179,19 @@ private static PythonVersion getSrcsVersionAttr(RuleContext ruleContext) {
   }
 
   /**
-   * If the {@code python_version} attribute is defined for {@code ruleContext}, this method reports
-   * an attribute error if the attribute is set explicitly without the new API being enabled (via
-   * {@code --experimental_better_python_version_mixing}).
+   * Reports an attribute error if the {@code default_python_version} attribute is set but
+   * disallowed by the configuration.
    */
   private static void validatePythonVersionAttr(RuleContext ruleContext) {
     AttributeMap attrs = ruleContext.attributes();
-    if (!attrs.has(PYTHON_VERSION_ATTRIBUTE, Type.STRING)) {
-      return;
-    }
-    boolean newApiEnabled =
-        ruleContext.getFragment(PythonConfiguration.class).newPyVersionApiEnabled();
-    if (attrs.isAttributeValueExplicitlySpecified(PYTHON_VERSION_ATTRIBUTE) && !newApiEnabled) {
+    PythonConfiguration config = ruleContext.getFragment(PythonConfiguration.class);
+    if (attrs.has(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING)
+        && attrs.isAttributeValueExplicitlySpecified(DEFAULT_PYTHON_VERSION_ATTRIBUTE)
+        && !config.oldPyVersionApiAllowed()) {
       ruleContext.attributeError(
-          PYTHON_VERSION_ATTRIBUTE,
-          "using the 'python_version' attribute requires the "
-              + "'--experimental_better_python_version_mixing' flag");
+          DEFAULT_PYTHON_VERSION_ATTRIBUTE,
+          "the 'default_python_version' attribute is disabled by the "
+              + "'--experimental_remove_old_python_version_api' flag");
     }
   }
 

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

@@ -41,18 +41,22 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
   private final PythonVersion version;
   private final TriState buildPythonZip;
   private final boolean buildTransitiveRunfilesTrees;
-  // TODO(brandjon): Remove once migration to the new API is complete (#6583).
-  private final boolean newPyVersionApiEnabled;
+
+  // TODO(brandjon): Remove these once migration to the new API is complete (#6583).
+  private final boolean oldPyVersionApiAllowed;
+  private final boolean useNewPyVersionSemantics;
 
   PythonConfiguration(
       PythonVersion defaultPythonVersion,
       TriState buildPythonZip,
       boolean buildTransitiveRunfilesTrees,
-      boolean newPyVersionApiEnabled) {
+      boolean oldPyVersionApiAllowed,
+      boolean useNewPyVersionSemantics) {
     this.version = defaultPythonVersion;
     this.buildPythonZip = buildPythonZip;
     this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees;
-    this.newPyVersionApiEnabled = newPyVersionApiEnabled;
+    this.oldPyVersionApiAllowed = oldPyVersionApiAllowed;
+    this.useNewPyVersionSemantics = useNewPyVersionSemantics;
   }
 
   /**
@@ -88,13 +92,11 @@ public String getOutputDirectoryName() {
 
   @Override
   public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {
-    PythonOptions pythonOptions = buildOptions.get(PythonOptions.class);
-    if (pythonOptions.pythonVersion != null
-        && !pythonOptions.experimentalBetterPythonVersionMixing) {
+    PythonOptions opts = buildOptions.get(PythonOptions.class);
+    if (opts.forcePython != null && opts.experimentalRemoveOldPythonVersionApi) {
       reporter.handle(
           Event.error(
-              "`--python_version` is only allowed with "
-                  + "`--experimental_better_python_version_mixing`"));
+              "`--force_python` is disabled by `--experimental_remove_old_python_version_api`"));
     }
   }
 
@@ -119,10 +121,15 @@ public boolean buildTransitiveRunfilesTrees() {
   }
 
   /**
-   * Returns whether use of {@code --python_version} flag and {@code python_version} attribute is
-   * allowed.
+   * Returns whether use of {@code --force_python} flag and {@code default_python_version} attribute
+   * is allowed.
    */
-  public boolean newPyVersionApiEnabled() {
-    return newPyVersionApiEnabled;
+  public boolean oldPyVersionApiAllowed() {
+    return oldPyVersionApiAllowed;
+  }
+
+  /** Returns true if the new semantics should be used for transitions on the Python version. */
+  public boolean useNewPyVersionSemantics() {
+    return useNewPyVersionSemantics;
   }
 }

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

@@ -38,7 +38,8 @@ public PythonConfiguration create(BuildOptions buildOptions)
         pythonVersion,
         pythonOptions.buildPythonZip,
         pythonOptions.buildTransitiveRunfilesTrees,
-        /*newPyVersionApiEnabled=*/ pythonOptions.experimentalBetterPythonVersionMixing);
+        /*oldPyVersionApiAllowed=*/ !pythonOptions.experimentalRemoveOldPythonVersionApi,
+        /*useNewPyVersionSemantics=*/ pythonOptions.experimentalBetterPythonVersionMixing);
   }
 
   @Override

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

@@ -32,11 +32,9 @@
 /**
  * Python-related command-line options.
  *
- * <p>Due to the migration associated with {@code --experimental_better_python_version_mixing} (see
- * #6583), the Python major version mode ({@code PY2} vs {@code PY3}) is a function of three
- * separate flags: this experimental feature-guarding flag, the old version flag {@code
- * --force_python}, and the new version flag {@code --python_version}. See {@link #getPythonVersion}
- * for more details.
+ * <p>Due to the migration from the old Python version API to the new (see #6583), the Python major
+ * version mode ({@code PY2} vs {@code PY3}) is a function of multiple flags. See {@link
+ * #getPythonVersion} for more details.
  */
 public class PythonOptions extends FragmentOptions {
 
@@ -71,29 +69,39 @@ public String getTypeDescription() {
       help = "Build python executable zip; on on Windows, off on other platforms")
   public TriState buildPythonZip;
 
+  // TODO(brandjon): For both experimental options below, add documentation and add a link to it
+  // from the help text, then change the documentationCategory to SKYLARK_SEMANTICS.
+
   @Option(
-      name = "experimental_better_python_version_mixing",
-      // TODO(brandjon): Do not flip until we have an answer for how to guard the "python_version"
-      // attribute without hacking up native.existing_rules(). See b/122596733.
+      name = "experimental_remove_old_python_version_api",
+      // TODO(brandjon): Do not flip until we have an answer for how to disallow the
+      // "default_python_version" attribute without hacking up native.existing_rules(). See
+      // #7071 and b/122596733.
       defaultValue = "false",
-      // TODO(brandjon): Change to OptionDocumentationCategory.SKYLARK_SEMANTICS when this is
-      // sufficiently implemented/documented. Also fill in the ref in the help text below.
       documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
       metadataTags = {OptionMetadataTag.EXPERIMENTAL},
       help =
-          "If true, enables use of the `--python_version` flag and the `python_version` "
-              + "attribute for `py_binary` and `py_test`, and uses the new PY2/PY3 version "
-              + "semantics. See <TODO: ADD LINK> for more details.")
+          "If true, disables use of the `--force_python` flag and the `default_python_version` "
+              + "attribute for `py_binary` and `py_test`.")
+  public boolean experimentalRemoveOldPythonVersionApi;
+
+  @Option(
+      name = "experimental_better_python_version_mixing",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+      help = "If true, Python rules use the new PY2/PY3 version semantics.")
   public boolean experimentalBetterPythonVersionMixing;
 
   /**
-   * 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.
+   * This field should be either null (unset), {@code PY2}, or {@code PY3}. Other {@code
+   * PythonVersion} values do not represent distinct Python versions and are not allowed.
    *
-   * <p>Null represents that the value is not set by the user. This is only relevant for deciding
-   * whether or not to ignore the old flag, {@code --force_python}. Rule logic can't tell whether or
-   * not this field is null.
+   * <p>Native rule logic should call {@link #getPythonVersion} / {@link #setPythonVersion} instead
+   * of accessing this option directly. BUILD/.bzl code should {@code select()} on {@code <tools
+   * repo>//tools/python:python_version} rather than on this option directly.
    */
   @Option(
       name = "python_version",
@@ -116,15 +124,15 @@ public String getTypeDescription() {
       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.
+   * This field should be either null (unset), {@code PY2}, or {@code PY3}. Other {@code
+   * PythonVersion} values do not represent distinct Python versions and are not allowed.
+   *
+   * <p>This flag is not accessible to the user when {@link #experimentalRemoveOldPythonVersionApi}
+   * is true.
    *
-   * <p>Null represents that the value is not set by the user. When {@code
-   * --experimental_better_python_version_mixing} is false, null means that the default value {@link
-   * PythonVersion#DEFAULT_TARGET_VALUE} should be used, but that it is possible for {@link
-   * PythonVersionTransition} to override this default. When the experimental flag is true, {@code
-   * PythonVersionTransition} can always override the version anyway, and null has the same effect
-   * as setting it to the default.
+   * <p>Native rule logic should call {@link #getPythonVersion} / {@link #setPythonVersion} instead
+   * of accessing this option directly. BUILD/.bzl code should {@code select()} on {@code <tools
+   * repo>//tools/python:python_version} rather than on this option directly.
    */
   @Option(
       name = "force_python",
@@ -139,10 +147,13 @@ public String getTypeDescription() {
       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.
+   * This field should be either null (unset), {@code PY2}, or {@code PY3}. Other {@code
+   * PythonVersion} values do not represent distinct Python versions and are not allowed.
    *
    * <p>Null is treated the same as the default ({@link PythonVersion#DEFAULT_TARGET_VALUE}).
+   *
+   * <p>This option is only read by {@link #getHost}. It should not be read by other native code or
+   * by {@code select()}s in user code.
    */
   @Option(
       name = "host_force_python",
@@ -166,7 +177,7 @@ public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {
     restrictions.put(
         PYTHON_VERSION_DEFINITION,
         new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null));
-    if (experimentalBetterPythonVersionMixing) {
+    if (experimentalRemoveOldPythonVersionApi) {
       restrictions.put(
           FORCE_PYTHON_DEFINITION,
           new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null));
@@ -180,50 +191,48 @@ public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {
   /**
    * Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for.
    *
-   * <p>The version is taken from the following in order:
-   *
-   * <ul>
-   *   <li>If {@code --experimental_better_python_version_mixing} is true, then it is the value of
-   *       {@code --python_version} if not null, otherwise {@code --force_python} if not null,
-   *       otherwise {@link PythonVersion#DEFAULT_TARGET_VALUE}.
-   *   <li>If {@code --experimental_better_python_version_mixing} is false, then it is the same
-   *       except {@code --python_version} is ignored.
-   * </ul>
+   * <p>The version is taken as the value of {@code --python_version} if not null, otherwise {@code
+   * --force_python} if not null, otherwise {@link PythonVersion#DEFAULT_TARGET_VALUE}.
    */
   public PythonVersion getPythonVersion() {
-    if (experimentalBetterPythonVersionMixing) {
-      if (pythonVersion != null) {
-        return pythonVersion;
-      }
+    if (pythonVersion != null) {
+      return pythonVersion;
+    } else if (forcePython != null) {
+      return forcePython;
+    } else {
+      return PythonVersion.DEFAULT_TARGET_VALUE;
     }
-    return (forcePython != null) ? forcePython : PythonVersion.DEFAULT_TARGET_VALUE;
   }
 
   /**
    * Returns whether a Python version transition to {@code version} is allowed and not a no-op.
    *
    * <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. 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.
+   * 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.
    *
    * <p>Under the old semantics ({@link #experimentalBetterPythonVersionMixing} is false), version
-   * transitions are not allowed once the version has already been set ({@link #forcePython} is
-   * non-null). Due to a historical bug, it is also not allowed to transition {@code forcePython} to
-   * the hard-coded default value. Under these constraints, any transition that is allowed is also
-   * not a no-op.
+   * transitions are not allowed once the version has already been set ({@link #forcePython} or
+   * {@link #pythonVersion} is non-null). Due to a historical bug, it is also not allowed to
+   * 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.
    *
    * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
    */
   public boolean canTransitionPythonVersion(PythonVersion version) {
     Preconditions.checkArgument(version.isTargetValue());
     if (experimentalBetterPythonVersionMixing) {
-      PythonVersion currentVersion = getPythonVersion();
-      return !version.equals(currentVersion) || !version.equals(forcePython);
+      boolean currentVersionNeedsUpdating = !version.equals(getPythonVersion());
+      boolean forcePythonNeedsUpdating =
+          !experimentalRemoveOldPythonVersionApi && !version.equals(forcePython);
+      return currentVersionNeedsUpdating || forcePythonNeedsUpdating;
     } else {
-      return forcePython == null && !version.equals(PythonVersion.DEFAULT_TARGET_VALUE);
+      boolean currentlyUnset = forcePython == null && pythonVersion == null;
+      boolean transitioningToNonDefault = !version.equals(PythonVersion.DEFAULT_TARGET_VALUE);
+      return currentlyUnset && transitioningToNonDefault;
     }
   }
 
@@ -236,32 +245,26 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
    * #canTransitionPythonVersion} would return true.
    *
    * <p>If the old semantics are in effect ({@link #experimentalBetterPythonVersionMixing} is
-   * false), after this method is called {@code transitionPythonVersion} will not be able to change
-   * the version ({@code forcePython} will be non-null).
+   * false), after this method is called {@link #canTransitionPythonVersion} will return false.
    *
-   * <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}.
+   * <p>To help avoid breaking old-API {@code select()} expressions that check the value of {@code
+   * "force_python"}, both the old and new flags are updated even though {@code --python_version}
+   * takes precedence over {@code --force_python}.
    *
    * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
    */
   public void setPythonVersion(PythonVersion 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;
-    }
+    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;
   }
 
   @Override
   public FragmentOptions getHost() {
     PythonOptions hostPythonOptions = (PythonOptions) getDefault();
+    hostPythonOptions.experimentalRemoveOldPythonVersionApi = experimentalRemoveOldPythonVersionApi;
     hostPythonOptions.experimentalBetterPythonVersionMixing = experimentalBetterPythonVersionMixing;
     PythonVersion hostVersion =
         (hostForcePython != null) ? hostForcePython : PythonVersion.DEFAULT_TARGET_VALUE;

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

@@ -896,6 +896,7 @@ java_library(
     resources = [
         "packages/util/MOCK_ANDROID_CROSSTOOL",
         "packages/util/MOCK_OSX_CROSSTOOL",
+        "//tools/python:python_version_srcs",
     ],
     deps = [
         ":default_test_build_rules",