Skip to content

Commit

Permalink
Enable command-line aspects parameters
Browse files Browse the repository at this point in the history
This CL adds `--aspects_parameters` option to pass values for command-line aspects specified via `--aspects` option. The new option `--aspects_parameters` is only effective with `--experimental_allow_top_level_aspects_parameters` flag.

PiperOrigin-RevId: 412476585
  • Loading branch information
mai93 authored and copybara-github committed Nov 26, 2021
1 parent 4f3bf33 commit 3771072
Show file tree
Hide file tree
Showing 17 changed files with 1,025 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.analysis;


import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
Expand Down Expand Up @@ -197,6 +198,7 @@ public AnalysisResult update(
Set<String> multiCpu,
ImmutableSet<String> explicitTargetPatterns,
List<String> aspects,
ImmutableMap<String, String> aspectsParameters,
AnalysisOptions viewOptions,
boolean keepGoing,
boolean checkForActionConflicts,
Expand Down Expand Up @@ -348,7 +350,7 @@ public AnalysisResult update(
if (!aspectClasses.isEmpty()) {
aspectsKeys.add(
AspectKeyCreator.createTopLevelAspectsKey(
aspectClasses, targetSpec.getLabel(), configuration));
aspectClasses, targetSpec.getLabel(), configuration, aspectsParameters));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ private static AnalysisAndExecutionResult runAnalysisAndExecutionPhase(
multiCpu,
explicitTargetPatterns,
request.getAspects(),
request.getAspectsParameters(),
request.getViewOptions(),
request.getKeepGoing(),
request.getCheckForActionConflicts(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ private static AnalysisResult runAnalysisPhase(
multiCpu,
explicitTargetPatterns,
request.getAspects(),
request.getAspectsParameters(),
request.getViewOptions(),
request.getKeepGoing(),
request.getCheckForActionConflicts(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,19 @@
// limitations under the License.
package com.google.devtools.build.lib.buildtool;

import static com.google.common.collect.ImmutableMap.toImmutableMap;

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.devtools.build.lib.analysis.AnalysisOptions;
import com.google.devtools.build.lib.analysis.OutputGroupInfo;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
import com.google.devtools.build.lib.exec.ExecutionOptions;
Expand All @@ -31,6 +35,8 @@
import com.google.devtools.build.lib.runtime.KeepGoingOption;
import com.google.devtools.build.lib.runtime.LoadingPhaseThreadsOption;
import com.google.devtools.build.lib.runtime.UiOptions;
import com.google.devtools.build.lib.server.FailureDetails.Analysis;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.util.OptionsUtils;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.common.options.OptionsBase;
Expand All @@ -40,6 +46,7 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import javax.annotation.Nullable;

/**
* A BuildRequest represents a single invocation of the build tool by a user.
Expand Down Expand Up @@ -383,6 +390,24 @@ public ImmutableList<String> getAspects() {
return result.build();
}

@Nullable
public ImmutableMap<String, String> getAspectsParameters() throws ViewCreationFailedException {
List<Map.Entry<String, String>> aspectsParametersList = getBuildOptions().aspectsParameters;
try {
return aspectsParametersList.stream()
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
} catch (IllegalArgumentException e) {
String errorMessage = "Error in top-level aspects parameters";
throw new ViewCreationFailedException(
errorMessage,
FailureDetail.newBuilder()
.setMessage(errorMessage)
.setAnalysis(Analysis.newBuilder().setCode(Analysis.Code.ASPECT_CREATION_FAILED))
.build(),
e);
}
}

/** Whether {@value #VALIDATION_ASPECT_NAME} is in use. */
public boolean useValidationAspect() {
return validationMode() == OutputGroupInfo.ValidationMode.ASPECT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.RegexPatternOption;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -318,6 +319,23 @@ public class BuildRequestOptions extends OptionsBase {
+ " 'my_aspect' is a top-level value from a file tools/my_def.bzl")
public List<String> aspects;

@Option(
name = "aspects_parameters",
converter = Converters.AssignmentConverter.class,
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.GENERIC_INPUTS,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
allowMultiple = true,
help =
"Specifies the values of the command-line aspects parameters. Each parameter value is"
+ " specified via <param_name>=<param_value>, for example 'my_param=my_val' where"
+ " 'my_param' is a parameter of some aspect in --aspects list or required by an"
+ " aspect in the list. This option can be used multiple times. However, it is not"
+ " allowed to assign values to the same parameter more than once. This option is"
+ " only be effective under the experimental flag"
+ " --experimental_allow_top_level_aspects_parameters.")
public List<Map.Entry<String, String>> aspectsParameters;

public BuildRequestOptions() throws OptionsParsingException {}

public String getSymlinkPrefix(String productName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@

package com.google.devtools.build.lib.packages;

import static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
Expand Down Expand Up @@ -48,20 +52,64 @@ public ImmutableList<AspectDetails<?>> getAspectsDetails() {
return ImmutableList.copyOf(aspects.values());
}

/**
* Returns a list of Aspect objects for top level aspects.
*
* <p>Since top level aspects do not have parameters, a rule is not required to create their
* Aspect objects.
*/
public ImmutableList<Aspect> buildAspects() {
/** Returns a list of Aspect objects for top level aspects. */
public ImmutableList<Aspect> buildAspects(ImmutableMap<String, String> aspectsParameters)
throws EvalException {
Preconditions.checkArgument(aspectsParameters != null, "aspectsParameters cannot be null");

ImmutableList.Builder<Aspect> aspectsList = ImmutableList.builder();
for (AspectDetails<?> aspect : aspects.values()) {
aspectsList.add(aspect.getAspect(null));
aspectsList.add(aspect.getTopLevelAspect(aspectsParameters));
}
return aspectsList.build();
}

/**
* Validates top-level aspects parameters and reports error in the following cases:
*
* <p>If a parameter name is specified in command line but no aspect has a parameter with that
* name.
*
* <p>If a mandatory aspect attribute is not given a value in the top-level parameters list.
*/
public boolean validateTopLevelAspectsParameters(ImmutableMap<String, String> aspectsParameters)
throws EvalException {
Preconditions.checkArgument(aspectsParameters != null, "aspectsParameters cannot be null");

ImmutableSet.Builder<String> usedParametersBuilder = ImmutableSet.builder();
for (AspectDetails<?> aspectDetails : aspects.values()) {
if (aspectDetails instanceof StarlarkAspectDetails) {
ImmutableList<Attribute> aspectAttributes =
((StarlarkAspectDetails) aspectDetails).aspect.getAttributes();
for (Attribute attr : aspectAttributes) {
if (attr.isImplicit() || attr.isLateBound()) {
continue;
}
String attrName = attr.getName();
if (aspectsParameters.containsKey(attrName)) {
usedParametersBuilder.add(attrName);
} else if (attr.isMandatory()) {
throw Starlark.errorf(
"Missing mandatory attribute '%s' for aspect '%s'.",
attrName, aspectDetails.getName());
}
}
}
}
ImmutableSet<String> usedParameters = usedParametersBuilder.build();
ImmutableList<String> unusedParameters =
aspectsParameters.keySet().stream()
.filter(p -> !usedParameters.contains(p))
.collect(toImmutableList());
if (!unusedParameters.isEmpty()) {
throw Starlark.errorf(
"Parameters '%s' are not parameters of any of the top-level aspects but they are"
+ " specified in --aspects_parameters.",
unusedParameters);
}
return true;
}

/** Wraps the information necessary to construct an Aspect. */
@VisibleForSerialization
abstract static class AspectDetails<C extends AspectClass> {
Expand Down Expand Up @@ -92,7 +140,10 @@ ImmutableSet<String> getRequiredParameters() {
return ImmutableSet.of();
}

protected abstract Aspect getAspect(@Nullable Rule rule);
protected abstract Aspect getAspect(Rule rule);

protected abstract Aspect getTopLevelAspect(ImmutableMap<String, String> aspectParameters)
throws EvalException;

C getAspectClass() {
return aspectClass;
Expand All @@ -114,14 +165,16 @@ private static class NativeAspectDetails extends AspectDetails<NativeAspectClass

@Override
public Aspect getAspect(Rule rule) {
AspectParameters params;
if (rule == null) {
params = AspectParameters.EMPTY;
} else {
params = parametersExtractor.apply(rule);
}
AspectParameters params = parametersExtractor.apply(rule);
return params == null ? null : Aspect.forNative(aspectClass, params);
}

@Override
protected Aspect getTopLevelAspect(ImmutableMap<String, String> aspectParameters)
throws EvalException {
// Native aspects ignore their top-level parameters values for now.
return Aspect.forNative(aspectClass, AspectParameters.EMPTY);
}
}

@VisibleForSerialization
Expand All @@ -142,12 +195,14 @@ public ImmutableSet<String> getRequiredParameters() {

@Override
public Aspect getAspect(Rule rule) {
AspectParameters params;
if (rule == null) {
params = AspectParameters.EMPTY;
} else {
params = parametersExtractor.apply(rule);
}
AspectParameters params = parametersExtractor.apply(rule);
return Aspect.forStarlark(aspectClass, aspect.getDefinition(params), params);
}

@Override
public Aspect getTopLevelAspect(ImmutableMap<String, String> aspectParameters)
throws EvalException {
AspectParameters params = aspect.extractTopLevelParameters(aspectParameters);
return Aspect.forStarlark(aspectClass, aspect.getDefinition(params), params);
}
}
Expand All @@ -165,6 +220,12 @@ private static class PredefinedAspectDetails extends AspectDetails<AspectClass>
public Aspect getAspect(Rule rule) {
return aspect;
}

@Override
public Aspect getTopLevelAspect(ImmutableMap<String, String> aspectParameters)
throws EvalException {
return aspect;
}
}

@SerializationConstant @AutoCodec.VisibleForSerialization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.cmdline.Label;
Expand Down Expand Up @@ -219,6 +220,38 @@ public Function<Rule, AspectParameters> getDefaultParametersExtractor() {
};
}

public AspectParameters extractTopLevelParameters(ImmutableMap<String, String> parametersValues)
throws EvalException {
AspectParameters.Builder builder = new AspectParameters.Builder();
for (Attribute aspectParameter : attributes) {
String parameterName = aspectParameter.getName();
if (Attribute.isImplicit(parameterName) || Attribute.isLateBound(parameterName)) {
// These attributes are the private matters of the aspect
continue;
}

Preconditions.checkArgument(
aspectParameter.getType() == Type.STRING,
"Cannot pass value of non-string attribute '%s' in aspect %s.",
getName(),
parameterName);

String parameterValue =
parametersValues.getOrDefault(
parameterName, (String) aspectParameter.getDefaultValue(null));

PredicateWithMessage<Object> allowedValues = aspectParameter.getAllowedValues();
if (allowedValues.apply(parameterValue)) {
builder.addAttribute(parameterName, parameterValue);
} else {
throw Starlark.errorf(
"%s: invalid value in '%s' attribute: %s",
getName(), parameterName, allowedValues.getErrorReason(parameterValue));
}
}
return builder.build();
}

public ImmutableList<Label> getRequiredToolchains() {
return requiredToolchains;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,17 @@ public final class BuildLanguageOptions extends OptionsBase {
+ " providers of the aspect.")
public boolean incompatibleTopLevelAspectsRequireProviders;

@Option(
name = "experimental_allow_top_level_aspects_parameters",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
help =
"If set to true, top level aspects will accept values for their parameters passed via"
+ " --aspects_parameters option.")
public boolean experimentalAllowTopLevelAspectsParameters;

/**
* An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
* never accumulate a large number of these and being able to shortcut on object identity makes a
Expand Down Expand Up @@ -605,6 +616,9 @@ public StarlarkSemantics toStarlarkSemantics() {
.setBool(
INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS,
incompatibleTopLevelAspectsRequireProviders)
.setBool(
EXPERIMENTAL_ALLOW_TOP_LEVEL_ASPECTS_PARAMETERS,
experimentalAllowTopLevelAspectsParameters)
.build();
return INTERNER.intern(semantics);
}
Expand Down Expand Up @@ -673,6 +687,8 @@ public StarlarkSemantics toStarlarkSemantics() {
"-incompatible_visibility_private_attributes_at_definition";
public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS =
"-incompatible_top_level_aspects_require_providers";
public static final String EXPERIMENTAL_ALLOW_TOP_LEVEL_ASPECTS_PARAMETERS =
"-experimental_allow_top_level_aspects_parameters";

// non-booleans
public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
Expand Down
Loading

0 comments on commit 3771072

Please sign in to comment.