[6.1.0] Rerun the artifact conflict check when --incompatible_strict_conflict_checks changes. (bazelbuild#17592)

tjgq · ShreeM01 · web-flow · commit 938e34823206 · 2023-02-28T17:34:17.000Z
Related to bazelbuild#16729. PiperOrigin-RevId: 511432374 Change-Id: I00b0bff5731a3468bf0a56c4a44e95590da7b463 Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
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
@@ -610,6 +610,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
@@ -43,6 +43,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;
@@ -400,7 +401,7 @@ public AnalysisResult update(
                 bugReporter,
                 keepGoing,
                 loadingPhaseThreads,
-                viewOptions.strictConflictChecks,
+                targetOptions.get(CoreOptions.class).strictConflictChecks,
                 checkForActionConflicts,
                 viewOptions.cpuHeavySkyKeysThreadPoolSize);
         setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
@@ -426,7 +427,7 @@ public AnalysisResult update(
                 Preconditions.checkNotNull(resourceManager), // non-null for skymeld.
                 Preconditions.checkNotNull(buildResultListener), // non-null for skymeld.
                 keepGoing,
-                viewOptions.strictConflictChecks,
+                targetOptions.get(CoreOptions.class).strictConflictChecks,
                 checkForActionConflicts,
                 loadingPhaseThreads,
                 viewOptions.cpuHeavySkyKeysThreadPoolSize,
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
@@ -170,6 +170,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;
+
   @Option(
       name = "experimental_strict_fileset_output",
       defaultValue = "false",
@@ -959,6 +970,7 @@ public FragmentOptions getHost() {
     host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider;
     host.debugSelectsAlwaysSucceed = debugSelectsAlwaysSucceed;
     host.checkTestonlyForOutputFiles = checkTestonlyForOutputFiles;
+    host.strictConflictChecks = strictConflictChecks;
 
     // === Runfiles ===
     host.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
@@ -734,4 +734,54 @@ public void dependencyHasConflict_keepGoing_bothTopLevelTargetsFail(
     assertThat(eventListener.failedTargetNames)
         .containsExactly("//foo:top_level_a", "//foo:top_level_b");
   }
+
+  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());
+  }
 }
