Skip to content

Commit

Permalink
Make allTargets a memoized value of BuildGraphData rather than ex…
Browse files Browse the repository at this point in the history
…plicity calculating it inside `BlazeQueryParser`.

Since it can be derived from other data that `BuildGraphData` already has, do this to make the semantics clearer.

PiperOrigin-RevId: 568525276
  • Loading branch information
Googler authored and copybara-github committed Sep 26, 2023
1 parent 29338d2 commit 404e0a1
Show file tree
Hide file tree
Showing 18 changed files with 277 additions and 223 deletions.
23 changes: 12 additions & 11 deletions aswb/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
# Description: Builds ASwB for blaze and bazel
#

load(
"//:build-visibility.bzl",
"ASWB_PACKAGES_VISIBILITY",
"ASWB_PLUGIN_PACKAGES_VISIBILITY",
"ASWB_SUBPACKAGES_VISIBILITY",
"TEST_ASWB_SUBPACKAGES_VISIBILITY",
)
load("//:version.bzl", "VERSION")
load(
"//build_defs:build_defs.bzl",
"intellij_plugin",
Expand All @@ -15,23 +23,15 @@ load(
"//build_defs:intellij_plugin_debug_target.bzl",
"intellij_plugin_debug_target",
)
load("//:version.bzl", "VERSION")
load(
"//intellij_platform_sdk:build_defs.bzl",
"combine_visibilities",
"select_for_plugin_api",
)
load(
"//testing:test_defs.bzl",
"intellij_integration_test_suite",
"intellij_unit_test_suite",
)
load(
"//:build-visibility.bzl",
"ASWB_PACKAGES_VISIBILITY",
"ASWB_PLUGIN_PACKAGES_VISIBILITY",
"ASWB_SUBPACKAGES_VISIBILITY",
"TEST_ASWB_SUBPACKAGES_VISIBILITY",
"//intellij_platform_sdk:build_defs.bzl",
"combine_visibilities",
"select_for_plugin_api",
)

licenses(["notice"])
Expand Down Expand Up @@ -179,6 +179,7 @@ java_library(
"//intellij_platform_sdk:plugin_api",
"//java",
"//proto:proto_deps",
"//querysync",
"//shared",
"//third_party/auto_value",
"@gson//jar",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
import com.google.idea.blaze.base.model.BlazeProjectData;
import com.google.idea.blaze.base.model.primitives.Label;
import com.google.idea.blaze.base.qsync.QuerySync;
import com.google.idea.blaze.common.BuildTarget;
import com.google.idea.blaze.java.AndroidBlazeRules.RuleTypes;
import com.google.idea.blaze.qsync.project.ProjectTarget;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -78,7 +78,8 @@ public static InstrumentationInfo getInstrumentationInfo(
Label instrumentationTestLabel, BlazeProjectData projectData)
throws InstrumentationParserException {
if (QuerySync.isEnabled()) {
BuildTarget testTarget = projectData.getBuildTarget(instrumentationTestLabel);
ProjectTarget testTarget =
(ProjectTarget) projectData.getBuildTarget(instrumentationTestLabel);
if (testTarget == null) {
String msg = "Unable to identify target \"" + instrumentationTestLabel + "\".";
throw new InstrumentationParserException(msg);
Expand All @@ -88,7 +89,7 @@ public static InstrumentationInfo getInstrumentationInfo(
throw new InstrumentationParserException(msg);
}
Label testApp = Label.create(testTarget.testApp().get().toString());
BuildTarget targetApp = projectData.getBuildTarget(testApp);
ProjectTarget targetApp = (ProjectTarget) projectData.getBuildTarget(testApp);
Label instruments = null;
if (targetApp != null && targetApp.instruments().isPresent()) {
instruments = Label.create(targetApp.instruments().get().toString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,8 @@ public BuildTarget getBuildTarget(Label label) {
if (targetInfo == null) {
return null;
}
return BuildTarget.builder()
.setLabel(com.google.idea.blaze.common.Label.of(targetInfo.label.toString()))
.setKind(targetInfo.kindString)
.build();
return BuildTarget.create(
com.google.idea.blaze.common.Label.of(targetInfo.label.toString()), targetInfo.kindString);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
import com.google.idea.blaze.base.sync.projectview.WorkspaceLanguageSettings;
import com.google.idea.blaze.base.sync.workspace.ArtifactLocationDecoder;
import com.google.idea.blaze.base.sync.workspace.WorkspacePathResolver;
import com.google.idea.blaze.common.BuildTarget;
import com.google.idea.blaze.qsync.project.BlazeProjectSnapshot;
import com.google.idea.blaze.qsync.project.ProjectTarget;
import com.intellij.openapi.diagnostic.Logger;
import java.nio.file.Path;
import java.util.Collection;
Expand Down Expand Up @@ -66,7 +66,7 @@ public QuerySyncProjectData withSnapshot(BlazeProjectSnapshot newSnapshot) {

@Nullable
@Override
public BuildTarget getBuildTarget(Label label) {
public ProjectTarget getBuildTarget(Label label) {
return blazeProject
.map(BlazeProjectSnapshot::getTargetMap)
.map(map -> map.get(com.google.idea.blaze.common.Label.of(label.toString())))
Expand All @@ -80,7 +80,7 @@ public BuildTarget getBuildTarget(Label label) {
* <p>If project target A depends on external target B, and external target B depends on project
* target C, target A is *not* included in {@code getReverseDeps} for a source file in target C.
*/
public Collection<BuildTarget> getReverseDeps(Path sourcePath) {
public Collection<ProjectTarget> getReverseDeps(Path sourcePath) {
return blazeProject
.map(BlazeProjectSnapshot::graph)
.map(graph -> graph.getReverseDepsForSource(sourcePath))
Expand Down
1 change: 1 addition & 0 deletions querysync/BUILD
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package(default_visibility = [
":__subpackages__",
"//aswb:__subpackages__",
"//base:__subpackages__",
"//java/com/google/devtools/intellij/blaze/plugin/base:__subpackages__",
"//java/com/google/devtools/intellij/blaze/plugin/querysync:__subpackages__",
Expand Down
106 changes: 50 additions & 56 deletions querysync/java/com/google/idea/blaze/qsync/BlazeQueryParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,15 @@
import static com.google.idea.blaze.common.Label.toLabelList;
import static java.util.Objects.requireNonNull;

import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
import com.google.common.collect.Streams;
import com.google.idea.blaze.common.BuildTarget;
import com.google.idea.blaze.common.Context;
import com.google.idea.blaze.common.Label;
import com.google.idea.blaze.common.PrintOutput;
import com.google.idea.blaze.qsync.project.BuildGraphData;
import com.google.idea.blaze.qsync.project.BuildGraphData.Location;
import com.google.idea.blaze.qsync.project.ProjectTarget;
import com.google.idea.blaze.qsync.query.PackageSet;
import com.google.idea.blaze.qsync.query.Query;
import com.google.idea.blaze.qsync.query.Query.Rule;
Expand All @@ -48,6 +44,24 @@
*/
public class BlazeQueryParser {

/**
* Factory class to allow the creation of the the parser to be decoupled from the injection of its
* dependencies.
*/
public static class Factory {
private final Context<?> context;
private final ImmutableSet<String> alwaysBuildRuleKinds;

public Factory(Context<?> context, ImmutableSet<String> alwaysBuildRuleKinds) {
this.context = context;
this.alwaysBuildRuleKinds = alwaysBuildRuleKinds;
}

public BlazeQueryParser create(QuerySummary querySummary) {
return new BlazeQueryParser(querySummary, context, alwaysBuildRuleKinds);
}
}

// Rules that will need to be built, whether or not the target is included in the
// project.
public static final ImmutableSet<String> ALWAYS_BUILD_RULE_KINDS =
Expand Down Expand Up @@ -85,27 +99,29 @@ public class BlazeQueryParser {
private final Context<?> context;
private final SetView<String> alwaysBuildRuleKinds;

private final QuerySummary query;

private final BuildGraphData.Builder graphBuilder = BuildGraphData.builder();
private final PackageSet.Builder packages = new PackageSet.Builder();
private final Map<Label, Set<Label>> ruleDeps = Maps.newHashMap();
private final Map<Label, Set<Label>> ruleRuntimeDeps = Maps.newHashMap();

private final Set<Label> projectDeps = Sets.newHashSet();
private final ImmutableMultimap.Builder<Label, Label> targetSources = ImmutableMultimap.builder();
// All the project targets the aspect needs to build
private final Set<Label> projectTargetsToBuild = new HashSet<>();
// An aggregation of all the dependencies of java rules
private final Set<Label> javaDeps = new HashSet<>();

public BlazeQueryParser(Context<?> context, ImmutableSet<String> handledRuleKinds) {
public BlazeQueryParser(
QuerySummary query, Context<?> context, ImmutableSet<String> handledRuleKinds) {
this.context = context;
this.alwaysBuildRuleKinds = Sets.difference(ALWAYS_BUILD_RULE_KINDS, handledRuleKinds);
this.query = query;
}

private static boolean isJavaRule(String ruleClass) {
return JAVA_RULE_TYPES.contains(ruleClass) || ANDROID_RULE_TYPES.contains(ruleClass);
}

public BuildGraphData parse(QuerySummary query) {
public BuildGraphData parse() {
context.output(PrintOutput.log("Analyzing project structure..."));

long now = System.nanoTime();
Expand All @@ -125,25 +141,27 @@ public BuildGraphData parse(QuerySummary query) {
String ruleClass = ruleEntry.getValue().getRuleClass();
ruleCount.compute(ruleClass, (k, v) -> (v == null ? 0 : v) + 1);

BuildTarget.Builder buildTarget =
BuildTarget.builder().setLabel(ruleEntry.getKey()).setKind(ruleClass);
ProjectTarget.Builder targetBuilder = ProjectTarget.builder();

targetBuilder.label(ruleEntry.getKey()).kind(ruleClass);
if (!ruleEntry.getValue().getTestApp().isEmpty()) {
buildTarget.setTestApp(Label.of(ruleEntry.getValue().getTestApp()));
targetBuilder.testApp(Label.of(ruleEntry.getValue().getTestApp()));
}
if (!ruleEntry.getValue().getInstruments().isEmpty()) {
buildTarget.setInstruments(Label.of(ruleEntry.getValue().getInstruments()));
targetBuilder.instruments(Label.of(ruleEntry.getValue().getInstruments()));
}
if (!ruleEntry.getValue().getCustomPackage().isEmpty()) {
buildTarget.setCustomPackage(ruleEntry.getValue().getCustomPackage());
targetBuilder.customPackage(ruleEntry.getValue().getCustomPackage());
}
graphBuilder.targetMapBuilder().put(ruleEntry.getKey(), buildTarget.build());

if (isJavaRule(ruleClass)) {
visitJavaRule(query, ruleEntry.getKey(), ruleEntry.getValue());
visitJavaRule(ruleEntry.getKey(), ruleEntry.getValue(), targetBuilder);
}
if (alwaysBuildRuleKinds.contains(ruleClass)) {
projectTargetsToBuild.add(ruleEntry.getKey());
}

graphBuilder.targetMapBuilder().put(ruleEntry.getKey(), targetBuilder.build());
}
int nTargets = query.proto().getRulesCount();

Expand All @@ -159,15 +177,7 @@ public BuildGraphData parse(QuerySummary query) {
long elapsedMs = (System.nanoTime() - now) / 1000000L;
context.output(PrintOutput.log("%-10d Targets (%d ms):", nTargets, elapsedMs));

BuildGraphData graph =
graphBuilder
.targetSources(targetSources.build())
.ruleDeps(ruleDeps)
.ruleRuntimeDeps(ruleRuntimeDeps)
.projectDeps(projectDeps)
.packages(packages.build())
.reverseDeps(calculateReverseDeps())
.build();
BuildGraphData graph = graphBuilder.projectDeps(projectDeps).packages(packages.build()).build();

context.output(PrintOutput.log("%-10d Source files", graph.locations().size()));
context.output(PrintOutput.log("%-10d Java sources", graph.javaSources().size()));
Expand All @@ -178,15 +188,13 @@ public BuildGraphData parse(QuerySummary query) {
return graph;
}

private void visitJavaRule(QuerySummary query, Label label, Query.Rule rule) {
graphBuilder.allTargetsBuilder().add(label);
ImmutableSet<Label> thisSources = sourcesWithExpandedFileGroups(rule, query);
private void visitJavaRule(Label label, Query.Rule rule, ProjectTarget.Builder targetBuilder) {
ImmutableSet<Label> thisSources = sourcesWithExpandedFileGroups(rule);
Set<Label> thisDeps = Sets.newHashSet(toLabelList(rule.getDepsList()));
ruleDeps.computeIfAbsent(label, x -> Sets.newHashSet()).addAll(thisDeps);
targetBuilder.depsBuilder().addAll(thisDeps);

Set<Label> thisRuntimeDeps = Sets.newHashSet(toLabelList(rule.getRuntimeDepsList()));
ruleRuntimeDeps.computeIfAbsent(label, x -> Sets.newHashSet()).addAll(thisRuntimeDeps);
targetSources.putAll(label, thisSources);
targetBuilder.runtimeDepsBuilder().addAll(toLabelList(rule.getRuntimeDepsList()));
targetBuilder.sourceLabelsBuilder().addAll(thisSources);
for (Label thisSource : thisSources) {
// Require build step for targets with generated sources.
if (!query.getSourceFilesMap().containsKey(thisSource)) {
Expand All @@ -205,54 +213,40 @@ private void visitJavaRule(QuerySummary query, Label label, Query.Rule rule) {
projectTargetsToBuild.add(label);
}
if (!rule.getManifest().isEmpty()) {
targetSources.put(label, Label.of(rule.getManifest()));
targetBuilder.sourceLabelsBuilder().add(Label.of(rule.getManifest()));
}
}
}

private ImmutableMultimap<Label, Label> calculateReverseDeps() {
ArrayListMultimap<Label, Label> map = ArrayListMultimap.create();
Streams.concat(ruleDeps.entrySet().stream(), ruleRuntimeDeps.entrySet().stream())
.forEach(
entry -> {
for (Label dep : entry.getValue()) {
map.put(dep, entry.getKey());
}
});
return ImmutableMultimap.copyOf(map);
}

/** Returns a set of all sources for a rule, expanding any in-project {@code filegroup} rules */
private ImmutableSet<Label> sourcesWithExpandedFileGroups(Rule rule, QuerySummary query) {
private ImmutableSet<Label> sourcesWithExpandedFileGroups(Rule rule) {
ImmutableSet<Label> rawLabels =
ImmutableSet.<Label>builder()
.addAll(toLabelList(rule.getSourcesList()))
.addAll(toLabelList(rule.getResourceFilesList()))
.build();
return rawLabels.stream()
.flatMap(l -> expandFileGroups(l, query).stream())
.collect(toImmutableSet());
return rawLabels.stream().flatMap(l -> expandFileGroups(l).stream()).collect(toImmutableSet());
}

private ImmutableSet<Label> expandFileGroups(Label label, QuerySummary summary) {
if (!isFileGroup(label, summary)) {
private ImmutableSet<Label> expandFileGroups(Label label) {
if (!isFileGroup(label)) {
return ImmutableSet.of(label);
}
Set<Label> visited = Sets.newHashSet();
ImmutableSet.Builder<Label> result = ImmutableSet.builder();

for (String source : requireNonNull(summary.getRulesMap().get(label)).getSourcesList()) {
for (String source : requireNonNull(query.getRulesMap().get(label)).getSourcesList()) {
Label asLabel = Label.of(source);
if (visited.add(asLabel)) {
result.addAll(expandFileGroups(asLabel, summary));
result.addAll(expandFileGroups(asLabel));
}
}

return result.build();
}

private boolean isFileGroup(Label label, QuerySummary summary) {
Rule rule = summary.getRulesMap().get(label);
private boolean isFileGroup(Label label) {
Rule rule = query.getRulesMap().get(label);
if (rule == null) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class FullProjectUpdate implements RefreshOperation {

private final Context<?> context;
private final Path effectiveWorkspaceRoot;
private final BlazeQueryParser queryParser;
private final BlazeQueryParser.Factory queryParser;
private final ProjectDefinition projectDefinition;
private final GraphToProjectConverter graphToProjectConverter;

Expand All @@ -60,7 +60,7 @@ public FullProjectUpdate(
this.result =
PostQuerySyncData.builder().setProjectDefinition(definition).setVcsState(vcsState);
this.projectDefinition = definition;
this.queryParser = new BlazeQueryParser(context, handledRuleKinds);
this.queryParser = new BlazeQueryParser.Factory(context, handledRuleKinds);
this.graphToProjectConverter =
new GraphToProjectConverter(
packageReader, effectiveWorkspaceRoot, context, projectDefinition, executor);
Expand All @@ -80,7 +80,7 @@ public void setQueryOutput(QuerySummary output) {
@Override
public BlazeProjectSnapshot createBlazeProject() throws BuildException {
PostQuerySyncData newData = result.build();
BuildGraphData graph = queryParser.parse(newData.querySummary());
BuildGraphData graph = queryParser.create(newData.querySummary()).parse();
ProjectProto.Project project = graphToProjectConverter.createProject(graph);
return BlazeProjectSnapshot.builder().queryData(newData).graph(graph).project(project).build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class PartialProjectRefresh implements RefreshOperation {

private final Path effectiveWorkspaceRoot;
private final PostQuerySyncData previousState;
private final BlazeQueryParser queryParser;
private final BlazeQueryParser.Factory queryParser;
private final GraphToProjectConverter graphToProjectConverter;
private final PostQuerySyncData.Builder newState;
@VisibleForTesting final ImmutableSet<Path> modifiedPackages;
Expand All @@ -72,7 +72,7 @@ class PartialProjectRefresh implements RefreshOperation {
.setProjectDefinition(previousState.projectDefinition());
this.modifiedPackages = modifiedPackages;
this.deletedPackages = deletedPackages;
this.queryParser = new BlazeQueryParser(context, handledRuleKinds);
this.queryParser = new BlazeQueryParser.Factory(context, handledRuleKinds);
this.graphToProjectConverter =
new GraphToProjectConverter(
packageReader,
Expand Down Expand Up @@ -111,7 +111,7 @@ public BlazeProjectSnapshot createBlazeProject() throws BuildException {
Preconditions.checkNotNull(partialQuery, "queryOutput");
QuerySummary effectiveQuery = applyDelta();
PostQuerySyncData postQuerySyncData = newState.setQuerySummary(effectiveQuery).build();
BuildGraphData graph = queryParser.parse(effectiveQuery);
BuildGraphData graph = queryParser.create(effectiveQuery).parse();
ProjectProto.Project project = graphToProjectConverter.createProject(graph);
return BlazeProjectSnapshot.builder()
.queryData(postQuerySyncData)
Expand Down
Loading

0 comments on commit 404e0a1

Please sign in to comment.