From 860f3801e1ca9db1484a1ce6803129660c299b95 Mon Sep 17 00:00:00 2001 From: Luca Di Grazia Date: Sun, 4 Sep 2022 18:10:12 +0200 Subject: [PATCH] Turn deprecated --force_python into a no-op The flag now behaves as if it is always unset. It will be removed entirely in a separate CL. In the meantime, it is still disallowed to select() on the flag. Work toward #7797. RELNOTES: None PiperOrigin-RevId: 302665836 --- .../lib/rules/python/PythonConfiguration.java | 36 ++- .../build/lib/rules/python/PythonOptions.java | 121 +++++--- .../PyBaseConfiguredTargetTestBase.java | 64 ++++ .../PyExecutableConfiguredTargetTestBase.java | 285 +++++++++++++++--- .../rules/python/PythonConfigurationTest.java | 116 +++++-- .../rules/python/PythonVersionSelectTest.java | 14 +- 6 files changed, 531 insertions(+), 105 deletions(-) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java index 99e327f0c20..9acc4cf49ce 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java @@ -16,8 +16,11 @@ import com.google.common.base.Preconditions; import com.google.common.base.Verify; -import com.google.devtools.build.lib.analysis.config.Fragment; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.StarlarkValue; @@ -33,13 +36,17 @@ name = "py", doc = "A configuration fragment for Python.", category = SkylarkModuleCategory.CONFIGURATION_FRAGMENT) -public class PythonConfiguration extends Fragment implements StarlarkValue { +public class PythonConfiguration extends BuildConfiguration.Fragment implements StarlarkValue { private final PythonVersion version; private final PythonVersion defaultVersion; private final TriState buildPythonZip; private final boolean buildTransitiveRunfilesTrees; + // TODO(brandjon): Remove these once migration to the new version API is complete (#6583). + private final boolean oldPyVersionApiAllowed; + private final boolean useNewPyVersionSemantics; + // TODO(brandjon): Remove this once migration to PY3-as-default is complete. private final boolean py2OutputsAreSuffixed; @@ -59,6 +66,8 @@ public class PythonConfiguration extends Fragment implements StarlarkValue { PythonVersion defaultVersion, TriState buildPythonZip, boolean buildTransitiveRunfilesTrees, + boolean oldPyVersionApiAllowed, + boolean useNewPyVersionSemantics, boolean py2OutputsAreSuffixed, boolean disallowLegacyPyProvider, boolean useToolchains, @@ -68,6 +77,8 @@ public class PythonConfiguration extends Fragment implements StarlarkValue { this.defaultVersion = defaultVersion; this.buildPythonZip = buildPythonZip; this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees; + this.oldPyVersionApiAllowed = oldPyVersionApiAllowed; + this.useNewPyVersionSemantics = useNewPyVersionSemantics; this.py2OutputsAreSuffixed = py2OutputsAreSuffixed; this.disallowLegacyPyProvider = disallowLegacyPyProvider; this.useToolchains = useToolchains; @@ -120,6 +131,17 @@ public String getOutputDirectoryName() { } } + @Override + public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) { + PythonOptions opts = buildOptions.get(PythonOptions.class); + if (opts.incompatiblePy3IsDefault && !opts.incompatibleAllowPythonVersionTransitions) { + reporter.handle( + Event.error( + "cannot enable `--incompatible_py3_is_default` without also enabling " + + "`--incompatible_allow_python_version_transitions`")); + } + } + /** Returns whether to build the executable zip file for Python binaries. */ public boolean buildPythonZip() { switch (buildPythonZip) { @@ -140,6 +162,16 @@ public boolean buildTransitiveRunfilesTrees() { return buildTransitiveRunfilesTrees; } + /** Returns whether use of the {@code default_python_version} attribute is allowed. */ + 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; + } + /** * Returns true if Python rules should omit the legacy "py" provider and fail-fast when given this * provider from their {@code deps}. diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index cc93d3a2163..44b835c4712 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -69,40 +69,34 @@ public String getTypeDescription() { help = "Build python executable zip; on on Windows, off on other platforms") public TriState buildPythonZip; - /** - * Deprecated machinery for setting the Python version; will be removed soon. - * - *

