Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix manual targets sync for target explicitly specified #5458

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Fix manual targets sync for target explicitly specified #5458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
561d1d7
Check to see if we can exclude manual tag based on allow_manual_targe…
rogerhu Oct 11, 2023
6808626
Enable shard sync when we also specify manual targets
rogerhu Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/src/com/google/idea/blaze/base/sync/SyncProjectTargetsHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private static boolean shouldDeriveSyncTargetsFromDirectories(ProjectViewSet vie
return viewSet.getScalarValue(AutomaticallyDeriveTargetsSection.KEY).orElse(false);
}

private static boolean shouldSyncManualTargets(ProjectViewSet viewSet) {
public static boolean shouldSyncManualTargets(ProjectViewSet viewSet) {
return viewSet.getScalarValue(SyncManualTargetsSection.KEY).orElse(false);
}

Expand Down
5 changes: 4 additions & 1 deletion base/src/com/google/idea/blaze/base/sync/sharding/BlazeBuildTargetSharder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.idea.blaze.base.projectview.ProjectViewManager;
import com.google.idea.blaze.base.projectview.ProjectViewSet;
import com.google.idea.blaze.base.projectview.section.sections.ShardBlazeBuildsSection;
import com.google.idea.blaze.base.projectview.section.sections.SyncManualTargetsSection;
import com.google.idea.blaze.base.projectview.section.sections.TargetShardSizeSection;
import com.google.idea.blaze.base.scope.BlazeContext;
import com.google.idea.blaze.base.scope.Scope;
Expand Down Expand Up @@ -87,7 +88,9 @@ static boolean shardingRequested(Project project) {
}

private static boolean shardingRequested(ProjectViewSet projectViewSet) {
return projectViewSet.getScalarValue(ShardBlazeBuildsSection.KEY).orElse(false);
// We need to perform expansion of query targets if we are to allow for manual targets to be synced.
return projectViewSet.getScalarValue(ShardBlazeBuildsSection.KEY).orElse(false) ||
projectViewSet.getScalarValue(SyncManualTargetsSection.KEY).orElse(false);
}

/** Number of individual targets per blaze build shard. */
Expand Down
6 changes: 4 additions & 2 deletions base/src/com/google/idea/blaze/base/sync/sharding/WildcardTargetExpander.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.idea.blaze.base.scope.scopes.TimingScope;
import com.google.idea.blaze.base.scope.scopes.TimingScope.EventType;
import com.google.idea.blaze.base.settings.Blaze;
import com.google.idea.blaze.base.sync.SyncProjectTargetsHelper;
import com.google.idea.blaze.base.sync.aspects.BuildResult;
import com.google.idea.blaze.base.sync.aspects.BuildResult.Status;
import com.google.idea.blaze.base.sync.projectview.LanguageSupport;
Expand All @@ -64,6 +65,7 @@
/** Expands wildcard target patterns into individual blaze targets. */
public class WildcardTargetExpander {

public static final String MANUAL_EXCLUDE_TAG = "^((?!manual).)*$";
private static final BoolExperiment filterByRuleType =
new BoolExperiment("blaze.build.filter.by.rule.type", true);

Expand Down Expand Up @@ -195,7 +197,7 @@ private static boolean excludeManualTargets(
BlazeCommandName.BUILD,
context,
BlazeInvocationContext.SYNC_CONTEXT)
.contains("--build_manual_tests");
.contains("--build_manual_tests") && !SyncProjectTargetsHelper.shouldSyncManualTargets(projectView);
}

/** Runs a blaze query to expand the input target patterns to individual blaze targets. */
Expand Down Expand Up @@ -272,7 +274,7 @@ private static String queryString(List<TargetExpression> targets, boolean exclud
return targetList;
}
return excludeManualTargets
? String.format("attr('tags', '^((?!manual).)*$', %s)", targetList)
? String.format("attr('tags', '%s', %s)", MANUAL_EXCLUDE_TAG, targetList)
: targetList;
}
}
101 changes: 93 additions & 8 deletions ...tests/unittests/com/google/idea/blaze/base/sync/sharding/BlazeBuildTargetSharderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
import static junit.framework.TestCase.assertFalse;
import static junit.framework.TestCase.assertTrue;
import static junit.framework.TestCase.fail;

