Add PyInfo migration flag to --all_incompatible_changes

This renames --experimental_disallow_legacy_py_provider to --incompatible_disallow_legacy_py_provider and makes it available under --all_incompatible_changes. Migrate legacy "py" struct providers to PyInfo instead. See #7298 for more information. Work toward #7298 and #7010. RELNOTES[INC]: Python rules will soon reject the legacy "py" struct provider (preview by enabling --incompatible_disallow_legacy_py_provider). Upgrade rules to use PyInfo instead. See [#7298](bazelbuild/bazel#7298). PiperOrigin-RevId: 231885505
luca-digrazia · Sep 4, 2022 · 2fcb6fc · 2fcb6fc
1 parent 4316579
commit 2fcb6fc
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 197 deletions.
diff --git a/...l/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/...l/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java
@@ -36,16 +36,11 @@ public PythonConfiguration create(BuildOptions buildOptions)
     PythonVersion pythonVersion = pythonOptions.getPythonVersion();
     return new PythonConfiguration(
         pythonVersion,
-        pythonOptions.getDefaultPythonVersion(),
         pythonOptions.buildPythonZip,
         pythonOptions.buildTransitiveRunfilesTrees,
-        /*oldPyVersionApiAllowed=*/ !pythonOptions.incompatibleRemoveOldPythonVersionApi,
-        /*useNewPyVersionSemantics=*/ pythonOptions.incompatibleAllowPythonVersionTransitions,
-        /*py2OutputsAreSuffixed=*/ pythonOptions.incompatiblePy2OutputsAreSuffixed,
-        /*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider,
-        /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains,
-        /*loadPythonRulesFromBzl=*/ pythonOptions.loadPythonRulesFromBzl,
-        /*defaultToExplicitInitPy=*/ pythonOptions.incompatibleDefaultToExplicitInitPy);
+        /*oldPyVersionApiAllowed=*/ !pythonOptions.experimentalRemoveOldPythonVersionApi,
+        /*useNewPyVersionSemantics=*/ pythonOptions.experimentalAllowPythonVersionTransitions,
+        /*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider);
   }
 
   @Override

diff --git a/...elbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/...elbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
@@ -69,28 +69,30 @@ 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_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",
-      documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
       metadataTags = {OptionMetadataTag.EXPERIMENTAL},
       help =
           "If true, disables use of the `--force_python` flag and the `default_python_version` "
-              + "attribute for `py_binary` and `py_test`. Use the `--python_version` flag and "
-              + "`python_version` attribute instead, which have exactly the same meaning. This "
-              + "flag also disables `select()`-ing over `--host_force_python`.")
+              + "attribute for `py_binary` and `py_test`.")
   public boolean experimentalRemoveOldPythonVersionApi;
 
   @Option(
       name = "experimental_allow_python_version_transitions",
       defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
       metadataTags = {OptionMetadataTag.EXPERIMENTAL},
-      help =
-          "If true, Python rules use the new PY2/PY3 version semantics. For more information, see "
-              + "the documentation for `py_binary`'s `python_version` attribute.")
+      help = "If true, Python rules use the new PY2/PY3 version semantics.")
   public boolean experimentalAllowPythonVersionTransitions;
 
   /**
@@ -105,16 +107,17 @@ public String getTypeDescription() {
       name = "python_version",
       defaultValue = "null",
       converter = TargetPythonVersionConverter.class,
-      documentationCategory = OptionDocumentationCategory.GENERIC_INPUTS,
+      // TODO(brandjon): Change to OptionDocumentationCategory.GENERIC_INPUTS when this is
+      // sufficiently implemented/documented.
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
       effectTags = {
         OptionEffectTag.LOADING_AND_ANALYSIS,
         OptionEffectTag.AFFECTS_OUTPUTS // because of "-py3" output root
       },
       help =
-          "The Python major version mode, either `PY2` or `PY3`. Note that under the new version "
-              + "semantics (`--experimental_allow_python_version_transitions`) this is overridden "
-              + "by `py_binary` and `py_test` targets (even if they don't explicitly specify a "
-              + "version) so there is usually not much reason to supply this flag.")
+          "The Python major version mode, either `PY2` or `PY3`. Note that this is overridden by "
+              + "`py_binary` and `py_test` targets (whether or not they specify their version "
+              + "explicitly), so there is usually not much reason to supply this flag.")
   public PythonVersion pythonVersion;
 
   private static final OptionDefinition PYTHON_VERSION_DEFINITION =
@@ -137,9 +140,7 @@ public String getTypeDescription() {
       converter = TargetPythonVersionConverter.class,
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS},
-      help =
-          "Deprecated alias for `--python_version`. Disabled by "
-              + "`--experimental_remove_old_python_version_api`.")
+      help = "Overrides default_python_version attribute. Can be \"PY2\" or \"PY3\".")
   public PythonVersion forcePython;
 
   private static final OptionDefinition FORCE_PYTHON_DEFINITION =
@@ -185,25 +186,19 @@ public String getTypeDescription() {
 
   @Override
   public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {
-    // TODO(brandjon): Instead of referencing the python_version target, whose path depends on the
-    // tools repo name, reference a standalone documentation page instead.
+    // 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,
-            "Use @bazel_tools//python/tools:python_version instead."));
+        new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null));
     if (experimentalRemoveOldPythonVersionApi) {
       restrictions.put(
           FORCE_PYTHON_DEFINITION,
-          new SelectRestriction(
-              /*visibleWithinToolsPackage=*/ true,
-              "Use @bazel_tools//python/tools:python_version instead."));
+          new SelectRestriction(/*visibleWithinToolsPackage=*/ true, /*errorMessage=*/ null));
       restrictions.put(
           HOST_FORCE_PYTHON_DEFINITION,
-          new SelectRestriction(
-              /*visibleWithinToolsPackage=*/ false,
-              "Use @bazel_tools//python/tools:python_version instead."));
+          new SelectRestriction(/*visibleWithinToolsPackage=*/ false, /*errorMessage=*/ null));
     }
     return restrictions.build();
   }

