From d1f482ddd7085d90b9bd0ee10bd8ecb756f9e9af Mon Sep 17 00:00:00 2001 From: brandjon Date: Tue, 15 Jan 2019 11:42:33 -0800 Subject: [PATCH] Refactor how PyCommon manages its transitive sources Simplify computing the transitive sources and transitive-without-local sources by factoring a method or two and separating out the computation of `convertedFiles`. Work toward #7010, yak shaving for #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229410930 --- .../build/lib/rules/python/PyCommon.java | 105 +++++++++++------- 1 file changed, 67 insertions(+), 38 deletions(-) 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 554214dcac74e7..9ac96a195cb006 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 @@ -56,9 +56,7 @@ import java.util.Map; import java.util.UUID; -/** - * A helper class for Python rules. - */ +/** A helper class for analyzing a Python configured target. */ public final class PyCommon { public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version"; @@ -74,12 +72,6 @@ public void collectMetadataArtifacts(Iterable artifacts, private final RuleContext ruleContext; - private Artifact executable = null; - - private final NestedSet transitivePythonSources; - - private final boolean usesSharedLibraries; - /** * The Python major version for which this target is being built, as per the {@code * python_version} attribute or the configuration. @@ -94,7 +86,29 @@ public void collectMetadataArtifacts(Iterable artifacts, */ private final PythonVersion sourcesVersion; - private Map convertedFiles; + /** + * The Python sources belonging to this target's transitive {@code deps}, not including this + * target's own {@code srcs}. + */ + private final NestedSet dependencyTransitivePythonSources; + + /** + * The Python sources belonging to this target's transitive {@code deps}, including the Python + * sources in this target's {@code srcs}. + */ + private final NestedSet transitivePythonSources; + + /** Whether this target or any of its {@code deps} or {@code data} deps has a shared library. */ + private final boolean usesSharedLibraries; + + /** + * Symlink map from root-relative paths to 2to3 converted source artifacts. + * + *