import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -55,6 +57,7 @@
import com.google.idea.blaze.base.projectview.ProjectViewSet;
import com.google.idea.blaze.base.projectview.section.ScalarSection;
import com.google.idea.blaze.base.projectview.section.sections.ShardBlazeBuildsSection;
import com.google.idea.blaze.base.projectview.section.sections.SyncManualTargetsSection;
import com.google.idea.blaze.base.projectview.section.sections.TargetShardSizeSection;
import com.google.idea.blaze.base.scope.BlazeContext;
import com.google.idea.blaze.base.scope.BlazeScope;
Expand Down Expand Up @@ -267,7 +270,7 @@ public void shardTargetsRetainingOrdering_testShardWithOnlyExcludedTargetsIsDrop
public void expandAndShardTargets_shardingApproachPartitionWithoutExpanding() {
List<TargetExpression> targets = ImmutableList.of(target("//java/com/google:foo"));
ShardedTargetsResult result =
expandAndShardTargets(SyncStrategy.SERIAL, ProjectView.builder().build(), targets);
expandAndShardTargets(SyncStrategy.SERIAL, ProjectView.builder().build(), targets, fakeWildCardTargetExpanderBlazeCommandRunner);

assertThat(result.buildResult.exitCode).isEqualTo(0);
assertThat(result.shardedTargets.shardStats.shardingApproach())
Expand All @@ -281,7 +284,7 @@ public void expandAndShardTargets_remoteBuild_buildBatchingServiceIsUsed() {
.setFailToBatchTarget(false);
List<TargetExpression> targets = ImmutableList.of(target("//java/com/google:foo"));
ShardedTargetsResult result =
expandAndShardTargets(SyncStrategy.PARALLEL, ProjectView.builder().build(), targets);
expandAndShardTargets(SyncStrategy.PARALLEL, ProjectView.builder().build(), targets, fakeWildCardTargetExpanderBlazeCommandRunner);

assertThat(result.buildResult.exitCode).isEqualTo(0);
assertThat(result.shardedTargets.shardStats.shardingApproach())
Expand All @@ -301,7 +304,7 @@ public void expandAndShardTargets_localBuild_buildBatchingServiceIsUsed() {
.add(ScalarSection.builder(ShardBlazeBuildsSection.KEY).set(true))
.add(ScalarSection.builder(TargetShardSizeSection.KEY).set(500))
.build(),
targets);
targets, fakeWildCardTargetExpanderBlazeCommandRunner);

assertThat(result.buildResult.exitCode).isEqualTo(0);
ShardStats shardStats = result.shardedTargets.shardStats;
Expand All @@ -323,7 +326,7 @@ public void expandAndShardTargets_failToExpand_shardingApproachError() {
.add(ScalarSection.builder(ShardBlazeBuildsSection.KEY).set(true))
.add(ScalarSection.builder(TargetShardSizeSection.KEY).set(500))
.build(),
targets);
targets, fakeWildCardTargetExpanderBlazeCommandRunner);

assertThat(result.buildResult.exitCode).isEqualTo(BuildResult.FATAL_ERROR.exitCode);
assertThat(result.shardedTargets.shardStats.shardingApproach())
Expand All @@ -344,7 +347,7 @@ public void expandAndShardTargets_failToBatchingTargets_shardingApproachError()
.add(ScalarSection.builder(ShardBlazeBuildsSection.KEY).set(true))
.add(ScalarSection.builder(TargetShardSizeSection.KEY).set(500))
.build(),
targets);
targets, fakeWildCardTargetExpanderBlazeCommandRunner);

assertThat(result.buildResult.exitCode).isEqualTo(0);
assertThat(result.shardedTargets.shardStats.shardingApproach())
Expand Down Expand Up @@ -372,7 +375,7 @@ public void expandAndShardTargets_expandWildcardTargets() {
.add(ScalarSection.builder(ShardBlazeBuildsSection.KEY).set(true))
.add(ScalarSection.builder(TargetShardSizeSection.KEY).set(500))
.build(),
targets);
targets, fakeWildCardTargetExpanderBlazeCommandRunner);

ShardStats shardStats = result.shardedTargets.shardStats;
assertThat(shardStats.suggestedTargetSizePerShard()).isEqualTo(500);
Expand All @@ -381,8 +384,90 @@ public void expandAndShardTargets_expandWildcardTargets() {
.containsExactly(ImmutableList.of(target(expectedLabel1), target(expectedLabel2)));
}

