From 3c26f9eab8825754ec83ab65029fd8d3cce3bdce Mon Sep 17 00:00:00 2001 From: brandjon Date: Tue, 15 Jan 2019 10:59:28 -0800 Subject: [PATCH] Refactor PyProvider helper utilities PyProvider manages how Java code creates and accesses the fields of the legacy "py" struct provider (which we'll migrate away from soonish). This CL refactors default value logic into PyProvider, and adds a builder so it's easier to construct. It also slightly refactors some shared-library logic in PyCommon. More cleanup (hopefully) to come. Work toward #7010, yak shaving for #6583 and #1446. RELNOTES: None PiperOrigin-RevId: 229401739 --- .../build/lib/rules/python/PyCommon.java | 84 ++++++--- .../build/lib/rules/python/PyProvider.java | 94 +++++++--- .../lib/rules/python/PyProviderTest.java | 168 +++++++++++++----- 3 files changed, 249 insertions(+), 97 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 c5feaffc81fcbc..554214dcac74e7 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 @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.rules.cpp.CppFileTypes; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.syntax.EvalException; -import com.google.devtools.build.lib.syntax.SkylarkType; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.OS; @@ -79,8 +78,22 @@ public void collectMetadataArtifacts(Iterable artifacts, private final NestedSet transitivePythonSources; - private final PythonVersion sourcesVersion; + 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. + * + *

This is always either {@code PY2} or {@code PY3}. + */ private final PythonVersion version; + + /** + * The level of compatibility with Python major versions, as per the {@code srcs_version} + * attribute. + */ + private final PythonVersion sourcesVersion; + private Map convertedFiles; private NestedSet filesToBuild = null; @@ -90,6 +103,7 @@ public PyCommon(RuleContext ruleContext) { this.sourcesVersion = getSrcsVersionAttr(ruleContext); this.version = ruleContext.getFragment(PythonConfiguration.class).getPythonVersion(); this.transitivePythonSources = collectTransitivePythonSources(); + this.usesSharedLibraries = checkForSharedLibraries(); checkSourceIsCompatible(this.version, this.sourcesVersion, ruleContext.getLabel()); validatePythonVersionAttr(ruleContext); } @@ -155,7 +169,11 @@ public void addCommonTransitiveInfoProviders( /* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER))) .addSkylarkTransitiveInfo( PyProvider.PROVIDER_NAME, - PyProvider.create(this.transitivePythonSources, usesSharedLibraries(), imports)) + PyProvider.builder() + .setTransitiveSources(transitivePythonSources) + .setUsesSharedLibraries(usesSharedLibraries) + .setImports(imports) + .build()) // Python targets are not really compilable. The best we can do is make sure that all // generated source files are ready. .addOutputGroup(OutputGroupInfo.FILES_TO_COMPILE, transitivePythonSources) @@ -454,40 +472,50 @@ public NestedSet getFilesToBuild() { return filesToBuild; } - public boolean usesSharedLibraries() { - try { - return checkForSharedLibraries(Iterables.concat( + /** + * Returns true if any of this target's {@code deps} or {@code data} deps has a shared library + * file (e.g. a {@code .so}) in its transitive dependency closure. + * + *

For targets with the py provider, we consult the {@code uses_shared_libraries} field. For + * targets without this provider, we look for {@link CppFileTypes#SHARED_LIBRARY}-type files in + * the filesToBuild. + */ + private boolean checkForSharedLibraries() { + Iterable targets; + // For all rule types that use PyCommon, deps is a required attribute but not data. + if (ruleContext.attributes().has("data")) { + targets = + Iterables.concat( ruleContext.getPrerequisites("deps", Mode.TARGET), - ruleContext.getPrerequisites("data", Mode.DONT_CHECK))); + ruleContext.getPrerequisites("data", Mode.DONT_CHECK)); + } else { + targets = ruleContext.getPrerequisites("deps", Mode.TARGET); + } + try { + for (TransitiveInfoCollection target : targets) { + if (checkForSharedLibraries(target)) { + return true; + } + } + return false; } catch (EvalException e) { ruleContext.ruleError(e.getMessage()); return false; } } - - /** - * Returns true if this target has an .so file in its transitive dependency closure. - */ - public static boolean checkForSharedLibraries(Iterable deps) - throws EvalException{ - for (TransitiveInfoCollection dep : deps) { - Object providerObject = dep.get(PyProvider.PROVIDER_NAME); - if (providerObject != null) { - SkylarkType.checkType(providerObject, StructImpl.class, null); - StructImpl provider = (StructImpl) providerObject; - Boolean isUsingSharedLibrary = - provider.getValue(PyProvider.USES_SHARED_LIBRARIES, Boolean.class); - if (Boolean.TRUE.equals(isUsingSharedLibrary)) { - return true; - } - } else if (FileType.contains( - dep.getProvider(FileProvider.class).getFilesToBuild(), CppFileTypes.SHARED_LIBRARY)) { - return true; - } + private static boolean checkForSharedLibraries(TransitiveInfoCollection target) + throws EvalException { + if (PyProvider.hasProvider(target)) { + return PyProvider.getUsesSharedLibraries(PyProvider.getProvider(target)); + } else { + NestedSet files = target.getProvider(FileProvider.class).getFilesToBuild(); + return FileType.contains(files, CppFileTypes.SHARED_LIBRARY); } + } - return false; + public boolean usesSharedLibraries() { + return usesSharedLibraries; } private static String buildMultipleMainMatchesErrorText(boolean explicit, String proposedMainName, diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java index 75484ff2900b4e..fb6e3f609e15f5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyProvider.java @@ -14,10 +14,12 @@ package com.google.devtools.build.lib.rules.python; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.packages.StructImpl; import com.google.devtools.build.lib.packages.StructProvider; import com.google.devtools.build.lib.syntax.EvalException; @@ -28,7 +30,7 @@ /** * Static helper class for managing the "py" struct provider returned and consumed by Python rules. */ -// TODO(brandjon): Replace this with a real provider. +// TODO(#7010): Replace this with a real provider. public class PyProvider { // Disable construction. @@ -38,8 +40,8 @@ private PyProvider() {} public static final String PROVIDER_NAME = "py"; /** - * Name of field holding a depset of transitive sources (i.e., .py files in srcs and in srcs of - * transitive deps). + * Name of field holding a postorder depset of transitive sources (i.e., .py files in srcs and in + * srcs of transitive deps). */ public static final String TRANSITIVE_SOURCES = "transitive_sources"; @@ -52,22 +54,22 @@ private PyProvider() {} * Name of field holding a depset of import paths added by the transitive deps (including this * target). */ + // TODO(brandjon): Make this a pre-order depset, since higher-level targets should get precedence + // on PYTHONPATH. + // TODO(brandjon): Add assertions that this depset and transitive_sources have an order compatible + // with the one expected by the rules. public static final String IMPORTS = "imports"; - /** Constructs a provider instance with the given field values. */ - public static StructImpl create( - NestedSet transitiveSources, - boolean usesSharedLibraries, - NestedSet imports) { - return StructProvider.STRUCT.create( - ImmutableMap.of( - PyProvider.TRANSITIVE_SOURCES, - SkylarkNestedSet.of(Artifact.class, transitiveSources), - PyProvider.USES_SHARED_LIBRARIES, - usesSharedLibraries, - PyProvider.IMPORTS, - SkylarkNestedSet.of(String.class, imports)), - "No such attribute '%s'"); + private static final ImmutableMap DEFAULTS; + + static { + ImmutableMap.Builder builder = ImmutableMap.builder(); + // TRANSITIVE_SOURCES is mandatory + builder.put(USES_SHARED_LIBRARIES, false); + builder.put( + IMPORTS, + SkylarkNestedSet.of(String.class, NestedSetBuilder.compileOrder().build())); + DEFAULTS = builder.build(); } /** Returns whether a given dependency has the py provider. */ @@ -92,8 +94,11 @@ public static StructImpl getProvider(TransitiveInfoCollection dep) throws EvalEx private static Object getValue(StructImpl info, String fieldName) throws EvalException { Object fieldValue = info.getValue(fieldName); if (fieldValue == null) { - throw new EvalException( - /*location=*/ null, String.format("'py' provider missing '%s' field", fieldName)); + fieldValue = DEFAULTS.get(fieldName); + if (fieldValue == null) { + throw new EvalException( + /*location=*/ null, String.format("'py' provider missing '%s' field", fieldName)); + } } return fieldValue; } @@ -119,9 +124,9 @@ public static NestedSet getTransitiveSources(StructImpl info) throws E } /** - * Casts and returns the uses-shared-libraries field. + * Casts and returns the uses-shared-libraries field (or its default value). * - * @throws EvalException if the field does not exist or is not a boolean + * @throws EvalException if the field exists and is not a boolean */ public static boolean getUsesSharedLibraries(StructImpl info) throws EvalException { Object fieldValue = getValue(info, USES_SHARED_LIBRARIES); @@ -136,9 +141,9 @@ public static boolean getUsesSharedLibraries(StructImpl info) throws EvalExcepti } /** - * Casts and returns the imports field. + * Casts and returns the imports field (or its default value). * - * @throws EvalException if the field does not exist or is not a depset of strings + * @throws EvalException if the field exists and is not a depset of strings */ public static NestedSet getImports(StructImpl info) throws EvalException { Object fieldValue = getValue(info, IMPORTS); @@ -154,4 +159,47 @@ public static NestedSet getImports(StructImpl info) throws EvalException EvalUtils.getDataTypeNameFromClass(fieldValue.getClass())); return castValue.getSet(String.class); } + + public static Builder builder() { + return new Builder(); + } + + /** Builder for a py provider struct. */ + public static class Builder { + SkylarkNestedSet transitiveSources = null; + Boolean usesSharedLibraries = null; + SkylarkNestedSet imports = null; + + // Use the static builder() method instead. + private Builder() {} + + public Builder setTransitiveSources(NestedSet transitiveSources) { + this.transitiveSources = SkylarkNestedSet.of(Artifact.class, transitiveSources); + return this; + } + + public Builder setUsesSharedLibraries(boolean usesSharedLibraries) { + this.usesSharedLibraries = usesSharedLibraries; + return this; + } + + public Builder setImports(NestedSet imports) { + this.imports = SkylarkNestedSet.of(String.class, imports); + return this; + } + + private static void put( + ImmutableMap.Builder fields, String fieldName, Object value) { + fields.put(fieldName, value != null ? value : DEFAULTS.get(fieldName)); + } + + public StructImpl build() { + ImmutableMap.Builder fields = ImmutableMap.builder(); + Preconditions.checkNotNull(transitiveSources, "setTransitiveSources is required"); + put(fields, TRANSITIVE_SOURCES, transitiveSources); + put(fields, USES_SHARED_LIBRARIES, usesSharedLibraries); + put(fields, IMPORTS, imports); + return StructProvider.STRUCT.create(fields.build(), "No such attribute '%s'"); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java index 7af272b6c1414a..d91a657fed8505 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyProviderTest.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.packages.StructImpl; @@ -28,6 +29,8 @@ import com.google.devtools.build.lib.syntax.SkylarkNestedSet; import com.google.devtools.build.lib.testutil.MoreAsserts.ThrowingRunnable; import java.io.IOException; +import java.util.LinkedHashMap; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -36,32 +39,26 @@ @RunWith(JUnit4.class) public class PyProviderTest extends BuildViewTestCase { - private static final StructImpl EMPTY_STRUCT = - StructProvider.STRUCT.create(ImmutableMap.of(), "No such attribute '%s'"); - - private static final StructImpl WRONG_TYPES_STRUCT = - StructProvider.STRUCT.create( - ImmutableMap.of( - PyProvider.TRANSITIVE_SOURCES, - 123, - PyProvider.USES_SHARED_LIBRARIES, - 123, - PyProvider.IMPORTS, - 123), - "No such attribute '%s'"); - - private StructImpl getGoodStruct() { - return StructProvider.STRUCT.create( - ImmutableMap.of( - PyProvider.TRANSITIVE_SOURCES, - SkylarkNestedSet.of( - Artifact.class, - NestedSetBuilder.create(Order.STABLE_ORDER, getSourceArtifact("dummy_artifact"))), - PyProvider.USES_SHARED_LIBRARIES, - true, - PyProvider.IMPORTS, - SkylarkNestedSet.of(String.class, NestedSetBuilder.create(Order.STABLE_ORDER, "abc"))), - "No such attribute '%s'"); + /** + * Constructs a py provider struct with the given field values and with default values for any + * field not specified. + * + *

The struct is constructed directly, rather than using {@link PyProvider.Builder}, so that + * the resulting instance is suitable for asserting on {@code PyProvider}'s operations over + * structs with known contents. {@code overrides} is applied directly without validating the + * fields' names or types. + */ + private StructImpl makeStruct(Map overrides) { + Map fields = new LinkedHashMap<>(); + fields.put( + PyProvider.TRANSITIVE_SOURCES, + SkylarkNestedSet.of(Artifact.class, NestedSetBuilder.emptySet(Order.COMPILE_ORDER))); + fields.put(PyProvider.USES_SHARED_LIBRARIES, false); + fields.put( + PyProvider.IMPORTS, + SkylarkNestedSet.of(String.class, NestedSetBuilder.emptySet(Order.COMPILE_ORDER))); + fields.putAll(overrides); + return StructProvider.STRUCT.create(fields, "No such attribute '%s'"); } /** Defines //pytarget, a target that returns a py provider with some arbitrary field values. */ @@ -192,16 +189,35 @@ private static void assertHasWrongTypeMessage( "\'py' provider's '%s' field should be a %s (got a 'int')", fieldName, expectedType)); } + /** We need this because {@code NestedSet}s don't have value equality. */ + private static void assertHasOrderAndContainsExactly( + NestedSet set, Order order, Object... values) { + assertThat(set.getOrder()).isEqualTo(order); + assertThat(set).containsExactly(values); + } + @Test - public void getTransitiveSources() throws Exception { - assertHasMissingFieldMessage( - () -> PyProvider.getTransitiveSources(EMPTY_STRUCT), "transitive_sources"); + public void getTransitiveSources_Good() throws Exception { + NestedSet sources = + NestedSetBuilder.create(Order.COMPILE_ORDER, getSourceArtifact("dummy")); + StructImpl info = + makeStruct( + ImmutableMap.of( + PyProvider.TRANSITIVE_SOURCES, SkylarkNestedSet.of(Artifact.class, sources))); + assertThat(PyProvider.getTransitiveSources(info)).isSameAs(sources); + } + + @Test + public void getTransitiveSources_Missing() { + StructImpl info = StructProvider.STRUCT.createEmpty(null); + assertHasMissingFieldMessage(() -> PyProvider.getTransitiveSources(info), "transitive_sources"); + } + + @Test + public void getTransitiveSources_WrongType() { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.TRANSITIVE_SOURCES, 123)); assertHasWrongTypeMessage( - () -> PyProvider.getTransitiveSources(WRONG_TYPES_STRUCT), - "transitive_sources", - "depset of Files"); - assertThat(PyProvider.getTransitiveSources(getGoodStruct())) - .containsExactly(getSourceArtifact("dummy_artifact")); + () -> PyProvider.getTransitiveSources(info), "transitive_sources", "depset of Files"); } @Test @@ -242,21 +258,81 @@ public void getTransitiveSources_OrderMismatch() throws Exception { } @Test - public void getUsesSharedLibraries() throws Exception { - assertHasMissingFieldMessage( - () -> PyProvider.getUsesSharedLibraries(EMPTY_STRUCT), "uses_shared_libraries"); - assertHasWrongTypeMessage( - () -> PyProvider.getUsesSharedLibraries(WRONG_TYPES_STRUCT), - "uses_shared_libraries", - "boolean"); - assertThat(PyProvider.getUsesSharedLibraries(getGoodStruct())).isTrue(); + public void getUsesSharedLibraries_Good() throws Exception { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.USES_SHARED_LIBRARIES, true)); + assertThat(PyProvider.getUsesSharedLibraries(info)).isTrue(); } @Test - public void getImports() throws Exception { - assertHasMissingFieldMessage(() -> PyProvider.getImports(EMPTY_STRUCT), "imports"); + public void getUsesSharedLibraries_Missing() throws Exception { + StructImpl info = StructProvider.STRUCT.createEmpty(null); + assertThat(PyProvider.getUsesSharedLibraries(info)).isFalse(); + } + + @Test + public void getUsesSharedLibraries_WrongType() { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.USES_SHARED_LIBRARIES, 123)); assertHasWrongTypeMessage( - () -> PyProvider.getImports(WRONG_TYPES_STRUCT), "imports", "depset of strings"); - assertThat(PyProvider.getImports(getGoodStruct())).containsExactly("abc"); + () -> PyProvider.getUsesSharedLibraries(info), "uses_shared_libraries", "boolean"); + } + + @Test + public void getImports_Good() throws Exception { + NestedSet imports = NestedSetBuilder.create(Order.COMPILE_ORDER, "abc"); + StructImpl info = + makeStruct(ImmutableMap.of(PyProvider.IMPORTS, SkylarkNestedSet.of(String.class, imports))); + assertThat(PyProvider.getImports(info)).isSameAs(imports); + } + + @Test + public void getImports_Missing() throws Exception { + StructImpl info = StructProvider.STRUCT.createEmpty(null); + assertHasOrderAndContainsExactly(PyProvider.getImports(info), Order.COMPILE_ORDER); + } + + @Test + public void getImports_WrongType() { + StructImpl info = makeStruct(ImmutableMap.of(PyProvider.IMPORTS, 123)); + assertHasWrongTypeMessage(() -> PyProvider.getImports(info), "imports", "depset of strings"); + } + + /** Checks values set by the builder. */ + @Test + public void builder() throws Exception { + NestedSet sources = + NestedSetBuilder.create(Order.COMPILE_ORDER, getSourceArtifact("dummy")); + NestedSet imports = NestedSetBuilder.create(Order.COMPILE_ORDER, "abc"); + StructImpl info = + PyProvider.builder() + .setTransitiveSources(sources) + .setUsesSharedLibraries(true) + .setImports(imports) + .build(); + // Assert using struct operations, not PyProvider accessors, which aren't necessarily trusted to + // be correct. + assertHasOrderAndContainsExactly( + ((SkylarkNestedSet) info.getValue(PyProvider.TRANSITIVE_SOURCES)).getSet(Artifact.class), + Order.COMPILE_ORDER, + getSourceArtifact("dummy")); + assertThat((Boolean) info.getValue(PyProvider.USES_SHARED_LIBRARIES)).isTrue(); + assertHasOrderAndContainsExactly( + ((SkylarkNestedSet) info.getValue(PyProvider.IMPORTS)).getSet(String.class), + Order.COMPILE_ORDER, + "abc"); + } + + /** Checks the defaults set by the builder. */ + @Test + public void builderDefaults() throws Exception { + // transitive_sources is mandatory, so create a dummy value but no need to assert on it. + NestedSet sources = + NestedSetBuilder.create(Order.COMPILE_ORDER, getSourceArtifact("dummy")); + StructImpl info = PyProvider.builder().setTransitiveSources(sources).build(); + // Assert using struct operations, not PyProvider accessors, which aren't necessarily trusted to + // be correct. + assertThat((Boolean) info.getValue(PyProvider.USES_SHARED_LIBRARIES)).isFalse(); + assertHasOrderAndContainsExactly( + ((SkylarkNestedSet) info.getValue(PyProvider.IMPORTS)).getSet(String.class), + Order.COMPILE_ORDER); } }