Null if no 2to3 conversion is required. + */ + private final Map convertedFiles; + + private Artifact executable = null; private NestedSet filesToBuild = null; @@ -102,8 +116,10 @@ public PyCommon(RuleContext ruleContext) { this.ruleContext = ruleContext; this.sourcesVersion = getSrcsVersionAttr(ruleContext); this.version = ruleContext.getFragment(PythonConfiguration.class).getPythonVersion(); + this.dependencyTransitivePythonSources = collectDependencyTransitivePythonSources(); this.transitivePythonSources = collectTransitivePythonSources(); this.usesSharedLibraries = checkForSharedLibraries(); + this.convertedFiles = maybeMakeConvertedFiles(); checkSourceIsCompatible(this.version, this.sourcesVersion, ruleContext.getLabel()); validatePythonVersionAttr(ruleContext); } @@ -252,28 +268,21 @@ void validatePackageName() { } /** - * Adds a {@link PseudoAction} to the build graph that is only used - * for providing information to the blaze extra_action feature. + * Adds a {@link PseudoAction} to the build graph that is only used for providing information to + * the blaze extra_action feature. */ void addPyExtraActionPseudoAction() { if (ruleContext.getConfiguration().getActionListeners().isEmpty()) { return; } - - // We need to do it in this convoluted way because we must not add the files declared in the - // srcs of this rule. Note that it is not enough to remove the direct members from the nested - // set of the current rule, because the same files may have been declared in a dependency, too. - NestedSetBuilder depBuilder = NestedSetBuilder.compileOrder(); - collectTransitivePythonSourcesFrom(getTargetDeps(), depBuilder); - NestedSet dependencies = depBuilder.build(); - ruleContext.registerAction( makePyExtraActionPseudoAction( ruleContext.getActionOwner(), // Has to be unfiltered sources as filtered will give an error for // unsupported file types where as certain tests only expect a warning. ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list(), - dependencies, + // We must not add the files declared in the srcs of this rule.; + dependencyTransitivePythonSources, PseudoAction.getDummyOutput(ruleContext))); } @@ -305,16 +314,23 @@ public static Action makePyExtraActionPseudoAction( @AutoCodec @AutoCodec.VisibleForSerialization static final GeneratedExtension PYTHON_INFO = PythonInfo.pythonInfo; - private void addSourceFiles(NestedSetBuilder builder, Iterable artifacts) { - Preconditions.checkState(convertedFiles == null); + /** + * If 2to3 conversion is to be done, creates the 2to3 actions and returns the map of converted + * files; otherwise returns null. + */ + // TODO(#1393): 2to3 conversion doesn't work in Bazel and the attempt to invoke it for Bazel + // should be removed / factored away into PythonSemantics. + private Map maybeMakeConvertedFiles() { if (sourcesVersion == PythonVersion.PY2 && version == PythonVersion.PY3) { - convertedFiles = PythonUtils.generate2to3Actions(ruleContext, artifacts); + Iterable artifacts = + ruleContext + .getPrerequisiteArtifacts("srcs", Mode.TARGET) + .filter(PyRuleClasses.PYTHON_SOURCE) + .list(); + return PythonUtils.generate2to3Actions(ruleContext, artifacts); + } else { + return null; } - builder.addAll(artifacts); - } - - private Iterable getTargetDeps() { - return ruleContext.getPrerequisites("deps", Mode.TARGET); } private static String getOrderErrorMessage(String fieldName, Order expected, Order actual) { @@ -352,21 +368,34 @@ private void collectTransitivePythonSourcesFrom( private NestedSet collectTransitivePythonSources() { NestedSetBuilder builder = NestedSetBuilder.compileOrder(); - collectTransitivePythonSourcesFrom(getTargetDeps(), builder); - addSourceFiles(builder, - ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET) - .filter(PyRuleClasses.PYTHON_SOURCE).list()); + collectTransitivePythonSourcesFrom(ruleContext.getPrerequisites("deps", Mode.TARGET), builder); + builder.addAll( + ruleContext + .getPrerequisiteArtifacts("srcs", Mode.TARGET) + .filter(PyRuleClasses.PYTHON_SOURCE) + .list()); return builder.build(); } - // TODO(brandjon): Move provider merging logic into PyProvider. Have rule implementations read - // the sources off a merged provider of deps (with/without the local rule included in the merge). - public NestedSet collectTransitivePythonSourcesWithoutLocal() { + private NestedSet collectDependencyTransitivePythonSources() { NestedSetBuilder builder = NestedSetBuilder.compileOrder(); - collectTransitivePythonSourcesFrom(getTargetDeps(), builder); + collectTransitivePythonSourcesFrom(ruleContext.getPrerequisites("deps", Mode.TARGET), builder); return builder.build(); } + /** Returns the transitive Python sources collected from the deps and srcs attributes. */ + public NestedSet getTransitivePythonSources() { + return transitivePythonSources; + } + + /** + * Returns the transitive Python sources collected from the deps attribute, not including sources + * from the srcs attribute (unless they were separately reached via deps). + */ + public NestedSet getDependencyTransitivePythonSources() { + return dependencyTransitivePythonSources; + } + public NestedSet collectImports(RuleContext ruleContext, PythonSemantics semantics) { NestedSetBuilder builder = NestedSetBuilder.compileOrder(); builder.addAll(semantics.getImports(ruleContext)); @@ -375,7 +404,7 @@ public NestedSet collectImports(RuleContext ruleContext, PythonSemantics } private void collectTransitivePythonImports(NestedSetBuilder builder) { - for (TransitiveInfoCollection dep : getTargetDeps()) { + for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps", Mode.TARGET)) { if (dep.getProvider(PythonImportsProvider.class) != null) { PythonImportsProvider provider = dep.getProvider(PythonImportsProvider.class); NestedSet imports = provider.getTransitivePythonImports();