Skip to content

Commit

Permalink
Make --incompatible_disallow_legacy_py_provider a no-op
Browse files Browse the repository at this point in the history 
The legacy `py` provider struct is now silently ignored instead of causing
an error.

Cleanup for #7741

PiperOrigin-RevId: 449568980
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed May 18, 2022
1 parent 8b4ca01 commit f068b31
Show file tree
Hide file tree
Showing 13 changed files with 85 additions and 1,297 deletions.
14 changes: 1 addition & 13 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,10 @@
import com.google.devtools.build.lib.packages.Attribute.LabelLateBoundDefault;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
import com.google.devtools.build.lib.packages.TriState;
import com.google.devtools.build.lib.rules.python.PyCommon;
import com.google.devtools.build.lib.rules.python.PyInfo;
import com.google.devtools.build.lib.rules.python.PyRuleClasses;
import com.google.devtools.build.lib.rules.python.PyStructUtils;
import com.google.devtools.build.lib.rules.python.PythonVersion;

/**
Expand Down Expand Up @@ -69,17 +67,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
<a href="${link py_library}"><code>py_library</code></a> rules.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.override(
builder
.copy("deps")
.mandatoryProvidersList(
ImmutableList.of(
// Legacy provider.
// TODO(b/153363654): Remove this legacy set.
ImmutableList.of(
StarlarkProviderIdentifier.forLegacy(PyStructUtils.PROVIDER_NAME)),
// Modern provider.
ImmutableList.of(PyInfo.PROVIDER.id())))
.allowedFileTypes())
builder.copy("deps").mandatoryProviders(PyInfo.PROVIDER.id()).allowedFileTypes())
/* <!-- #BLAZE_RULE($base_py).ATTRIBUTE(imports) -->
List of import directories to be added to the <code>PYTHONPATH</code>.
<p>
Expand Down
132 changes: 60 additions & 72 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,6 @@ public PyCommon(
this.hasPy3OnlySources = initHasPy3OnlySources(ruleContext, this.sourcesVersion);
this.runtimeFromToolchain = initRuntimeFromToolchain(ruleContext, this.version);
validatePythonVersionAttr();
validateLegacyProviderNotUsedIfDisabled();
}

/** Returns the parsed value of the "srcs_version" attribute. */
Expand Down Expand Up @@ -244,17 +243,28 @@ private static NestedSet<Artifact> initTransitivePythonSources(RuleContext ruleC
/**
* Gathers transitive .py files from {@code deps} (not including this target's {@code srcs} and
* adds them to {@code builder}.
*
* <p>If a target has the PyInfo provider, the value from that provider is used. Otherwise, we
* fall back on collecting .py source files from the target's filesToBuild.
*/
// TODO(bazel-team): Eliminate the fallback behavior by returning an appropriate py provider from
// the relevant rules.
private static void collectTransitivePythonSourcesFromDeps(
RuleContext ruleContext, NestedSetBuilder<Artifact> builder) {
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps")) {
try {
builder.addTransitive(PyProviderUtils.getTransitiveSources(dep));
} catch (EvalException e) {
// Either the provider type or field type is bad.
ruleContext.attributeError(
"deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage()));
NestedSet<Artifact> sources;
if (dep.get(PyInfo.PROVIDER) != null) {
sources = dep.get(PyInfo.PROVIDER).getTransitiveSourcesSet();
} else {
sources =
NestedSetBuilder.<Artifact>compileOrder()
.addAll(
FileType.filter(
dep.getProvider(FileProvider.class).getFilesToBuild().toList(),
PyRuleClasses.PYTHON_SOURCE))
.build();
}
builder.addTransitive(sources);
}
}

Expand Down Expand Up @@ -308,34 +318,42 @@ private static boolean initUsesSharedLibraries(RuleContext ruleContext) {
targets = ruleContext.getPrerequisites("deps");
}
for (TransitiveInfoCollection target : targets) {
try {
if (PyProviderUtils.getUsesSharedLibraries(target)) {
if (target.get(PyInfo.PROVIDER) != null) {
if (target.get(PyInfo.PROVIDER).getUsesSharedLibraries()) {
return true;
}
} catch (EvalException e) {
ruleContext.ruleError(String.format("In dep '%s': %s", target.getLabel(), e.getMessage()));
} else if (FileType.contains(
target.getProvider(FileProvider.class).getFilesToBuild().toList(),
CppFileTypes.SHARED_LIBRARY)) {
return true;
}
}
return false;
}

/**
* Returns the transitive import paths of a target.
*
* <p>For targets with the PyInfo provider, the value from that provider is used. Otherwise, we
* default to an empty set.
*/
private static NestedSet<String> initImports(RuleContext ruleContext, PythonSemantics semantics) {
NestedSetBuilder<String> builder = NestedSetBuilder.compileOrder();
builder.addAll(semantics.getImports(ruleContext));
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps")) {
try {
NestedSet<String> imports = PyProviderUtils.getImports(dep);
if (!builder.getOrder().isCompatible(imports.getOrder())) {
// TODO(brandjon): We should make order an invariant of the Python provider, and move this
// check into PyInfo/PyStructUtils.
ruleContext.ruleError(
getOrderErrorMessage(PyStructUtils.IMPORTS, builder.getOrder(), imports.getOrder()));
} else {
builder.addTransitive(imports);
}
} catch (EvalException e) {
ruleContext.attributeError(
"deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage()));
NestedSet<String> imports;
if (dep.get(PyInfo.PROVIDER) != null) {
imports = dep.get(PyInfo.PROVIDER).getImportsSet();
} else {
imports = NestedSetBuilder.emptySet(Order.COMPILE_ORDER);
}
if (!builder.getOrder().isCompatible(imports.getOrder())) {
// TODO(brandjon): We should make order an invariant of the Python provider, and move this
// check into PyInfo.
ruleContext.ruleError(
getOrderErrorMessage("imports", builder.getOrder(), imports.getOrder()));
} else {
builder.addTransitive(imports);
}
}
return builder.build();
Expand All @@ -350,14 +368,9 @@ private static boolean initHasPy2OnlySources(
if (sourcesVersion == PythonVersion.PY2 || sourcesVersion == PythonVersion.PY2ONLY) {
return true;
}
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps")) {
try {
if (PyProviderUtils.getHasPy2OnlySources(dep)) {
return true;
}
} catch (EvalException e) {
ruleContext.attributeError(
"deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage()));
for (PyInfo depInfo : ruleContext.getPrerequisites("deps", PyInfo.PROVIDER)) {
if (depInfo.getHasPy2OnlySources()) {
return true;
}
}
return false;
Expand All @@ -372,14 +385,9 @@ private static boolean initHasPy3OnlySources(
if (sourcesVersion == PythonVersion.PY3 || sourcesVersion == PythonVersion.PY3ONLY) {
return true;
}
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps")) {
try {
if (PyProviderUtils.getHasPy3OnlySources(dep)) {
return true;
}
} catch (EvalException e) {
ruleContext.attributeError(
"deps", String.format("In dep '%s': %s", dep.getLabel(), e.getMessage()));
for (PyInfo depInfo : ruleContext.getPrerequisites("deps", PyInfo.PROVIDER)) {
if (depInfo.getHasPy3OnlySources()) {
return true;
}
}
return false;
Expand Down Expand Up @@ -523,27 +531,6 @@ private void validatePythonVersionAttr() {
}
}

/**
* 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")) {
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()));
}
}
}

/**
* If the Python version (as determined by the configuration) is inconsistent with {@link
* #hasPy2OnlySources} or {@link #hasPy3OnlySources}, emits a {@link FailAction} that "generates"
Expand Down Expand Up @@ -752,7 +739,9 @@ public void initBinary(List<Artifact> srcs) {
addPyExtraActionPseudoAction();
}

/** @return an artifact next to the executable file with a given suffix. */
/**
* @return an artifact next to the executable file with a given suffix.
*/
private Artifact getArtifactWithExtension(Artifact executable, String extension) {
// On Windows, the Python executable has .exe extension on Windows,
// On Linux, the Python executable has no extension.
Expand Down Expand Up @@ -793,21 +782,20 @@ public Artifact getPythonStubArtifactForWindows(Artifact executable) {
}

/**
* Adds a PyInfo or legacy "py" provider.
* Adds a PyInfo provider.
*
* <p>This is a public method because some rules just want a PyInfo provider without the other
* things py_library needs.
*/
public void addPyInfoProvider(RuleConfiguredTargetBuilder builder) {
boolean createLegacyPyProvider =
!ruleContext.getFragment(PythonConfiguration.class).disallowLegacyPyProvider();
PyProviderUtils.builder(createLegacyPyProvider)
.setTransitiveSources(transitivePythonSources)
.setUsesSharedLibraries(usesSharedLibraries)
.setImports(imports)
.setHasPy2OnlySources(hasPy2OnlySources)
.setHasPy3OnlySources(hasPy3OnlySources)
.buildAndAddToTarget(builder);
builder.addNativeDeclaredProvider(
PyInfo.builder()
.setTransitiveSources(transitivePythonSources)
.setUsesSharedLibraries(usesSharedLibraries)
.setImports(imports)
.setHasPy2OnlySources(hasPy2OnlySources)
.setHasPy3OnlySources(hasPy3OnlySources)
.build());
}

public void addCommonTransitiveInfoProviders(
Expand Down
Loading

0 comments on commit f068b31

Please sign in to comment.