From 938e34823206a2644d538ba655d20ac553352975 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 28 Feb 2023 18:34:17 +0100 Subject: [PATCH] [6.1.0] Rerun the artifact conflict check when --incompatible_strict_conflict_checks changes. (#17592) Related to #16729. PiperOrigin-RevId: 511432374 Change-Id: I00b0bff5731a3468bf0a56c4a44e95590da7b463 Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com> --- .../build/lib/analysis/AnalysisOptions.java | 11 ---- .../google/devtools/build/lib/analysis/BUILD | 1 + .../build/lib/analysis/BuildView.java | 5 +- .../lib/analysis/config/CoreOptions.java | 12 +++++ .../buildtool/OutputArtifactConflictTest.java | 50 +++++++++++++++++++ 5 files changed, 66 insertions(+), 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 index f7cc3967b0b436..95256a0549fd24 100644 --- 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 index 6c85fd43b93db5..c8b032e3eb9730 100644 --- 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 index e3a43f6f611467..c4208b0a9325c0 100644 --- 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 index 3a75df94bfc999..d028ad81e253a9 100644 --- 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 index 3112cf56f248b6..1144dfdbb137bd 100644 --- 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()); + } }