Skip to content

Commit

Permalink
Add incompatible flag to guard top-level aspects dependencies
Browse files Browse the repository at this point in the history
This CL introduces `incompatible_top_level_aspects_dependency` flag to enable aspect-on-aspect and requiring aspects for command line aspects. This can be needed because the new behavior can break builds where a relation between top-level aspects existed (based on required_aspect_providers and provides) but it was never actually applied as it was not supported.

One example of these cases:
blaze build //:main --aspects=/tools:my_def.bzl%a1,/tools:my_def.bzl%a2

If aspect a1 provides a1p provider, aspect a2 requires a1p provider and the rule of target `main` also provides a1p. Once top-level aspect-on-aspect is enabled this build will fail because a1p will be provided twice to a2 (from a1 applied on `main` and from `main` target rule). Previously the relation between a1 and a2 was not detected and a2 used to get only the value of a1p from `main`'s rule.

PiperOrigin-RevId: 390587290
  • Loading branch information
mai93 authored and copybara-github committed Aug 13, 2021
1 parent 055e40d commit ed25118
Show file tree
Hide file tree
Showing 7 changed files with 290 additions and 67 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -166,19 +166,4 @@ public class AnalysisOptions extends OptionsBase {
+ " be used. Example value: \"HOST_CPUS*0.5\".",
converter = CpuResourceConverter.class)
public int oomSensitiveSkyFunctionsSemaphoreSize;