diff --git a/.../test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java b/.../test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java
@@ -15,11 +15,10 @@
 package com.google.devtools.build.lib.rules.python;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.devtools.build.lib.rules.python.PythonTestUtils.assumesDefaultIsPY2;
+import static com.google.devtools.build.lib.rules.python.PythonTestUtils.ensureDefaultIsPY2;
 
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
-import com.google.devtools.build.lib.packages.util.MockPythonSupport;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -69,16 +68,15 @@ public void goodSrcsVersionValue() throws Exception {
   }
 
   @Test
-  public void srcsVersionClashesWithVersionFlagUnderOldSemantics() throws Exception {
+  public void srcsVersionClashesWithForcePythonFlagUnderOldSemantics() throws Exception {
     // Under the old version semantics, we fail on any Python target the moment a conflict between
     // srcs_version and the configuration is detected. Under the new semantics, py_binary and
     // py_test care if there's a conflict but py_library does not. This test case checks the old
     // semantics; the new semantics are checked in PyLibraryConfiguredTargetTest and
     // PyExecutableConfiguredTargetTestBase. Note that under the new semantics py_binary and
     // py_library ignore the version flag, so those tests use the attribute to set the version
     // instead.
-    useConfiguration(
-        "--incompatible_allow_python_version_transitions=false", "--python_version=PY3");
+    useConfiguration("--experimental_allow_python_version_transitions=false", "--force_python=PY3");
     checkError("pkg", "foo",
         // error:
         "'//pkg:foo' can only be used with Python 2",
@@ -91,7 +89,7 @@ public void srcsVersionClashesWithVersionFlagUnderOldSemantics() throws Exceptio
 
   @Test
   public void versionIs2IfUnspecified() throws Exception {
-    assumesDefaultIsPY2();
+    ensureDefaultIsPY2();
     scratch.file(
         "pkg/BUILD", //
         ruleName + "(",
@@ -102,14 +100,13 @@ public void versionIs2IfUnspecified() throws Exception {
 
   @Test
   public void versionIs3IfForcedByFlagUnderOldSemantics() throws Exception {
-    // Under the old version semantics, --python_version takes precedence over the rule's own
+    // Under the old version semantics, --force_python takes precedence over the rule's own
     // default_python_version attribute, so this test case applies equally well to py_library,
     // py_binary, and py_test. Under the new semantics the rule attribute takes precedence, so this
     // would only make sense for py_library; see PyLibraryConfiguredTargetTest for the analogous
     // test.
-    assumesDefaultIsPY2();
-    useConfiguration(
-        "--incompatible_allow_python_version_transitions=false", "--python_version=PY3");
+    ensureDefaultIsPY2();
+    useConfiguration("--experimental_allow_python_version_transitions=false", "--force_python=PY3");
     scratch.file(
         "pkg/BUILD", //
         ruleName + "(",
@@ -272,46 +269,4 @@ public void requiresProvider() throws Exception {
         "    deps = [':dep'],",
         ")");
   }
-
-  @Test
-  public void loadFromBzl_WithMagicTagPasses() throws Exception {
-    useConfiguration("--incompatible_load_python_rules_from_bzl=true");
-    scratch.file(
-        "pkg/BUILD",
-        MockPythonSupport.getMacroLoadStatementFor(ruleName),
-        ruleName + "(",
-        "    name = 'foo',",
-        "    srcs = ['foo.py'],",
-        ")");
-    assertThat(getConfiguredTarget("//pkg:foo")).isNotNull();
-    assertNoEvents();
-  }
-
-  @Test
-  public void loadFromBzl_WithoutMagicTagFails() throws Exception {
-    useConfiguration("--incompatible_load_python_rules_from_bzl=true");
-    checkError(
-        "pkg",
-        "foo",
-        // error:
-        "Direct access to the native Python rules is deprecated",
-        // build file:
-        ruleName + "(",
-        "    name = 'foo',",
-        "    srcs = ['foo.py'],",
-        ")");
-  }
-
-  @Test
-  public void loadFromBzl_WithoutFlagPasses() throws Exception {
-    useConfiguration("--incompatible_load_python_rules_from_bzl=false");
-    scratch.file(
-        "pkg/BUILD", //
-        ruleName + "(",
-        "    name = 'foo',",
-        "    srcs = ['foo.py'],",
-        ")");
-    assertThat(getConfiguredTarget("//pkg:foo")).isNotNull();
-    assertNoEvents();
-  }
 }