diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java index 15f1f9635cde68..4a46cc52306c6e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java @@ -186,6 +186,7 @@ public PyCommon(RuleContext ruleContext, PythonSemantics semantics) { validateTargetPythonVersionAttr(DEFAULT_PYTHON_VERSION_ATTRIBUTE); validateTargetPythonVersionAttr(PYTHON_VERSION_ATTRIBUTE); validateOldVersionAttrNotUsedIfDisabled(); + validateLegacyProviderNotUsedIfDisabled(); } /** Returns the parsed value of the "srcs_version" attribute. */ @@ -233,7 +234,8 @@ private static void collectTransitivePythonSourcesFromDeps( builder.addTransitive(PyProviderUtils.getTransitiveSources(dep)); } catch (EvalException e) { // Either the provider type or field type is bad. - ruleContext.ruleError(String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); + ruleContext.attributeError( + "deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); } } } @@ -285,7 +287,8 @@ private static NestedSet initImports(RuleContext ruleContext, PythonSema builder.addTransitive(imports); } } catch (EvalException e) { - ruleContext.ruleError(String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); + ruleContext.attributeError( + "deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); } } return builder.build(); @@ -307,7 +310,8 @@ private static boolean initHasPy2OnlySources( return true; } } catch (EvalException e) { - ruleContext.ruleError(String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); + ruleContext.attributeError( + "deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); } } return false; @@ -328,7 +332,8 @@ private static boolean initHasPy3OnlySources( return true; } } catch (EvalException e) { - ruleContext.ruleError(String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); + ruleContext.attributeError( + "deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage())); } } return false; @@ -437,6 +442,27 @@ private void validateOldVersionAttrNotUsedIfDisabled() { } } + /** + * Reports an attribute error if a target in {@code deps} passes the legacy "py" provider but this + * is disallowed by the configuration. + */ + private void validateLegacyProviderNotUsedIfDisabled() { + if (!ruleContext.getFragment(PythonConfiguration.class).disallowLegacyPyProvider()) { + return; + } + for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps", Mode.TARGET)) { + if (PyProviderUtils.hasLegacyProvider(dep)) { + ruleContext.attributeError( + "deps", + String.format( + "In dep '%s': The legacy 'py' provider is disallowed. Migrate to the PyInfo " + + "provider instead. You can temporarily disable this failure with " + + "--incompatible_disallow_legacy_py_provider=false.", + dep.getLabel())); + } + } + } + /** * Under the new version semantics ({@code --experimental_allow_python_version_transitions=true}), * if the Python version (as determined by the configuration) is inconsistent with {@link @@ -562,7 +588,9 @@ public void addCommonTransitiveInfoProviders( NestedSet filesToBuild, NestedSet imports) { - PyProviderUtils.builder() + boolean createLegacyPyProvider = + !ruleContext.getFragment(PythonConfiguration.class).disallowLegacyPyProvider(); + PyProviderUtils.builder(createLegacyPyProvider) .setTransitiveSources(transitivePythonSources) .setUsesSharedLibraries(usesSharedLibraries) .setImports(imports) diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyProviderUtils.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyProviderUtils.java index 94dede4d38cf69..f9c1d815fb6b92 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyProviderUtils.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyProviderUtils.java @@ -170,17 +170,27 @@ public static boolean getHasPy3OnlySources(TransitiveInfoCollection target) thro } } - public static Builder builder() { - return new Builder(); + /** + * Returns a builder to construct the legacy and/or modern Python providers and add them to a + * configured target. + * + *

If {@code createLegacy} is false, only the modern {@code PyInfo} provider is produced. + * Otherwise both {@code PyInfo} and the "py" provider are produced. + */ + public static Builder builder(boolean createLegacy) { + return new Builder(createLegacy); } /** A builder to add both the legacy and modern providers to a configured target. */ public static class Builder { private final PyInfo.Builder modernBuilder = PyInfo.builder(); private final PyStructUtils.Builder legacyBuilder = PyStructUtils.builder(); + private final boolean createLegacy; // Use the static builder() method instead. - private Builder() {} + private Builder(boolean createLegacy) { + this.createLegacy = createLegacy; + } public Builder setTransitiveSources(NestedSet transitiveSources) { modernBuilder.setTransitiveSources(transitiveSources); @@ -214,8 +224,10 @@ public Builder setHasPy3OnlySources(boolean hasPy3OnlySources) { public RuleConfiguredTargetBuilder buildAndAddToTarget( RuleConfiguredTargetBuilder targetBuilder) { - targetBuilder.addSkylarkTransitiveInfo(PyStructUtils.PROVIDER_NAME, legacyBuilder.build()); targetBuilder.addNativeDeclaredProvider(modernBuilder.build()); + if (createLegacy) { + targetBuilder.addSkylarkTransitiveInfo(PyStructUtils.PROVIDER_NAME, legacyBuilder.build()); + } return targetBuilder; } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java index cefd6659895dda..86373c5841bb53 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java @@ -42,21 +42,26 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { private final TriState buildPythonZip; private final boolean buildTransitiveRunfilesTrees; - // TODO(brandjon): Remove these once migration to the new API is complete (#6583). + // 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 the new provider is complete (#7010). + private final boolean disallowLegacyPyProvider; + PythonConfiguration( PythonVersion version, TriState buildPythonZip, boolean buildTransitiveRunfilesTrees, boolean oldPyVersionApiAllowed, - boolean useNewPyVersionSemantics) { + boolean useNewPyVersionSemantics, + boolean disallowLegacyPyProvider) { this.version = version; this.buildPythonZip = buildPythonZip; this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees; this.oldPyVersionApiAllowed = oldPyVersionApiAllowed; this.useNewPyVersionSemantics = useNewPyVersionSemantics; + this.disallowLegacyPyProvider = disallowLegacyPyProvider; } /** @@ -132,4 +137,14 @@ public boolean oldPyVersionApiAllowed() { 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}. + * + *

Any rules that pass this provider should be updated to pass {@code PyInfo} instead. + */ + public boolean disallowLegacyPyProvider() { + return disallowLegacyPyProvider; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java index 1fc48f52878dcb..6a1e1059372f0e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java @@ -26,7 +26,7 @@ public class PythonConfigurationLoader implements ConfigurationFragmentFactory { @Override public ImmutableSet> requiredOptions() { - return ImmutableSet.>of(PythonOptions.class); + return ImmutableSet.of(PythonOptions.class); } @Override @@ -39,7 +39,8 @@ public PythonConfiguration create(BuildOptions buildOptions) pythonOptions.buildPythonZip, pythonOptions.buildTransitiveRunfilesTrees, /*oldPyVersionApiAllowed=*/ !pythonOptions.experimentalRemoveOldPythonVersionApi, - /*useNewPyVersionSemantics=*/ pythonOptions.experimentalAllowPythonVersionTransitions); + /*useNewPyVersionSemantics=*/ pythonOptions.experimentalAllowPythonVersionTransitions, + /*disallowLegacyPyProvider=*/ pythonOptions.experimentalDisallowLegacyPyProvider); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index b95a46c28f93e4..4e0b5b934eb1c6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -169,6 +169,19 @@ public String getTypeDescription() { private static final OptionDefinition HOST_FORCE_PYTHON_DEFINITION = OptionsParser.getOptionDefinitionByName(PythonOptions.class, "host_force_python"); + // TODO(#7010): Change the option name to "incompatible_..." and enable the appropriate metadata + // tags. + @Option( + name = "experimental_disallow_legacy_py_provider", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "If set to true, native Python rules will neither produce nor consume the legacy \"py\" " + + "provider. Use PyInfo instead. Under this flag, passing the legacy provider to a " + + "Python target will be an error.") + public boolean experimentalDisallowLegacyPyProvider; + @Override public Map getSelectRestrictions() { // TODO(brandjon): Add an error string that references documentation explaining to use @@ -271,6 +284,7 @@ public FragmentOptions getHost() { (hostForcePython != null) ? hostForcePython : PythonVersion.DEFAULT_TARGET_VALUE; hostPythonOptions.setPythonVersion(hostVersion); hostPythonOptions.buildPythonZip = buildPythonZip; + hostPythonOptions.experimentalDisallowLegacyPyProvider = experimentalDisallowLegacyPyProvider; return hostPythonOptions; } diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java index a1a3794049732d..527b67b6a78d51 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java @@ -141,7 +141,8 @@ public void srcsPackageNameCannotHaveHyphen() throws Exception { } @Test - public void producesBothModernAndLegacyProviders() throws Exception { + public void producesBothModernAndLegacyProviders_WithoutIncompatibleFlag() throws Exception { + useConfiguration("--experimental_disallow_legacy_py_provider=false"); scratch.file( "pkg/BUILD", // ruleName + "(", @@ -153,7 +154,21 @@ public void producesBothModernAndLegacyProviders() throws Exception { } @Test - public void consumesLegacyProvider() throws Exception { + public void producesOnlyModernProvider_WithIncompatibleFlag() throws Exception { + useConfiguration("--experimental_disallow_legacy_py_provider=true"); + scratch.file( + "pkg/BUILD", // + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'])"); + ConfiguredTarget target = getConfiguredTarget("//pkg:foo"); + assertThat(target.get(PyInfo.PROVIDER)).isNotNull(); + assertThat(target.get(PyStructUtils.PROVIDER_NAME)).isNull(); + } + + @Test + public void consumesLegacyProvider_WithoutIncompatibleFlag() throws Exception { + useConfiguration("--experimental_disallow_legacy_py_provider=false"); scratch.file( "pkg/rules.bzl", "def _myrule_impl(ctx):", @@ -177,6 +192,33 @@ public void consumesLegacyProvider() throws Exception { assertNoEvents(); } + @Test + public void rejectsLegacyProvider_WithIncompatibleFlag() throws Exception { + useConfiguration("--experimental_disallow_legacy_py_provider=true"); + scratch.file( + "pkg/rules.bzl", + "def _myrule_impl(ctx):", + " return struct(py=struct(transitive_sources=depset([])))", + "myrule = rule(", + " implementation = _myrule_impl,", + ")"); + checkError( + "pkg", + "foo", + // error: + "In dep '//pkg:dep': The legacy 'py' provider is disallowed.", + // build file: + "load(':rules.bzl', 'myrule')", + "myrule(", + " name = 'dep',", + ")", + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'],", + " deps = [':dep'],", + ")"); + } + @Test public void consumesModernProvider() throws Exception { scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java index c4e78f9ac3e555..60add6fe63cdf7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java @@ -164,11 +164,13 @@ public void getHost_CopiesMostValues() throws Exception { parsePythonOptions( "--experimental_allow_python_version_transitions=true", "--experimental_remove_old_python_version_api=true", - "--build_python_zip=true"); + "--build_python_zip=true", + "--experimental_disallow_legacy_py_provider=true"); PythonOptions hostOpts = (PythonOptions) opts.getHost(); assertThat(hostOpts.experimentalAllowPythonVersionTransitions).isTrue(); assertThat(hostOpts.experimentalRemoveOldPythonVersionApi).isTrue(); assertThat(hostOpts.buildPythonZip).isEqualTo(TriState.YES); + assertThat(hostOpts.experimentalDisallowLegacyPyProvider).isTrue(); } @Test