@Option(
name = "incompatible_ignore_duplicate_top_level_aspects",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
help =
"If true, remove duplicates from --aspects list by keeping only the first occurrence of"
+ " every aspect. Otherwise, throw validation error if duplicate aspects are"
+ " encountered.")
public boolean ignoreDuplicateTopLevelAspects;
}
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,6 @@ public AnalysisResult update(
.collect(Collectors.toList());

ImmutableList.Builder<AspectClass> aspectClassesBuilder = ImmutableList.builder();
if (viewOptions.ignoreDuplicateTopLevelAspects) {
// remove duplicates from aspects list
aspects = ImmutableSet.copyOf(aspects).asList();
}
for (String aspect : aspects) {
// Syntax: label%aspect
int delimiterPosition = aspect.indexOf('%');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,17 +306,18 @@ public class BuildRequestOptions extends OptionsBase {
allowMultiple = true,
help =
"Comma-separated list of aspects to be applied to top-level targets. All aspects are"
+ " applied to all top-level targets. If aspect <code>some_aspect</code> specifies"
+ " required aspect providers via <code>required_aspect_providers</code>,"
+ " <code>some_aspect</code> will run after every aspect that was mentioned before it"
+ " in the aspects list and whose advertised providers satisfy"
+ " <code>some_aspect</code> required aspect providers. <code>some_aspect</code> will"
+ " then have access to the values of those aspects' providers. Aspects that do not"
+ " have such dependency will run independently on the top-level targets."
+ ""
+ " Aspects are specified in the form <bzl-file-label>%<aspect_name>, for example"
+ " '//tools:my_def.bzl%my_aspect', where 'my_aspect' is a top-level value from a"
+ " file tools/my_def.bzl")
+ " applied independently to all top-level targets except if"
+ " <code>incompatible_top_level_aspects_dependency</code> is used. In this case, if"
+ " aspect <code>some_aspect</code> specifies required aspect providers via"
+ " <code>required_aspect_providers</code>, <code>some_aspect</code> will run after"
+ " every aspect that was mentioned before it in the aspects list whose advertised"
+ " providers satisfy <code>some_aspect</code> required aspect providers. Moreover,"
+ " <code>some_aspect</code> will run after all its required aspects specified by"
+ " <code>requires</code> attribute which otherwise will be ignored."
+ " <code>some_aspect</code> will then have access to the values of those aspects'"
+ " providers."
+ " <bzl-file-label>%<aspect_name>, for example '//tools:my_def.bzl%my_aspect', where"
+ " 'my_aspect' is a top-level value from a file tools/my_def.bzl")
public List<String> aspects;

public BuildRequestOptions() throws OptionsParsingException {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,22 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable {
+ " providers of the aspect.")
public boolean incompatibleTopLevelAspectsRequireProviders;

@Option(
name = "incompatible_top_level_aspects_dependency",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
help =
"If set to true, a dependency between the top level aspects will be built based on their"
+ " required aspect providers, advertised providers and required aspects. Otherwise,"
+ " each aspect in the list will run independently and its required aspects will be"
+ " ignored.")
public boolean incompatibleTopLevelAspectsDependOnAspects;

@Option(
name = "experimental_required_aspects",
defaultValue = "false",
Expand Down Expand Up @@ -698,6 +714,9 @@ public StarlarkSemantics toStarlarkSemantics() {
.setBool(
INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS,
incompatibleTopLevelAspectsRequireProviders)
.setBool(
INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY,
incompatibleTopLevelAspectsDependOnAspects)
.setBool(EXPERIMENTAL_REQUIRED_ASPECTS, experimentalRequiredAspects)
.build();
return INTERNER.intern(semantics);
Expand Down Expand Up @@ -770,6 +789,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 INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY =
"-incompatible_top_level_aspects_dependency";
public static final String EXPERIMENTAL_REQUIRED_ASPECTS = "-experimental_required_aspects";

// non-booleans
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import com.google.common.base.Objects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Interner;
import com.google.devtools.build.lib.analysis.AspectCollection;
import com.google.devtools.build.lib.analysis.AspectCollection.AspectCycleOnPathException;
Expand All @@ -28,6 +29,7 @@
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.StarlarkAspect;
import com.google.devtools.build.lib.packages.StarlarkAspectClass;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.server.FailureDetails.Analysis;
import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
Expand All @@ -46,6 +48,7 @@
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.StarlarkSemantics;

/**
* SkyFunction to load top level aspects, build the dependency relation between them based on the
Expand All @@ -68,11 +71,33 @@ public class BuildTopLevelAspectsDetailsFunction implements SkyFunction {
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws BuildTopLevelAspectsDetailsFunctionException, InterruptedException {

BuildTopLevelAspectsDetailsKey topLevelAspectsDetailsKey =
(BuildTopLevelAspectsDetailsKey) skyKey.argument();
ImmutableList<AspectClass> topLevelAspectsClasses =
topLevelAspectsDetailsKey.getTopLevelAspectsClasses();

StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (starlarkSemantics == null) {
return null;
}
boolean buildTopLevelAspectsDependency =
starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY);
if (!buildTopLevelAspectsDependency) {
// If building a relation between top-level aspects is not required, then we can remove
// duplicate aspects by keeping the first occurrence of each aspect.
topLevelAspectsClasses = ImmutableSet.copyOf(topLevelAspectsClasses).asList();

// Then return a list of indenpendent aspects to be applied on the top-level targets
ImmutableList<AspectDetails> aspectsDetailsList =
getIndependentTopLevelAspects(env, topLevelAspectsClasses);
if (aspectsDetailsList == null) {
return null; // some aspects are not loaded
}
return new BuildTopLevelAspectsDetailsValue(aspectsDetailsList);
}

ImmutableList<Aspect> topLevelAspects =
getTopLevelAspects(env, topLevelAspectsDetailsKey.getTopLevelAspectsClasses());
ImmutableList<Aspect> topLevelAspects = getTopLevelAspects(env, topLevelAspectsClasses);

if (topLevelAspects == null) {
return null; // some aspects are not loaded
Expand All @@ -98,11 +123,9 @@ public String extractTag(SkyKey skyKey) {
}

@Nullable
private static ImmutableList<Aspect> getTopLevelAspects(
private static Map<SkyKey, SkyValue> loadAspects(
Environment env, ImmutableList<AspectClass> topLevelAspectsClasses)
throws InterruptedException, BuildTopLevelAspectsDetailsFunctionException {
AspectsListBuilder aspectsList = new AspectsListBuilder();

throws InterruptedException {
ImmutableList.Builder<StarlarkAspectLoadingKey> aspectLoadingKeys = ImmutableList.builder();
for (AspectClass aspectClass : topLevelAspectsClasses) {
if (aspectClass instanceof StarlarkAspectClass) {
Expand All @@ -116,6 +139,47 @@ private static ImmutableList<Aspect> getTopLevelAspects(
if (env.valuesMissing()) {
return null;
}
return loadedAspects;
}

@Nullable
private static ImmutableList<AspectDetails> getIndependentTopLevelAspects(
Environment env, ImmutableList<AspectClass> topLevelAspectsClasses)
throws InterruptedException {
Map<SkyKey, SkyValue> loadedAspects = loadAspects(env, topLevelAspectsClasses);
if (loadedAspects == null) {
return null;
}
ImmutableList.Builder<AspectDetails> aspectDetailsList = ImmutableList.builder();

for (AspectClass aspectClass : topLevelAspectsClasses) {
if (aspectClass instanceof StarlarkAspectClass) {
StarlarkAspectLoadingValue aspectLoadingValue =
(StarlarkAspectLoadingValue)
loadedAspects.get(
LoadStarlarkAspectFunction.createStarlarkAspectLoadingKey(
(StarlarkAspectClass) aspectClass));
StarlarkAspect starlarkAspect = aspectLoadingValue.getAspect();
aspectDetailsList.add(
new AspectDetails(
ImmutableList.of(), new AspectDescriptor(starlarkAspect.getAspectClass())));
} else {
aspectDetailsList.add(
new AspectDetails(ImmutableList.of(), new AspectDescriptor(aspectClass)));
}
}
return aspectDetailsList.build();
}

@Nullable
private static ImmutableList<Aspect> getTopLevelAspects(
Environment env, ImmutableList<AspectClass> topLevelAspectsClasses)
throws InterruptedException, BuildTopLevelAspectsDetailsFunctionException {
AspectsListBuilder aspectsList = new AspectsListBuilder();
Map<SkyKey, SkyValue> loadedAspects = loadAspects(env, topLevelAspectsClasses);
if (loadedAspects == null) {
return null;
}

for (AspectClass aspectClass : topLevelAspectsClasses) {
if (aspectClass instanceof StarlarkAspectClass) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ public void duplicateTopLevelAspects_duplicateAspectsNotAllowed() throws Excepti
AspectApplyingToFiles aspectApplyingToFiles = new AspectApplyingToFiles();
setRulesAndAspectsAvailableInTests(ImmutableList.of(aspectApplyingToFiles), ImmutableList.of());
pkg("a", "java_binary(name = 'x', main_class = 'x.FooBar', srcs = ['x.java'])");
useConfiguration("--incompatible_ignore_duplicate_top_level_aspects=false");
useConfiguration("--incompatible_top_level_aspects_dependency");
reporter.removeHandler(failFastHandler);

assertThrows(
Expand Down
Loading

0 comments on commit ed25118

Please sign in to comment.