Skip to content

Commit 1e2a8ea

Browse files
committed
Add --experimental_check_non_output_external_files option
This option disables the code path that checks so-called "non-output external" files. These are files outside of roots that Bazel tracks; they occur when outputs/sources/repos symlink from the host. Addresses #25954. RELNOTES: File change checks for non-output external files (host files and embedded tools) can now be disabled with the `--experimental_check_non_output_external_files` flag.
1 parent 17c9949 commit 1e2a8ea

File tree

3 files changed

+35
-11
lines changed

3 files changed

+35
-11
lines changed

src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,15 @@ public ParallelismConverter() throws OptionsParsingException {
188188
+ "previous run's cache.")
189189
public boolean checkOutputFiles;
190190

191+
@Option(
192+
name = "experimental_check_non_output_external_files",
193+
defaultValue = "true",
194+
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
195+
effectTags = {OptionEffectTag.UNKNOWN},
196+
help =
197+
"Check for modifications made to the non-output external files (install root, host files)")
198+
public boolean checkNonOutputExternalFiles;
199+
191200
/** A converter from strings containing comma-separated names of packages to lists of strings. */
192201
public static class CommaSeparatedPackageNameListConverter
193202
extends Converter.Contextless<List<PackageIdentifier>> {

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3445,17 +3445,21 @@ public boolean hasDiffAwareness() {
34453445
return diffAwarenessManager != null;
34463446
}
34473447

3448+
@VisibleForTesting
3449+
public void handleDiffsForTesting(ExtendedEventHandler eventHandler) throws InterruptedException, AbruptExitException {
3450+
handleDiffsForTesting(eventHandler, Options.getDefaults(PackageOptions.class));
3451+
}
3452+
34483453
/** Uses diff awareness on all the package paths to invalidate changed files. */
34493454
@VisibleForTesting
3450-
public void handleDiffsForTesting(ExtendedEventHandler eventHandler)
3455+
public void handleDiffsForTesting(ExtendedEventHandler eventHandler, PackageOptions packageOptions)
34513456
throws InterruptedException, AbruptExitException {
34523457
if (lastAnalysisDiscarded) {
34533458
// Values were cleared last build, but they couldn't be deleted because they were needed for
34543459
// the execution phase. We can delete them now.
34553460
dropConfiguredTargetsNow(eventHandler);
34563461
lastAnalysisDiscarded = false;
34573462
}
3458-
PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
34593463
packageOptions.checkOutputFiles = false;
34603464
ClassToInstanceMap<OptionsBase> options =
34613465
ImmutableClassToInstanceMap.of(PackageOptions.class, packageOptions);
@@ -3558,12 +3562,14 @@ protected WorkspaceInfoFromDiff handleDiffs(
35583562
}
35593563
RepositoryOptions repoOptions = options.getOptions(RepositoryOptions.class);
35603564
try (SilentCloseable c = Profiler.instance().profile("handleDiffsWithMissingDiffInformation")) {
3565+
PackageOptions packageOptions = options.getOptions(PackageOptions.class);
35613566
handleDiffsWithMissingDiffInformation(
35623567
eventHandler,
35633568
tsgm,
35643569
pathEntriesWithoutDiffInformation,
3565-
options.getOptions(PackageOptions.class).checkOutputFiles,
3566-
repoOptions != null && repoOptions.checkExternalRepositoryFiles,
3570+
packageOptions.checkOutputFiles,
3571+
repoOptions != null && repoOptions.checkExternalRepositoryFiles,
3572+
packageOptions.checkNonOutputExternalFiles,
35673573
fsvcThreads);
35683574
}
35693575
handleClientEnvironmentChanges();
@@ -3640,6 +3646,7 @@ protected void handleDiffsWithMissingDiffInformation(
36403646
Set<Pair<Root, ProcessableModifiedFileSet>> pathEntriesWithoutDiffInformation,
36413647
boolean checkOutputFiles,
36423648
boolean checkExternalRepositoryFiles,
3649+
boolean checkNonoutputExternalFiles,
36433650
int fsvcThreads)
36443651
throws InterruptedException, AbruptExitException {
36453652

@@ -3692,8 +3699,8 @@ protected void handleDiffsWithMissingDiffInformation(
36923699
if (checkExternalRepositoryFiles) {
36933700
fileTypesToCheck = EnumSet.of(FileType.EXTERNAL_REPO);
36943701
}
3695-
if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen
3696-
|| !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
3702+
if (checkNonoutputExternalFiles && (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen
3703+
|| !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty())) {
36973704
fileTypesToCheck.add(FileType.EXTERNAL);
36983705
}
36993706
// See the comment for FileType.OUTPUT for why we need to consider output files here.
@@ -3727,8 +3734,8 @@ protected void handleDiffsWithMissingDiffInformation(
37273734
// once an external file gets into the Skyframe graph, we'll overly-conservatively always
37283735
// think the graph needs to be scanned.
37293736
externalFilesHelper.setExternalFilesKnowledge(
3730-
tmpExternalFilesHelper.getExternalFilesKnowledge());
3731-
} else if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
3737+
tmpExternalFilesHelper.getExternalFilesKnowledge());
3738+
} else if (checkNonoutputExternalFiles && !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
37323739
logger.atInfo().log(
37333740
"About to scan %d external files",
37343741
externalFilesKnowledge.nonOutputExternalFilesSeen.size());

src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -406,8 +406,13 @@ def b():
406406
tester.addSymlink("a/b.bzl", "/b.bzl");
407407
tester.sync();
408408
tester.getTarget("//a:BUILD");
409+
PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
410+
packageOptions.checkNonOutputExternalFiles = false;
409411
tester.modifyFile("/b.bzl", "ERROR ERROR");
410-
tester.sync();
412+
tester.syncWithOptions(packageOptions);
413+
tester.getTarget("//a:BUILD");
414+
packageOptions.checkNonOutputExternalFiles = true;
415+
tester.syncWithOptions(packageOptions);
411416

412417
assertThrows(NoSuchThingException.class, () -> tester.getTarget("//a:BUILD"));
413418
}
@@ -618,10 +623,13 @@ private ModifiedFileSet getModifiedFileSet() {
618623
}
619624

620625
void sync() throws InterruptedException, AbruptExitException {
626+
syncWithOptions(Options.getDefaults(PackageOptions.class));
627+
}
628+
629+
void syncWithOptions(PackageOptions packageOptions) throws InterruptedException, AbruptExitException {
621630
clock.advanceMillis(1);
622631

623632
modifiedFileSet = getModifiedFileSet();
624-
PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
625633
packageOptions.defaultVisibility = RuleVisibility.PUBLIC;
626634
packageOptions.showLoadingProgress = true;
627635
packageOptions.globbingThreads = 7;
@@ -642,7 +650,7 @@ void sync() throws InterruptedException, AbruptExitException {
642650
skyframeExecutor.invalidateFilesUnderPathForTesting(
643651
new Reporter(new EventBus()), modifiedFileSet, Root.fromPath(workspace));
644652
((SequencedSkyframeExecutor) skyframeExecutor)
645-
.handleDiffsForTesting(new Reporter(new EventBus()));
653+
.handleDiffsForTesting(new Reporter(new EventBus()), packageOptions);
646654

647655
changes.clear();
648656
}

0 commit comments

Comments
 (0)