Skip to content

Commit

Permalink
[Incremental import] Auto target alias expansion (#401)
Browse files Browse the repository at this point in the history
### Problem

Previously with incremental import, target aliases `target` and `alias` will get counted as one level, causing user having to expand more levels.

### Solution

This patch adds the mechanism that if a target alias is included in the query, it automatically adds its dependencies into the final results.
  • Loading branch information
wisechengyi authored Apr 15, 2019
1 parent 5726838 commit 2df4d62
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package com.twitter.intellij.pants.model;

import com.google.common.annotations.VisibleForTesting;
import com.intellij.openapi.util.text.StringUtil;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand Down Expand Up @@ -79,6 +80,7 @@ public String getInternalPantsTargetType() {
return pants_target_type;
}

@VisibleForTesting
public void setPantsTargetType(@NotNull String type) {
pants_target_type = type;
}
Expand All @@ -96,4 +98,6 @@ public boolean isPython() {
public boolean isJarLibrary() {
return StringUtil.equals("jar_library", getInternalPantsTargetType());
}

public boolean isTargetAlias() { return pants_target_type != null && (pants_target_type.equals("alias") || pants_target_type.equals("target"));}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
package com.twitter.intellij.pants.service.project.model.graph;


import com.google.common.collect.Queues;
import com.google.common.collect.Sets;
import com.intellij.openapi.diagnostic.Logger;
import com.twitter.intellij.pants.PantsException;
import com.twitter.intellij.pants.service.project.model.TargetInfo;

import java.util.ArrayDeque;
import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -89,18 +91,36 @@ public int getMaxDepth() {
}
}

private Set<BuildGraphNode> expandAliasTargets(Set<BuildGraphNode> initialTargets) {
Set<BuildGraphNode> results = Sets.newHashSet();
ArrayDeque<BuildGraphNode> q = Queues.newArrayDeque(initialTargets);
while (!q.isEmpty()) {
BuildGraphNode curr = q.pop();
if (curr.isAliasTarget()) {
q.addAll(curr.getDependencies());
}
else {
results.add(curr);
}
}
return results;
}

// level 0 - target roots
// level 1 - target roots + direct deps
// ...
public Set<BuildGraphNode> getNodesUpToLevel(int level) {
// Holds the current scope of build graph.
Set<BuildGraphNode> results = getTargetRoots();
results.addAll(expandAliasTargets(results));

for (int i = 0; i < level; i++) {
Set<BuildGraphNode> dependencies = new HashSet<>();
for (BuildGraphNode node : results) {
dependencies.addAll(node.getDependencies());
}
results.addAll(dependencies);
results.addAll(expandAliasTargets(dependencies));
// All nodes are in, no need to iterate more.
if (results.size() == allNodes.size()) {
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ public boolean isTargetRoot() {
return myTargetInfo.getAddressInfos().stream().anyMatch(TargetAddressInfo::isTargetRoot);
}

public boolean isAliasTarget() {
return myTargetInfo.getAddressInfos().stream().anyMatch(TargetAddressInfo::isTargetAlias);
}

public void addDependency(BuildGraphNode node) {
myDependencies.add(node);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,14 +104,71 @@ public void testOrphan() throws Exception {
}
}

private void injectTargetInfo(
public void testTargetAliasExpansion1() {
// a -> b
injectTargetInfoWithInternalPantsTargetType(targets, "a", "source", "target", IS_TARGET_ROOT, Optional.empty());
injectTargetInfoWithInternalPantsTargetType(targets, "b", "source", "java_library", !IS_TARGET_ROOT, Optional.of("a"));
// a (root) -> b, level 1
assertEquals(1, new BuildGraph(targets).getMaxDepth());
// because 'a' is an alias, so its deps will automatically get expanded if 'a' is included.
assertEquals(2, new BuildGraph(targets).getNodesUpToLevel(0).size());
}

public void testTargetAliasExpansion2() {
// a -> b
injectTargetInfoWithInternalPantsTargetType(targets, "a", "source", "alias", IS_TARGET_ROOT, Optional.empty());
injectTargetInfoWithInternalPantsTargetType(targets, "b", "source", "java_library", !IS_TARGET_ROOT, Optional.of("a"));
// a (root) -> b, level 1
assertEquals(1, new BuildGraph(targets).getMaxDepth());
// because 'a' is an alias, so its deps will automatically get expanded if 'a' is included.
assertEquals(2, new BuildGraph(targets).getNodesUpToLevel(0).size());
}

public void testTargetAliasExpansion3() {
// a -> b
injectTargetInfoWithInternalPantsTargetType(targets, "a", "source", "java_library", IS_TARGET_ROOT, Optional.empty());
injectTargetInfoWithInternalPantsTargetType(targets, "b", "source", "target", !IS_TARGET_ROOT, Optional.of("a"));
// a (root) -> b, level 1
assertEquals(1, new BuildGraph(targets).getMaxDepth());
assertEquals(1, new BuildGraph(targets).getNodesUpToLevel(0).size());
assertEquals(2, new BuildGraph(targets).getNodesUpToLevel(1).size());
}

public void testTargetAliasExpansion4() {
// a -> b -> c
// b -> d
injectTargetInfoWithInternalPantsTargetType(targets, "a", "source", "java_library", IS_TARGET_ROOT, Optional.empty());
injectTargetInfoWithInternalPantsTargetType(targets, "b", "source", "target", !IS_TARGET_ROOT, Optional.of("a"));
injectTargetInfoWithInternalPantsTargetType(targets, "c", "source", "java_library", !IS_TARGET_ROOT, Optional.of("b"));
injectTargetInfoWithInternalPantsTargetType(targets, "d", "source", "java_library", !IS_TARGET_ROOT, Optional.of("b"));
// a (root) -> b, level 1
assertEquals(2, new BuildGraph(targets).getMaxDepth());
// depth 0 only has target 'a'
assertEquals(1, new BuildGraph(targets).getNodesUpToLevel(0).size());
// depth 1 initially has 'a' & 'b', but because 'b' is an alias, its deps 'c' & 'd' will be added'
assertEquals(4, new BuildGraph(targets).getNodesUpToLevel(1).size());
}

private void injectTargetInfoWithInternalPantsTargetType(
Map<String, TargetInfo> targets,
String targetAddress,
String targetType,
String internalPantsTargetType,
boolean is_target_root,
Optional<String> dependee
) {
TargetInfo info = injectTargetInfo(targets, targetAddress, targetType, is_target_root, dependee);
info.getAddressInfos().stream().forEach(s -> s.setPantsTargetType(internalPantsTargetType));
}

private TargetInfo injectTargetInfo(
Map<String, TargetInfo> targets,
String targetAddress,
String type,
String targetType,
boolean is_target_root,
Optional<String> dependee
) {
TargetInfo source = TargetInfoTest.createTargetInfoWithTargetAddressInfo(type);
TargetInfo source = TargetInfoTest.createTargetInfoWithTargetAddressInfo(targetType);
source.getAddressInfos().forEach(s -> s.setIsTargetRoot(is_target_root));

//getTarget here is actually getting dependencies :/
Expand All @@ -124,5 +181,6 @@ private void injectTargetInfo(
}

targets.put(targetAddress, source);
return source;
}
}

0 comments on commit 2df4d62

Please sign in to comment.