Skip to content

Commit

Permalink
Refactor "py" provider implementation as an ADT
Browse files Browse the repository at this point in the history
The py provider is currently just a struct manipulated by native code. Eventually it should be a full-fledged Provider, but in the meantime this moves its accessors to a separate utility class.

This is preparation for adding new provider fields to address #6583 and #1446. The cleanup is also a small step toward #7010.

RELNOTES: None
PiperOrigin-RevId: 227771462
  • Loading branch information
brandjon authored and Copybara-Service committed Jan 4, 2019
1 parent c2cd16d commit 455caab
Show file tree
Hide file tree
Showing 9 changed files with 507 additions and 102 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
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.PyProvider;
import com.google.devtools.build.lib.rules.python.PyRuleClasses;
import com.google.devtools.build.lib.rules.python.PythonVersion;
import com.google.devtools.build.lib.util.FileType;
Expand Down Expand Up @@ -69,7 +70,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.override(
builder
.copy("deps")
.legacyMandatoryProviders(PyCommon.PYTHON_SKYLARK_PROVIDER_NAME)
.legacyMandatoryProviders(PyProvider.PROVIDER_NAME)
.allowedFileTypes())
/* <!-- #BLAZE_RULE($base_py).ATTRIBUTE(imports) -->
List of import directories to be added to the <code>PYTHONPATH</code>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ public NestedSetBuilder(Order order) {
this.order = order;
}

/**
* Returns the order used by this builder.
*
* <p>This is useful for testing for incompatibilities (via {@link Order#isCompatible}) without
* catching an unchecked exception from {@link #addTransitive}.
*/
public Order getOrder() {
return order;
}

