Skip to content

Commit

Permalink
[Skymeld] Gracefully handle --explain.
Browse files Browse the repository at this point in the history
At the moment --explain is incompatible with Skymeld. It's quite rarely used so it makes sense to gracefully handle it now and maybe revisit in the future.

PiperOrigin-RevId: 521435703
Change-Id: I6b1f133da9425afecbf0ec735d4e6164861f7b65
  • Loading branch information
joeleba authored and copybara-github committed Apr 3, 2023
1 parent 842d23e commit 83c954d
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ boolean determineIfMergingAnalysisExecution(CommandEnvironment env) {
PathPackageLocator packageLocator = env.getPackageLocator();
BuildRequestOptions buildRequestOptions =
env.getOptions().getOptions(BuildRequestOptions.class);
boolean plainValueFromFlag = getPlainValueFromFlag(buildRequestOptions);
// --nobuild means no execution will be carried out, hence it doesn't make sense to interleave
// analysis and execution in that case and --experimental_merged_skyframe_analysis_execution
// should be ignored.
// Aquery and Cquery implicitly set --nobuild, so there's no need to have a warning here: it
// makes no different from the users' perspective.
if (buildRequestOptions != null
&& buildRequestOptions.mergedSkyframeAnalysisExecutionDoNotUseDirectly
if (plainValueFromFlag
&& !buildRequestOptions.performExecutionPhase
&& !(commandName.equals("aquery") || commandName.equals("cquery"))) {
env.getReporter()
Expand All @@ -46,7 +46,15 @@ boolean determineIfMergingAnalysisExecution(CommandEnvironment env) {
"--experimental_merged_skyframe_analysis_execution is incompatible with --nobuild"
+ " and will be ignored."));
}
boolean plainValueFromFlag = getPlainValueFromFlag(buildRequestOptions);
// TODO(b/245922903): Make --explain compatible with Skymeld.
if (plainValueFromFlag && buildRequestOptions.explanationPath != null) {
env.getReporter()
.handle(
Event.warn(
"--experimental_merged_skyframe_analysis_execution is incompatible with --explain"
+ " and will be ignored."));
}

boolean havingMultiPackagePath =
packageLocator != null && packageLocator.getPathEntries().size() > 1;
// TODO(b/246324830): Skymeld and multi-package_path are incompatible.
Expand All @@ -61,6 +69,7 @@ boolean determineIfMergingAnalysisExecution(CommandEnvironment env) {
}
return plainValueFromFlag
&& buildRequestOptions.performExecutionPhase
&& buildRequestOptions.explanationPath == null
&& !havingMultiPackagePath;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -475,10 +475,22 @@ public void targetWithNoConfiguration_success() throws Exception {
assertThat(result.getSuccess()).isTrue();
}

@Test
public void explain_ignoreSkymeldWithWarning() throws Exception {
addOptions("--explain=/dev/null");
write("foo/BUILD", "genrule(name = 'foo', outs = ['foo.out'], cmd = 'touch $@')");
BuildResult buildResult = buildTarget("//foo");

assertThat(buildResult.getSuccess()).isTrue();

events.assertContainsWarning(
"--experimental_merged_skyframe_analysis_execution is incompatible with --explain");
events.assertContainsWarning("and will be ignored.");
}

@Test
public void multiplePackagePath_ignoreSkymeldWithWarning() throws Exception {
write("foo/BUILD", "genrule(name = 'foo', outs = ['foo.out'], cmd = 'touch $@')");
// write("foo/root.sh");
write("otherroot/bar/BUILD", "genrule(name = 'bar', outs = ['bar.out'], cmd = 'touch $@')");
addOptions("--package_path=%workspace%:otherroot");

Expand Down

0 comments on commit 83c954d

Please sign in to comment.