Not GraveyardOptions'd because we'll delete this alongside other soon-to-be-removed options - * in this file. - */ @Option( name = "incompatible_remove_old_python_version_api", defaultValue = "true", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, metadataTags = { OptionMetadataTag.INCOMPATIBLE_CHANGE, OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES }, - help = "No-op, will be removed soon.") + help = + "If true, disables use of 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`.") public boolean incompatibleRemoveOldPythonVersionApi; - /** - * Deprecated machinery for setting the Python version; will be removed soon. - * - *

Not GraveyardOptions'd because we'll delete this alongside other soon-to-be-removed options - * in this file. - */ @Option( name = "incompatible_allow_python_version_transitions", defaultValue = "true", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, metadataTags = { OptionMetadataTag.INCOMPATIBLE_CHANGE, OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES }, - help = "No-op, will be removed soon.") + 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.") public boolean incompatibleAllowPythonVersionTransitions; /** @@ -123,9 +117,10 @@ public String getTypeDescription() { }, help = "If true, `py_binary` and `py_test` targets that do not set their `python_version` (or " - + "`default_python_version`) attribute will default to PY3 rather than to PY2. If " - + "you set this flag it is also recommended to set " - + "`--incompatible_py2_outputs_are_suffixed`.") + + "`default_python_version`) attribute will default to PY3 rather than to PY2. It is " + + "an error to set this flag without also enabling " + + "`--incompatible_allow_python_version_transitions`. If you set this flag it is " + + "also recommended to set `--incompatible_py2_outputs_are_suffixed`.") public boolean incompatiblePy3IsDefault; @Option( @@ -164,8 +159,9 @@ public String getTypeDescription() { OptionEffectTag.AFFECTS_OUTPUTS // because of "-py2"/"-py3" output root }, help = - "The Python major version mode, either `PY2` or `PY3`. Note that this is overridden by " - + "`py_binary` and `py_test` targets (even if they don't explicitly specify a " + "The Python major version mode, either `PY2` or `PY3`. Note that under the new version " + + "semantics (`--incompatible_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.") public PythonVersion pythonVersion; @@ -225,7 +221,6 @@ public String getTypeDescription() { + "Python target will be an error.") public boolean incompatibleDisallowLegacyPyProvider; - // TODO(b/153369373): Delete this flag. @Option( name = "incompatible_use_python_toolchains", defaultValue = "true", @@ -241,6 +236,21 @@ public String getTypeDescription() { + "--python_top.") public boolean incompatibleUsePythonToolchains; + @Option( + name = "incompatible_load_python_rules_from_bzl", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES, + }, + help = + "If enabled, direct use of the native Python rules is disabled. Please use the Starlark " + + "rules instead: https://github.com/bazelbuild/rules_python. See also " + + "https://github.com/bazelbuild/bazel/issues/9006.") + public boolean loadPythonRulesFromBzl; + @Option( name = "experimental_build_transitive_python_runfiles", defaultValue = "true", @@ -278,16 +288,18 @@ public Map getSelectRestrictions() { new SelectRestriction( /*visibleWithinToolsPackage=*/ true, "Use @bazel_tools//python/tools:python_version instead.")); - restrictions.put( - FORCE_PYTHON_DEFINITION, - new SelectRestriction( - /*visibleWithinToolsPackage=*/ true, - "Use @bazel_tools//python/tools:python_version instead.")); - restrictions.put( - HOST_FORCE_PYTHON_DEFINITION, - new SelectRestriction( - /*visibleWithinToolsPackage=*/ false, - "Use @bazel_tools//python/tools:python_version instead.")); + if (incompatibleRemoveOldPythonVersionApi) { + restrictions.put( + FORCE_PYTHON_DEFINITION, + new SelectRestriction( + /*visibleWithinToolsPackage=*/ true, + "Use @bazel_tools//python/tools:python_version instead.")); + restrictions.put( + HOST_FORCE_PYTHON_DEFINITION, + new SelectRestriction( + /*visibleWithinToolsPackage=*/ false, + "Use @bazel_tools//python/tools:python_version instead.")); + } return restrictions.build(); } @@ -314,24 +326,44 @@ public PythonVersion getPythonVersion() { } /** - * Returns whether a Python version transition to {@code version} is not a no-op. + * Returns whether a Python version transition to {@code version} is allowed and not a no-op. + * + *

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. + * + *

Under the old semantics ({@link #incompatibleAllowPythonVersionTransitions} is false), + * version transitions are not allowed once the version has already been set ({@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()); - return !version.equals(getPythonVersion()); + if (incompatibleAllowPythonVersionTransitions) { + return !version.equals(getPythonVersion()); + } else { + boolean currentlyUnset = pythonVersion == null; + boolean transitioningToNonDefault = !version.equals(getDefaultPythonVersion()); + return currentlyUnset && transitioningToNonDefault; + } } /** - * Sets the Python version to {@code version}. + * Manipulates the Python version fields so that {@link #getPythonVersion()} returns {@code + * version}. * - *

Since this is a mutation, it should only be called on a newly constructed instance. + *

This method is a mutation on the current instance, so it should only be invoked on a newly + * constructed instance. The mutation does not depend on whether or not {@link + * #canTransitionPythonVersion} would return true. + * + *

If the old semantics are in effect ({@link #incompatibleAllowPythonVersionTransitions} is + * false), after this method is called {@link #canTransitionPythonVersion} will return false. * * @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3} */ - // TODO(brandjon): Consider removing this mutator now that the various flags and semantics it - // used to consider are gone. We'd revert to just setting the public option field directly. public void setPythonVersion(PythonVersion version) { Preconditions.checkArgument(version.isTargetValue()); this.pythonVersion = version; @@ -340,6 +372,9 @@ public void setPythonVersion(PythonVersion version) { @Override public FragmentOptions getHost() { PythonOptions hostPythonOptions = (PythonOptions) getDefault(); + hostPythonOptions.incompatibleRemoveOldPythonVersionApi = incompatibleRemoveOldPythonVersionApi; + hostPythonOptions.incompatibleAllowPythonVersionTransitions = + incompatibleAllowPythonVersionTransitions; PythonVersion hostVersion = (hostForcePython != null) ? hostForcePython : getDefaultPythonVersion(); hostPythonOptions.setPythonVersion(hostVersion); @@ -348,6 +383,7 @@ public FragmentOptions getHost() { hostPythonOptions.buildPythonZip = buildPythonZip; hostPythonOptions.incompatibleDisallowLegacyPyProvider = incompatibleDisallowLegacyPyProvider; hostPythonOptions.incompatibleUsePythonToolchains = incompatibleUsePythonToolchains; + hostPythonOptions.loadPythonRulesFromBzl = loadPythonRulesFromBzl; // Save host options in case of a further exec->host transition. hostPythonOptions.hostForcePython = hostForcePython; @@ -357,10 +393,13 @@ public FragmentOptions getHost() { @Override public FragmentOptions getNormalized() { - // We want to ensure that options with "null" physical default values are normalized, to avoid - // #7808. + // 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. PythonOptions newOptions = (PythonOptions) clone(); - newOptions.setPythonVersion(newOptions.getPythonVersion()); + if (incompatibleAllowPythonVersionTransitions) { + newOptions.setPythonVersion(newOptions.getPythonVersion()); + } return newOptions; } } diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java index 5fb3a8a025a..d3767f6cba1 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java @@ -68,6 +68,27 @@ public void goodSrcsVersionValue() throws Exception { assertNoEvents(); } + @Test + public void srcsVersionClashesWithVersionFlagUnderOldSemantics() 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"); + checkError("pkg", "foo", + // error: + "'//pkg:foo' can only be used with Python 2", + // build file: + ruleName + "(", + " name = 'foo',", + " srcs = [':foo.py'],", + " srcs_version = 'PY2ONLY')"); + } + @Test public void versionIs2IfUnspecified() throws Exception { assumesDefaultIsPY2(); @@ -79,6 +100,49 @@ public void versionIs2IfUnspecified() throws Exception { assertThat(getPythonVersion(getConfiguredTarget("//pkg:foo"))).isEqualTo(PythonVersion.PY2); } + @Test + public void versionIs3IfForcedByFlagUnderOldSemantics() throws Exception { + // Under the old version semantics, --python_version 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"); + scratch.file( + "pkg/BUILD", // + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'])"); + assertThat(getPythonVersion(getConfiguredTarget("//pkg:foo"))).isEqualTo(PythonVersion.PY3); + } + + @Test + public void packageNameCannotHaveHyphen() throws Exception { + checkError("pkg-hyphenated", "foo", + // error: + "paths to Python packages may not contain '-'", + // build file: + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'])"); + } + + @Test + public void srcsPackageNameCannotHaveHyphen() throws Exception { + scratch.file( + "pkg-hyphenated/BUILD", // + "exports_files(['bar.py'])"); + checkError("otherpkg", "foo", + // error: + "paths to Python packages may not contain '-'", + // build file: + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py', '//pkg-hyphenated:bar.py'])"); + } + @Test public void producesBothModernAndLegacyProviders_WithoutIncompatibleFlag() throws Exception { useConfiguration("--incompatible_disallow_legacy_py_provider=false"); diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java index d5deeb14367..5cd8e8b9ca8 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java @@ -79,11 +79,6 @@ protected String getPyExecutableDeferredError(String label) throws Exception { return ((FailAction) action).getErrorMessage(); } - /** Asserts that a configured target has the given Python version. */ - protected void assertPythonVersionIs(String targetName, PythonVersion version) throws Exception { - assertThat(getPythonVersion(getOkPyTarget(targetName))).isEqualTo(version); - } - /** * Sets the configuration, then asserts that a configured target has the given Python version. * @@ -92,7 +87,7 @@ protected void assertPythonVersionIs(String targetName, PythonVersion version) t protected void assertPythonVersionIs_UnderNewConfig( String targetName, PythonVersion version, String... flags) throws Exception { useConfiguration(flags); - assertPythonVersionIs(targetName, version); + assertThat(getPythonVersion(getOkPyTarget(targetName))).isEqualTo(version); } /** @@ -116,6 +111,15 @@ private static String join(String... lines) { return String.join("\n", lines); } + private String ruleDeclWithDefaultPyVersionAttr(String name, String version) { + return join( + ruleName + "(", + " name = '" + name + "',", + " srcs = ['" + name + ".py'],", + " default_python_version = '" + version + "',", + ")"); + } + private String ruleDeclWithPyVersionAttr(String name, String version) { return join( ruleName + "(", @@ -138,7 +142,20 @@ public void pyRuntimeInfoIsPresent() throws Exception { } @Test - public void versionAttr_UnknownValue() throws Exception { + public void oldVersionAttr_UnknownValue() throws Exception { + useConfiguration("--incompatible_remove_old_python_version_api=false"); + checkError( + "pkg", + "foo", + // error: + "invalid value in 'default_python_version' attribute: " + + "has to be one of 'PY2' or 'PY3' instead of 'doesnotexist'", + // build file: + ruleDeclWithDefaultPyVersionAttr("foo", "doesnotexist")); + } + + @Test + public void newVersionAttr_UnknownValue() throws Exception { checkError( "pkg", "foo", @@ -150,7 +167,20 @@ public void versionAttr_UnknownValue() throws Exception { } @Test - public void versionAttr_BadValue() throws Exception { + public void oldVersionAttr_BadValue() throws Exception { + useConfiguration("--incompatible_remove_old_python_version_api=false"); + checkError( + "pkg", + "foo", + // error: + "invalid value in 'default_python_version' attribute: " + + "has to be one of 'PY2' or 'PY3' instead of 'PY2AND3'", + // build file: + ruleDeclWithDefaultPyVersionAttr("foo", "PY2AND3")); + } + + @Test + public void newVersionAttr_BadValue() throws Exception { checkError( "pkg", "foo", @@ -162,27 +192,86 @@ public void versionAttr_BadValue() throws Exception { } @Test - public void versionAttr_GoodValue() throws Exception { + public void oldVersionAttr_GoodValue() throws Exception { + useConfiguration("--incompatible_remove_old_python_version_api=false"); + scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY2")); + getOkPyTarget("//pkg:foo"); + assertNoEvents(); + } + + @Test + public void newVersionAttr_GoodValue() throws Exception { scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2")); getOkPyTarget("//pkg:foo"); assertNoEvents(); } @Test - public void py3IsDefaultFlag_SetsDefaultPythonVersion() throws Exception { + public void cannotUseOldVersionAttrWithRemovalFlag() throws Exception { + useConfiguration("--incompatible_remove_old_python_version_api=true"); + checkError( + "pkg", + "foo", + // error: + "the 'default_python_version' attribute is disabled by the " + + "'--incompatible_remove_old_python_version_api' flag", + // build file: + ruleDeclWithDefaultPyVersionAttr("foo", "PY2")); + } + + /** + * Regression test for #7071: Don't let prohibiting the old attribute get in the way of cloning a + * target using {@code native.existing_rules()}. + * + *

The use case of cloning a target is pretty dubious and brittle. But as long as it's possible + * and not proscribed, we won't let version attribute validation get in the way. + */ + @Test + public void canCopyTargetWhenOldAttrDisallowed() throws Exception { + useConfiguration("--incompatible_remove_old_python_version_api=true"); scratch.file( - "pkg/BUILD", // + "pkg/rules.bzl", + "def copy_target(rulefunc, src, dest):", + " t = native.existing_rule(src)", + " t.pop('kind')", + " t.pop('name')", + " # Also remove these because they get in the way of creating the new target but aren't", + " # related to the attribute under test.", + " t.pop('restricted_to')", + " t.pop('shard_count', default=None)", + " rulefunc(name = dest, **t)"); + scratch.file( + "pkg/BUILD", + "load(':rules.bzl', 'copy_target')", ruleName + "(", " name = 'foo',", " srcs = ['foo.py'],", + " main = 'foo.py',", + " python_version = 'PY2',", + ")", + "copy_target(" + ruleName + ", 'foo', 'bar')"); + ConfiguredTarget target = getConfiguredTarget("//pkg:bar"); + assertThat(target).isNotNull(); + } + + @Test + public void py3IsDefaultFlag_SetsDefaultPythonVersion() throws Exception { + scratch.file( // + "pkg/BUILD", // + ruleName + "(", // + " name = 'foo',", // + " srcs = ['foo.py'],", // ")"); + // --incompatible_py3_is_default requires --incompatible_allow_python_version_transitions assertPythonVersionIs_UnderNewConfig( "//pkg:foo", PythonVersion.PY2, + "--incompatible_allow_python_version_transitions=true", "--incompatible_py3_is_default=false"); assertPythonVersionIs_UnderNewConfig( "//pkg:foo", PythonVersion.PY3, + "--incompatible_allow_python_version_transitions=true", "--incompatible_py3_is_default=true", // Keep the host Python as PY2, because we don't want to drag any implicit dependencies on // tools into PY3 for this test. (Doing so may require setting extra options to get it to @@ -193,9 +282,11 @@ public void py3IsDefaultFlag_SetsDefaultPythonVersion() throws Exception { @Test public void py3IsDefaultFlag_DoesntOverrideExplicitVersion() throws Exception { scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2")); + // --incompatible_py3_is_default requires --incompatible_allow_python_version_transitions assertPythonVersionIs_UnderNewConfig( "//pkg:foo", PythonVersion.PY2, + "--incompatible_allow_python_version_transitions=true", "--incompatible_py3_is_default=true", // Keep the host Python as PY2, because we don't want to drag any implicit dependencies on // tools into PY3 for this test. (Doing so may require setting extra options to get it to @@ -204,51 +295,170 @@ public void py3IsDefaultFlag_DoesntOverrideExplicitVersion() throws Exception { } @Test - public void versionAttrWorks_WhenNotDefaultValue() throws Exception { + public void newVersionAttrTakesPrecedenceOverOld() throws Exception { + scratch.file( + "pkg/BUILD", + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'],", + " default_python_version = 'PY2',", + " python_version = 'PY3',", + ")"); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", PythonVersion.PY3, "--incompatible_remove_old_python_version_api=false"); + } + + @Test + public void versionAttrWorksUnderOldAndNewSemantics_WhenNotDefaultValue() throws Exception { assumesDefaultIsPY2(); scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3")); - assertPythonVersionIs("//pkg:foo", PythonVersion.PY3); + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo", + PythonVersion.PY3, + new String[] {"--incompatible_allow_python_version_transitions=false"}, + new String[] {"--incompatible_allow_python_version_transitions=true"}); + } + + @Test + public void versionAttrWorksUnderOldAndNewSemantics_WhenSameAsDefaultValue() throws Exception { + assumesDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2")); + + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo", + PythonVersion.PY2, + new String[] {"--incompatible_allow_python_version_transitions=false"}, + new String[] {"--incompatible_allow_python_version_transitions=true"}); } @Test - public void versionAttrWorks_WhenSameAsDefaultValue() throws Exception { + public void flagTakesPrecedenceUnderOldSemantics_NonDefaultValue() throws Exception { assumesDefaultIsPY2(); scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2")); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", + PythonVersion.PY3, + "--incompatible_allow_python_version_transitions=false", + "--python_version=PY3"); + } - assertPythonVersionIs("//pkg:foo", PythonVersion.PY2); + @Test + public void flagTakesPrecedenceUnderOldSemantics_DefaultValue() throws Exception { + assumesDefaultIsPY2(); + scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3")); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", + PythonVersion.PY2, + "--incompatible_allow_python_version_transitions=false", + "--python_version=PY2"); } @Test - public void versionAttrTakesPrecedence_NonDefaultValue() throws Exception { + public void versionAttrTakesPrecedenceUnderNewSemantics_NonDefaultValue() throws Exception { assumesDefaultIsPY2(); scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3")); - assertPythonVersionIs_UnderNewConfig("//pkg:foo", PythonVersion.PY3, "--python_version=PY2"); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", + PythonVersion.PY3, + "--incompatible_allow_python_version_transitions=true", + "--python_version=PY2"); } @Test - public void versionAttrTakesPrecedence_DefaultValue() throws Exception { + public void versionAttrTakesPrecedenceUnderNewSemantics_DefaultValue() throws Exception { assumesDefaultIsPY2(); scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2")); - assertPythonVersionIs_UnderNewConfig("//pkg:foo", PythonVersion.PY2, "--python_version=PY3"); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo", + PythonVersion.PY2, + "--incompatible_allow_python_version_transitions=true", + "--python_version=PY3"); + } + + @Test + public void canBuildWithDifferentVersionAttrs_UnderOldAndNewSemantics() throws Exception { + scratch.file( + "pkg/BUILD", + ruleDeclWithPyVersionAttr("foo_v2", "PY2"), + ruleDeclWithPyVersionAttr("foo_v3", "PY3")); + + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo_v2", + PythonVersion.PY2, + new String[] {"--incompatible_allow_python_version_transitions=false"}, + new String[] {"--incompatible_allow_python_version_transitions=true"}); + assertPythonVersionIs_UnderNewConfigs( + "//pkg:foo_v3", + PythonVersion.PY3, + new String[] {"--incompatible_allow_python_version_transitions=false"}, + new String[] {"--incompatible_allow_python_version_transitions=true"}); + } + + @Test + public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToDefault() + throws Exception { + assumesDefaultIsPY2(); + scratch.file( + "pkg/BUILD", + ruleDeclWithPyVersionAttr("foo_v2", "PY2"), + ruleDeclWithPyVersionAttr("foo_v3", "PY3")); + + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo_v2", + PythonVersion.PY2, + "--incompatible_allow_python_version_transitions=false", + "--python_version=PY2"); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo_v3", + PythonVersion.PY2, + "--incompatible_allow_python_version_transitions=false", + "--python_version=PY2"); } @Test - public void canBuildWithDifferentVersionAttrs() throws Exception { + public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToNonDefault() + throws Exception { + assumesDefaultIsPY2(); scratch.file( "pkg/BUILD", ruleDeclWithPyVersionAttr("foo_v2", "PY2"), ruleDeclWithPyVersionAttr("foo_v3", "PY3")); - assertPythonVersionIs("//pkg:foo_v2", PythonVersion.PY2); - assertPythonVersionIs("//pkg:foo_v3", PythonVersion.PY3); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo_v2", + PythonVersion.PY3, + "--incompatible_allow_python_version_transitions=false", + "--python_version=PY3"); + assertPythonVersionIs_UnderNewConfig( + "//pkg:foo_v3", + PythonVersion.PY3, + "--incompatible_allow_python_version_transitions=false", + "--python_version=PY3"); } @Test - public void incompatibleSrcsVersion() throws Exception { + public void incompatibleSrcsVersion_OldSemantics() throws Exception { + useConfiguration("--incompatible_allow_python_version_transitions=false"); + checkError( + "pkg", + "foo", + // error: + "'//pkg:foo' can only be used with Python 2", + // build file: + ruleName + "(", + " name = 'foo',", + " srcs = [':foo.py'],", + " srcs_version = 'PY2ONLY',", + " default_python_version = 'PY3')"); + } + + @Test + public void incompatibleSrcsVersion_NewSemantics() throws Exception { reporter.removeHandler(failFastHandler); // We assert below that we don't fail at analysis. + useConfiguration("--incompatible_allow_python_version_transitions=true"); scratch.file( "pkg/BUILD", // build file: @@ -257,24 +467,29 @@ public void incompatibleSrcsVersion() throws Exception { " srcs = [':foo.py'],", " srcs_version = 'PY2ONLY',", " python_version = 'PY3')"); + // Under the new semantics, this is an execution-time error, not an analysis-time one. We fail + // by setting the generating action to FailAction. + assertNoEvents(); assertThat(getPyExecutableDeferredError("//pkg:foo")) .contains("being built for Python 3 but (transitively) includes Python 2-only sources"); - // This is an execution-time error, not an analysis-time one. We fail by setting the generating - // action to FailAction. - assertNoEvents(); } @Test - public void targetInPackageWithHyphensOkIfSrcsFromOtherPackage() throws Exception { - scratch.file( - "pkg/BUILD", // - "exports_files(['foo.py'])"); - scratch.file( - "pkg-with-hyphens/BUILD", + public void incompatibleSrcsVersion_DueToVersionAttrDefault() throws Exception { + assumesDefaultIsPY2(); // When changed to PY3, flip srcs_version below to be PY2ONLY. + + // This test doesn't care whether we use old and new semantics, but it affects how we assert. + useConfiguration("--incompatible_allow_python_version_transitions=false"); + + // Fails because default_python_version is PY2 by default, so the config is set to PY2 + // regardless of srcs_version. + checkError("pkg", "foo", + // error: + "'//pkg:foo' can only be used with Python 3", + // build file: ruleName + "(", " name = 'foo',", - " main = '//pkg:foo.py',", - " srcs = ['//pkg:foo.py'])"); - getOkPyTarget("//pkg-with-hyphens:foo"); // should not fail + " srcs = [':foo.py'],", + " srcs_version = 'PY3ONLY')"); } } diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java index b3240800602..998fc0c2aa1 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java @@ -16,11 +16,10 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.rules.python.PythonTestUtils.assumesDefaultIsPY2; -import static org.junit.Assert.assertThrows; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.util.ConfigurationTestCase; -import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.TriState; import org.junit.Test; @@ -31,7 +30,6 @@ @RunWith(JUnit4.class) public class PythonConfigurationTest extends ConfigurationTestCase { - // Do not mutate the returned PythonOptions - it will poison skyframe caches. private PythonOptions parsePythonOptions(String... cmdline) throws Exception { BuildConfiguration config = create(cmdline); return config.getOptions().get(PythonOptions.class); @@ -51,11 +49,26 @@ public void invalidTargetPythonValue_UnknownValue() { assertThat(expected).hasMessageThat().contains("Not a valid Python major version"); } + @Test + public void py3IsDefaultFlagRequiresNewSemanticsFlag() throws Exception { + checkError( + "cannot enable `--incompatible_py3_is_default` without also enabling " + + "`--incompatible_allow_python_version_transitions`", + "--incompatible_allow_python_version_transitions=false", + "--incompatible_py3_is_default=true"); + } + @Test public void getDefaultPythonVersion() throws Exception { + // --incompatible_py3_is_default requires --incompatible_allow_python_version_transitions PythonOptions withoutPy3IsDefaultOpts = - parsePythonOptions("--incompatible_py3_is_default=false"); - PythonOptions withPy3IsDefaultOpts = parsePythonOptions("--incompatible_py3_is_default=true"); + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_py3_is_default=false"); + PythonOptions withPy3IsDefaultOpts = + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_py3_is_default=true"); assertThat(withoutPy3IsDefaultOpts.getDefaultPythonVersion()).isEqualTo(PythonVersion.PY2); assertThat(withPy3IsDefaultOpts.getDefaultPythonVersion()).isEqualTo(PythonVersion.PY3); } @@ -63,28 +76,79 @@ public void getDefaultPythonVersion() throws Exception { @Test public void getPythonVersion_FallBackOnDefaultPythonVersion() throws Exception { // Run it twice with two different values for the incompatible flag to confirm it's actually - // reading getDefaultPythonVersion() and not some other source of default values. - PythonOptions py2Opts = parsePythonOptions("--incompatible_py3_is_default=false"); - PythonOptions py3Opts = parsePythonOptions("--incompatible_py3_is_default=true"); + // reading getDefaultPythonVersion() and not some other source of default values. Note that + // --incompatible_py3_is_default requires --incompatible_allow_python_version_transitions. + PythonOptions py2Opts = + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_py3_is_default=false"); + PythonOptions py3Opts = + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_py3_is_default=true"); assertThat(py2Opts.getPythonVersion()).isEqualTo(PythonVersion.PY2); assertThat(py3Opts.getPythonVersion()).isEqualTo(PythonVersion.PY3); } @Test - public void canTransitionPythonVersion_Yes() throws Exception { - PythonOptions opts = parsePythonOptions("--python_version=PY3"); + public void canTransitionPythonVersion_OldSemantics_Yes() throws Exception { + assumesDefaultIsPY2(); + PythonOptions opts = + parsePythonOptions("--incompatible_allow_python_version_transitions=false"); + assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isTrue(); + } + + @Test + public void canTransitionPythonVersion_OldSemantics_NoBecauseAlreadySet() throws Exception { + assumesDefaultIsPY2(); + PythonOptions opts = + parsePythonOptions( + "--incompatible_allow_python_version_transitions=false", "--python_version=PY2"); + assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isFalse(); + } + + @Test + public void canTransitionPythonVersion_OldSemantics_NoBecauseNewValueSameAsDefault() + throws Exception { + assumesDefaultIsPY2(); + PythonOptions opts = + parsePythonOptions("--incompatible_allow_python_version_transitions=false"); + assertThat(opts.canTransitionPythonVersion(PythonVersion.PY2)).isFalse(); + } + + @Test + public void canTransitionPythonVersion_NewSemantics_Yes() throws Exception { + PythonOptions opts = + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", "--python_version=PY3"); assertThat(opts.canTransitionPythonVersion(PythonVersion.PY2)).isTrue(); } @Test - public void canTransitionPythonVersion_NoBecauseSameAsCurrent() throws Exception { - PythonOptions opts = parsePythonOptions("--python_version=PY3"); + public void canTransitionPythonVersion_NewSemantics_NoBecauseSameAsCurrent() throws Exception { + PythonOptions opts = + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_remove_old_python_version_api=false", + "--python_version=PY3"); assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isFalse(); } @Test - public void setPythonVersion() throws Exception { - PythonOptions opts = Options.parse(PythonOptions.class, "--python_version=PY2").getOptions(); + public void setPythonVersion_OldApiEnabled() throws Exception { + PythonOptions opts = + parsePythonOptions( + "--incompatible_remove_old_python_version_api=false", + "--python_version=PY2"); + opts.setPythonVersion(PythonVersion.PY3); + 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.pythonVersion).isEqualTo(PythonVersion.PY3); } @@ -93,12 +157,16 @@ public void setPythonVersion() throws Exception { public void getHost_CopiesMostValues() throws Exception { PythonOptions opts = parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_remove_old_python_version_api=true", "--incompatible_py3_is_default=true", "--incompatible_py2_outputs_are_suffixed=true", "--build_python_zip=true", "--incompatible_disallow_legacy_py_provider=true", "--incompatible_use_python_toolchains=true"); PythonOptions hostOpts = (PythonOptions) opts.getHost(); + assertThat(hostOpts.incompatibleAllowPythonVersionTransitions).isTrue(); + assertThat(hostOpts.incompatibleRemoveOldPythonVersionApi).isTrue(); assertThat(hostOpts.incompatiblePy3IsDefault).isTrue(); assertThat(hostOpts.incompatiblePy2OutputsAreSuffixed).isTrue(); assertThat(hostOpts.buildPythonZip).isEqualTo(TriState.YES); @@ -112,7 +180,9 @@ public void getHost_AppliesHostForcePython() throws Exception { PythonOptions optsWithPythonVersionFlag = parsePythonOptions("--python_version=PY2", "--host_force_python=PY3"); PythonOptions optsWithPy3IsDefaultFlag = + // --incompatible_py3_is_default requires --incompatible_allow_python_version_transitions parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", "--incompatible_py3_is_default=true", // It's more interesting to set the incompatible flag true and force host to PY2, than // it is to set the flag false and force host to PY3. @@ -127,15 +197,27 @@ public void getHost_AppliesHostForcePython() throws Exception { @Test public void getHost_Py3IsDefaultFlagChangesHost() throws Exception { assumesDefaultIsPY2(); - PythonOptions opts = parsePythonOptions("--incompatible_py3_is_default=true"); + PythonOptions opts = + // --incompatible_py3_is_default requires --incompatible_allow_python_version_transitions + parsePythonOptions( + "--incompatible_allow_python_version_transitions=true", + "--incompatible_py3_is_default=true"); PythonOptions hostOpts = (PythonOptions) opts.getHost(); assertThat(hostOpts.getPythonVersion()).isEqualTo(PythonVersion.PY3); } @Test - public void getNormalized() throws Exception { + 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 { assumesDefaultIsPY2(); - PythonOptions opts = parsePythonOptions(); + PythonOptions opts = parsePythonOptions("--incompatible_allow_python_version_transitions=true"); PythonOptions normalizedOpts = (PythonOptions) opts.getNormalized(); assertThat(normalizedOpts.pythonVersion).isEqualTo(PythonVersion.PY2); } diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java index 413b48327f9..6b390586e87 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java @@ -72,6 +72,8 @@ public void cannotSelectOnNativePythonVersionFlag() throws Exception { public void canSelectOnForcePythonFlagsUnderOldApi() throws Exception { // For backwards compatibility purposes, select()-ing on --force_python and --host_force_python // is allowed while the old API is still enabled. + // TODO(brandjon): Although --force_python is a no-op, this test is still present because the + // behavior belongs to the remove-old-api flag. Delete the test when we no-op that flag too. useConfiguration("--incompatible_remove_old_python_version_api=false"); scratch.file("fp/BUILD", makeFooThatSelectsOnFlag("force_python", "PY2")); scratch.file("hfp/BUILD", makeFooThatSelectsOnFlag("host_force_python", "PY2")); @@ -119,7 +121,7 @@ public void selectOnPythonVersionTarget() throws Exception { " }),", ")"); - // Neither --python_version nor --force_python, use default value. + // No --python_version, use default value. doTestSelectOnPythonVersionTarget(py2, "--incompatible_py3_is_default=false"); doTestSelectOnPythonVersionTarget( py3, @@ -128,17 +130,9 @@ public void selectOnPythonVersionTarget() throws Exception { // enabled before we're allowed to set the default to PY3. "--incompatible_allow_python_version_transitions=true"); - // No --python_version, trust --force_python. - doTestSelectOnPythonVersionTarget(py2, "--force_python=PY2"); - doTestSelectOnPythonVersionTarget(py3, "--force_python=PY3"); - - // --python_version overrides --force_python. + // --python_version is given, use it. doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2"); - doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2", "--force_python=PY2"); - doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2", "--force_python=PY3"); doTestSelectOnPythonVersionTarget(py3, "--python_version=PY3"); - doTestSelectOnPythonVersionTarget(py3, "--python_version=PY3", "--force_python=PY2"); - doTestSelectOnPythonVersionTarget(py3, "--python_version=PY3", "--force_python=PY3"); } private void doTestSelectOnPythonVersionTarget(Artifact expected, String... flags)