/** Returns whether the set to be built is empty. */
public boolean isEmpty() {
return items.isEmpty() && transitiveSets.isEmpty();
Expand Down Expand Up @@ -93,8 +103,8 @@ public NestedSetBuilder<E> addAll(NestedSet<? extends E> elements) {
*
* <p>The relative left-to-right order of transitive members is preserved from the sequence of
* calls to {@link #addTransitive}. Since the traversal {@link Order} controls whether direct
* members appear before or after transitive ones, the interleaving of
* {@link #add}/{@link #addAll} with {@link #addTransitive} does not matter.
* members appear before or after transitive ones, the interleaving of {@link #add}/{@link
* #addAll} with {@link #addTransitive} does not matter.
*
* <p>The {@link Order} of the added set must be compatible with the order of this builder (see
* {@link Order#isCompatible}). This is true even if the added set is empty. Strictly speaking, it
Expand All @@ -109,7 +119,7 @@ public NestedSetBuilder<E> addAll(NestedSet<? extends E> elements) {
*
* @param subset the set to add as a transitive member; must not be null
* @return the builder
* @throws IllegalStateException if the order of {@code subset} is not compatible with the
* @throws IllegalArgumentException if the order of {@code subset} is not compatible with the
* order of this builder
*/
public NestedSetBuilder<E> addTransitive(NestedSet<? extends E> subset) {
Expand Down
106 changes: 36 additions & 70 deletions src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionOwner;
Expand All @@ -43,12 +42,9 @@
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
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.EvalUtils;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.FileType;
Expand All @@ -66,11 +62,6 @@
*/
public final class PyCommon {

public static final String PYTHON_SKYLARK_PROVIDER_NAME = "py";
public static final String TRANSITIVE_PYTHON_SRCS = "transitive_sources";
public static final String IS_USING_SHARED_LIBRARY = "uses_shared_libraries";
public static final String IMPORTS = "imports";

public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version";
public static final String PYTHON_VERSION_ATTRIBUTE = "python_version";

Expand Down Expand Up @@ -163,34 +154,14 @@ public void addCommonTransitiveInfoProviders(
filesToBuild,
/* reportedToActualSources= */ NestedSetBuilder.create(Order.STABLE_ORDER)))
.addSkylarkTransitiveInfo(
PYTHON_SKYLARK_PROVIDER_NAME,
createSourceProvider(this.transitivePythonSources, usesSharedLibraries(), imports))
PyProvider.PROVIDER_NAME,
PyProvider.create(this.transitivePythonSources, usesSharedLibraries(), imports))
// 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)
.addOutputGroup(OutputGroupInfo.COMPILATION_PREREQUISITES, transitivePythonSources);
}

/**
* Returns a Skylark struct for exposing transitive Python sources:
*
* <p>addSkylarkTransitiveInfo(PYTHON_SKYLARK_PROVIDER_NAME, createSourceProvider(...))
*/
public static StructImpl createSourceProvider(
NestedSet<Artifact> transitivePythonSources,
boolean isUsingSharedLibrary,
NestedSet<String> imports) {
return StructProvider.STRUCT.create(
ImmutableMap.<String, Object>of(
TRANSITIVE_PYTHON_SRCS,
SkylarkNestedSet.of(Artifact.class, transitivePythonSources),
IS_USING_SHARED_LIBRARY,
isUsingSharedLibrary,
IMPORTS,
SkylarkNestedSet.of(String.class, imports)),
"No such attribute '%s'");
}

/** Returns the parsed value of the "srcs_version" attribute. */
private static PythonVersion getSrcsVersionAttr(RuleContext ruleContext) {
String attrValue = ruleContext.attributes().get("srcs_version", Type.STRING);
Expand Down Expand Up @@ -331,49 +302,33 @@ private Iterable<? extends TransitiveInfoCollection> getTargetDeps() {
return ruleContext.getPrerequisites("deps", Mode.TARGET);
}

private NestedSet<Artifact> getTransitivePythonSourcesFromSkylarkProvider(
TransitiveInfoCollection dep) {
StructImpl pythonSkylarkProvider = null;
try {
pythonSkylarkProvider =
SkylarkType.cast(
dep.get(PYTHON_SKYLARK_PROVIDER_NAME),
StructImpl.class,
null,
"%s should be a struct",
PYTHON_SKYLARK_PROVIDER_NAME);

if (pythonSkylarkProvider != null) {
Object sourceFiles = pythonSkylarkProvider.getValue(TRANSITIVE_PYTHON_SRCS);
String errorType;
if (sourceFiles == null) {
errorType = "null";
} else {
errorType = EvalUtils.getDataTypeNameFromClass(sourceFiles.getClass());
}
String errorMsg = "Illegal Argument: attribute '%s' in provider '%s' is "
+ "of unexpected type. Should be a set, but got a '%s'";
NestedSet<Artifact> pythonSourceFiles = SkylarkType.cast(
sourceFiles, SkylarkNestedSet.class, Artifact.class, null,
errorMsg, TRANSITIVE_PYTHON_SRCS, PYTHON_SKYLARK_PROVIDER_NAME, errorType)
.getSet(Artifact.class);
return pythonSourceFiles;
}
} catch (EvalException e) {
ruleContext.ruleError(e.getMessage());
}
return null;
private static String getOrderErrorMessage(String fieldName, Order expected, Order actual) {
return String.format(
"Incompatible order for %s: expected 'default' or '%s', got '%s'",
fieldName, expected.getSkylarkName(), actual.getSkylarkName());
}

private void collectTransitivePythonSourcesFrom(
Iterable<? extends TransitiveInfoCollection> deps, NestedSetBuilder<Artifact> builder) {
for (TransitiveInfoCollection dep : deps) {
NestedSet<Artifact> pythonSourceFiles = getTransitivePythonSourcesFromSkylarkProvider(dep);
if (pythonSourceFiles != null) {
builder.addTransitive(pythonSourceFiles);
if (PyProvider.hasProvider(dep)) {
try {
StructImpl info = PyProvider.getProvider(dep);
NestedSet<Artifact> sources = PyProvider.getTransitiveSources(info);
if (!builder.getOrder().isCompatible(sources.getOrder())) {
ruleContext.ruleError(
getOrderErrorMessage(
PyProvider.TRANSITIVE_SOURCES, builder.getOrder(), sources.getOrder()));
} else {
builder.addTransitive(sources);
}
} catch (EvalException e) {
// Either the provider type or field type is bad.
ruleContext.ruleError(e.getMessage());
}
} else {
// TODO(bazel-team): We also collect .py source files from deps (e.g. for proto_library
// rules). Rules should implement PythonSourcesProvider instead.
// rules). We should have rules implement a "PythonSourcesProvider" instead.
FileProvider provider = dep.getProvider(FileProvider.class);
builder.addAll(FileType.filter(provider.getFilesToBuild(), PyRuleClasses.PYTHON_SOURCE));
}
Expand All @@ -389,6 +344,8 @@ private NestedSet<Artifact> collectTransitivePythonSources() {
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<Artifact> collectTransitivePythonSourcesWithoutLocal() {
NestedSetBuilder<Artifact> builder = NestedSetBuilder.compileOrder();
collectTransitivePythonSourcesFrom(getTargetDeps(), builder);
Expand All @@ -406,7 +363,15 @@ private void collectTransitivePythonImports(NestedSetBuilder<String> builder) {
for (TransitiveInfoCollection dep : getTargetDeps()) {
if (dep.getProvider(PythonImportsProvider.class) != null) {
PythonImportsProvider provider = dep.getProvider(PythonImportsProvider.class);
builder.addTransitive(provider.getTransitivePythonImports());
NestedSet<String> imports = provider.getTransitivePythonImports();
if (!builder.getOrder().isCompatible(imports.getOrder())) {
// TODO(brandjon): Add test case for this error, once we replace PythonImportsProvider
// with the normal Python provider and once we clean up our provider merge logic.
ruleContext.ruleError(
getOrderErrorMessage(PyProvider.IMPORTS, builder.getOrder(), imports.getOrder()));
} else {
builder.addTransitive(imports);
}
}
}
}
Expand Down Expand Up @@ -510,11 +475,12 @@ public boolean usesSharedLibraries() {
public static boolean checkForSharedLibraries(Iterable<TransitiveInfoCollection> deps)
throws EvalException{
for (TransitiveInfoCollection dep : deps) {
Object providerObject = dep.get(PYTHON_SKYLARK_PROVIDER_NAME);
Object providerObject = dep.get(PyProvider.PROVIDER_NAME);
if (providerObject != null) {
SkylarkType.checkType(providerObject, StructImpl.class, null);
StructImpl provider = (StructImpl) providerObject;
Boolean isUsingSharedLibrary = provider.getValue(IS_USING_SHARED_LIBRARY, Boolean.class);
Boolean isUsingSharedLibrary =
provider.getValue(PyProvider.USES_SHARED_LIBRARIES, Boolean.class);
if (Boolean.TRUE.equals(isUsingSharedLibrary)) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
// Copyright 2018 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.rules.python;

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.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import com.google.devtools.build.lib.syntax.SkylarkType;

/**
* Static helper class for managing the "py" struct provider returned and consumed by Python rules.
*/
// TODO(brandjon): Replace this with a real provider.
public class PyProvider {

// Disable construction.
private PyProvider() {}

/** Name of the Python provider in Starlark code (as a field of a {@code Target}. */
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).
*/
public static final String TRANSITIVE_SOURCES = "transitive_sources";

/**
* Name of field holding a boolean indicating whether any transitive dep uses shared libraries.
*/
public static final String USES_SHARED_LIBRARIES = "uses_shared_libraries";

/**
* Name of field holding a depset of import paths added by the transitive deps (including this
* target).
*/
public static final String IMPORTS = "imports";

/** Constructs a provider instance with the given field values. */
public static StructImpl create(
NestedSet<Artifact> transitiveSources,
boolean usesSharedLibraries,
NestedSet<String> 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'");
}

/** Returns whether a given dependency has the py provider. */
public static boolean hasProvider(TransitiveInfoCollection dep) {
return dep.get(PROVIDER_NAME) != null;
}

/**
* Returns the struct representing the py provider, from the given target info.
*
* @throws EvalException if the provider does not exist or has the wrong type.
*/
public static StructImpl getProvider(TransitiveInfoCollection dep) throws EvalException {
Object info = dep.get(PROVIDER_NAME);
if (info == null) {
throw new EvalException(/*location=*/ null, "Target does not have 'py' provider");
}
return SkylarkType.cast(
info, StructImpl.class, null, "'%s' provider should be a struct", PROVIDER_NAME);
}

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));
}
return fieldValue;
}

/**
* Casts and returns the transitive sources field.
*
* @throws EvalException if the field does not exist or is not a depset of {@link Artifact}
*/
public static NestedSet<Artifact> getTransitiveSources(StructImpl info) throws EvalException {
Object fieldValue = getValue(info, TRANSITIVE_SOURCES);
SkylarkNestedSet castValue =
SkylarkType.cast(
fieldValue,
SkylarkNestedSet.class,
Artifact.class,
null,
"'%s' provider's '%s' field should be a depset of Files (got a '%s')",
PROVIDER_NAME,
TRANSITIVE_SOURCES,
EvalUtils.getDataTypeNameFromClass(fieldValue.getClass()));
return castValue.getSet(Artifact.class);
}

/**
* Casts and returns the uses-shared-libraries field.
*
* @throws EvalException if the field does not exist or is not a boolean
*/
public static boolean getUsesSharedLibraries(StructImpl info) throws EvalException {
Object fieldValue = getValue(info, USES_SHARED_LIBRARIES);
return SkylarkType.cast(
fieldValue,
Boolean.class,
null,
"'%s' provider's '%s' field should be a boolean (got a '%s')",
PROVIDER_NAME,
USES_SHARED_LIBRARIES,
EvalUtils.getDataTypeNameFromClass(fieldValue.getClass()));
}

/**
* Casts and returns the imports field.
*
* @throws EvalException if the field does not exist or is not a depset of strings
*/
public static NestedSet<String> getImports(StructImpl info) throws EvalException {
Object fieldValue = getValue(info, IMPORTS);
SkylarkNestedSet castValue =
SkylarkType.cast(
fieldValue,
SkylarkNestedSet.class,
String.class,
null,
"'%s' provider's '%s' field should be a depset of strings (got a '%s')",
PROVIDER_NAME,
IMPORTS,
EvalUtils.getDataTypeNameFromClass(fieldValue.getClass()));
return castValue.getSet(String.class);
}
}
Loading

0 comments on commit 455caab

Please sign in to comment.