Rerun the artifact conflict check when --incompatible_strict_conflict…

…_checks changes. Related to #16729. PiperOrigin-RevId: 511432374 Change-Id: I00b0bff5731a3468bf0a56c4a44e95590da7b463
bazelbuild · Feb 22, 2023 · cb8db3b · cb8db3b
1 parent 474fc18
commit cb8db3b
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 13 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
@@ -90,17 +90,6 @@ public class AnalysisOptions extends OptionsBase {
       help = "Deprecated. No-op.")
   public boolean skyframePrepareAnalysis;
 
-  @Option(
-      name = "incompatible_strict_conflict_checks",
-      oldName = "experimental_strict_conflict_checks",
-      defaultValue = "false",
-      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-      metadataTags = OptionMetadataTag.INCOMPATIBLE_CHANGE,
-      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
-      help =
-          "Check for action prefix file path conflicts, regardless of action-specific overrides.")
-  public boolean strictConflictChecks;
-
   @Option(
       name = "experimental_skyframe_cpu_heavy_skykeys_thread_pool_size",
       defaultValue = "HOST_CPUS",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -604,6 +604,7 @@ java_library(
         ":blaze_directories",
         ":config/build_configuration",
         ":config/build_options",
+        ":config/core_options",
         ":config/invalid_configuration_exception",
         ":configured_target",
         ":constraints/platform_restrictions_result",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -44,6 +44,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigurationResolver.TopLevelTargetsAndConfigsResult;
+import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.constraints.PlatformRestrictionsResult;
 import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
@@ -424,7 +425,7 @@ public AnalysisResult update(
                     getCoverageArtifactsHelper(
                         configuredTargets, allTargetsToTest, eventHandler, eventBus, loadingResult),
                 keepGoing,
-                viewOptions.strictConflictChecks,
+                targetOptions.get(CoreOptions.class).strictConflictChecks,
                 checkForActionConflicts,
                 executors,
                 /* shouldDiscardAnalysisCache= */ viewOptions.discardAnalysisCache
@@ -442,7 +443,7 @@ public AnalysisResult update(
                 bugReporter,
                 keepGoing,
                 executors,
-                viewOptions.strictConflictChecks,
+                targetOptions.get(CoreOptions.class).strictConflictChecks,
                 checkForActionConflicts);
         setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
       }

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -173,6 +173,17 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
               + "disabled.")
   public boolean strictFilesets;
 
+  @Option(
+      name = "incompatible_strict_conflict_checks",
+      oldName = "experimental_strict_conflict_checks",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      metadataTags = OptionMetadataTag.INCOMPATIBLE_CHANGE,
+      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+      help =
+          "Check for action prefix file path conflicts, regardless of action-specific overrides.")
+  public boolean strictConflictChecks;
+
   // This option is only used during execution. However, it is a required input to the analysis
   // phase, as otherwise flipping this flag would not invalidate already-executed actions.
   @Option(
@@ -988,6 +999,7 @@ public FragmentOptions getExec() {
     exec.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles;
     exec.useAutoExecGroups = useAutoExecGroups;
     exec.experimentalWritableOutputs = experimentalWritableOutputs;
+    exec.strictConflictChecks = strictConflictChecks;
 
     // === Runfiles ===
     exec.buildRunfilesManifests = buildRunfilesManifests;

diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java
@@ -752,4 +752,54 @@ public void conflict_noTrackIncrementalState_detected(
     events.assertContainsError(
         "file 'foo/conflict_output' is generated by these conflicting actions:");
   }
+
+  private void setupStrictConflictChecksTest() throws IOException {
+    write(
+        "foo/conflict.bzl",
+        "def _impl(ctx):",
+        "  dir = ctx.actions.declare_directory(ctx.label.name + '.dir')",
+        "  file = ctx.actions.declare_file(ctx.label.name + '.dir/file.txt')",
+        "  ctx.actions.run_shell(",
+        "    outputs = [dir, file],",
+        "    command = 'mkdir -p $1 && touch $2',",
+        "    arguments = [dir.path, file.path],",
+        "  )",
+        "  return [DefaultInfo(files = depset([dir, file]))]",
+        "",
+        "my_rule = rule(implementation = _impl)");
+    write("foo/BUILD", "load(':conflict.bzl', 'my_rule')", "my_rule(name = 'bar')");
+  }
+
+  @Test
+  public void laxFollowedByStrictConflictChecks(@TestParameter boolean mergedAnalysisExecution)
+      throws Exception {
+    setupStrictConflictChecksTest();
+    addOptions("--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution);
+
+    addOptions("--noincompatible_strict_conflict_checks");
+    buildTarget("//foo:bar");
+    assertNoEvents(events.errors());
+
+    addOptions("--incompatible_strict_conflict_checks");
+    assertThrows(ViewCreationFailedException.class, () -> buildTarget("//foo:bar"));
+    events.assertContainsError("One of the output paths");
+    events.assertContainsError("is a prefix of the other");
+  }
+
+  @Test
+  public void strictFollowedByLaxConflictChecks(@TestParameter boolean mergedAnalysisExecution)
+      throws Exception {
+    setupStrictConflictChecksTest();
+    addOptions("--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution);
+
+    addOptions("--incompatible_strict_conflict_checks");
+    assertThrows(ViewCreationFailedException.class, () -> buildTarget("//foo:bar"));
+    events.assertContainsError("One of the output paths");
+    events.assertContainsError("is a prefix of the other");
+
+    events.clear();
+    addOptions("--noincompatible_strict_conflict_checks");
+    buildTarget("//foo:bar");
+    assertNoEvents(events.errors());
+  }
 }