@Test
public void expandAndShardTargets_expandWildcardTargetsNoExcludeManualTag() {
String expectedLabel1 = "//java/com/google:one";
FakeWildCardTargetExpanderBlazeCommandRunner commandRunner = new FakeWildCardTargetExpanderBlazeCommandRunner() {

@Override
public InputStream runQuery(Project project, BlazeCommand.Builder blazeCommandBuilder, BuildResultHelper buildResultHelper, BlazeContext context) throws BuildException {
// We need to confirm within the query runner because there are no public methods currently to
// perform this check downstream.
for (String argument : blazeCommandBuilder.build().toArgumentList()) {
assertFalse(argument.contains(WildcardTargetExpander.MANUAL_EXCLUDE_TAG));
}
return super.runQuery(project, blazeCommandBuilder, buildResultHelper, context);
}
};

fakeWildCardTargetExpanderExternalTaskProvider
.setReturnVal(0)
.setOutputMessage("sh_library rule " + expectedLabel1);
commandRunner.setOutputMessages(
ImmutableList.of("sh_library rule " + expectedLabel1));
fakeBuildBatchingService
.setShardingApproach(ShardingApproach.LEXICOGRAPHIC_TARGET_SHARDER)
.setFailToBatchTarget(false);

List<TargetExpression> targets = ImmutableList.of(target("//java/com/google/..."));
ProjectView projectView = ProjectView.builder()
.add(ScalarSection.builder(SyncManualTargetsSection.KEY).set(true))
.add(ScalarSection.builder(ShardBlazeBuildsSection.KEY).set(true))
.add(ScalarSection.builder(TargetShardSizeSection.KEY).set(500))
.build();

ShardedTargetsResult result =
expandAndShardTargets(
SyncStrategy.PARALLEL,
projectView,
targets, commandRunner);

assertThat(result.shardedTargets.shardedTargets)
.containsExactly(ImmutableList.of(target(expectedLabel1)));
}

@Test
public void expandAndShardTargets_expandWildcardTargetsIncludesManualTag() {
String expectedLabel1 = "//java/com/google:one";
FakeWildCardTargetExpanderBlazeCommandRunner commandRunner = new FakeWildCardTargetExpanderBlazeCommandRunner() {

@Override
public InputStream runQuery(Project project, BlazeCommand.Builder blazeCommandBuilder, BuildResultHelper buildResultHelper, BlazeContext context) throws BuildException {
// We need to confirm within the query runner because there are no public methods currently to
// perform this check downstream.
assertTrue(blazeCommandBuilder.build().toArgumentList().stream().anyMatch(argument -> argument.contains(WildcardTargetExpander.MANUAL_EXCLUDE_TAG)));
return super.runQuery(project, blazeCommandBuilder, buildResultHelper, context);
}
};

fakeWildCardTargetExpanderExternalTaskProvider
.setReturnVal(0)
.setOutputMessage("sh_library rule " + expectedLabel1);
commandRunner.setOutputMessages(
ImmutableList.of("sh_library rule " + expectedLabel1));
fakeBuildBatchingService
.setShardingApproach(ShardingApproach.LEXICOGRAPHIC_TARGET_SHARDER)
.setFailToBatchTarget(false);

List<TargetExpression> targets = ImmutableList.of(target("//java/com/google/..."));
ProjectView projectView = ProjectView.builder()
.add(ScalarSection.builder(SyncManualTargetsSection.KEY).set(false))
.add(ScalarSection.builder(ShardBlazeBuildsSection.KEY).set(true))
.add(ScalarSection.builder(TargetShardSizeSection.KEY).set(500))
.build();

ShardedTargetsResult result =
expandAndShardTargets(
SyncStrategy.PARALLEL,
projectView,
targets, commandRunner);

assertThat(result.shardedTargets.shardedTargets)
.containsExactly(ImmutableList.of(target(expectedLabel1)));
}

private ShardedTargetsResult expandAndShardTargets(
SyncStrategy syncStrategy, ProjectView projectView, List<TargetExpression> targets) {
SyncStrategy syncStrategy, ProjectView projectView, List<TargetExpression> targets, FakeBlazeCommandRunner commandRunner) {
WorkspaceRoot workspaceRoot = new WorkspaceRoot(new File("workspaceRoot"));
return BlazeBuildTargetSharder.expandAndShardTargets(
getProject(),
Expand All @@ -392,7 +477,7 @@ private ShardedTargetsResult expandAndShardTargets(
targets,
FakeBuildInvoker.builder()
.type(BuildBinaryType.BAZEL)
.commandRunner(fakeWildCardTargetExpanderBlazeCommandRunner)
.commandRunner(commandRunner)
.build(),
syncStrategy);
}
Expand Down
Loading