Skip to content

Commit 04b47aa

Browse files
hanwen-flowcopybara-github
authored andcommitted
Add --experimental_check_external_other_files option
This option disables the code path that checks EXTERNAL_OTHER files. These are files outside of roots that Bazel tracks; they occur when outputs/sources/repos symlink files from outside directories. This is useful for process migration scenarios using containers. Here ctime and inode numbers change from under the bazel server without file content changes. Addresses #25954. RELNOTES: File change checks for non-output, non-repo external files can now be disabled with the `--experimental_check_external_other_files` flag. Closes #25957. PiperOrigin-RevId: 762396407 Change-Id: I0a06356da5f287e3c6efd83bb3659326d485575a
1 parent 8868a9a commit 04b47aa

File tree

6 files changed

+78
-45
lines changed

6 files changed

+78
-45
lines changed

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

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

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

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ public SkyValueDirtinessChecker.DirtyResult check(
150150
private static boolean isCacheableType(FileType fileType) {
151151
switch (fileType) {
152152
case INTERNAL:
153-
case EXTERNAL:
153+
case EXTERNAL_OTHER:
154154
case BUNDLED:
155155
return true;
156156
case EXTERNAL_REPO:

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

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,11 @@ public class ExternalFilesHelper {
4848
// These variables are set to true from multiple threads, but only read in the main thread.
4949
// So volatility or an AtomicBoolean is not needed.
5050
private boolean anyOutputFilesSeen = false;
51-
private boolean tooManyNonOutputExternalFilesSeen = false;
51+
private boolean tooManyExternalOtherFilesSeen = false;
5252
private boolean anyFilesInExternalReposSeen = false;
5353

54-
// This is a set of external files that are not in external repositories.
55-
private Set<RootedPath> nonOutputExternalFilesSeen = Sets.newConcurrentHashSet();
54+
// This is the set of EXTERNAL_OTHER files.
55+
private Set<RootedPath> externalOtherFilesSeen = Sets.newConcurrentHashSet();
5656

5757
private ExternalFilesHelper(
5858
AtomicReference<PathPackageLocator> pkgLocator,
@@ -103,8 +103,8 @@ public enum ExternalFileAction {
103103
DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
104104

105105
/**
106-
* For paths of type {@link FileType#EXTERNAL} or {@link FileType#OUTPUT}, assume the path does
107-
* not exist and will never exist.
106+
* For paths of type {@link FileType#EXTERNAL_OTHER} or {@link FileType#OUTPUT}, assume the path
107+
* does not exist and will never exist.
108108
*/
109109
ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS,
110110
}
@@ -121,12 +121,6 @@ public enum FileType {
121121
/** A path inside the package roots. */
122122
INTERNAL,
123123

124-
/**
125-
* A non {@link #EXTERNAL_REPO} path outside the package roots about which we may make no other
126-
* assumptions.
127-
*/
128-
EXTERNAL,
129-
130124
/**
131125
* A path in Bazel's output tree that's a proper output of an action (*not* a source file in an
132126
* external repository). Such files are theoretically mutable, but certain Bazel flags may tell
@@ -153,6 +147,14 @@ public enum FileType {
153147
* RepositoryDirectoryValue is computed.
154148
*/
155149
EXTERNAL_REPO,
150+
151+
/**
152+
* None of the above. We encounter these paths when outputs, source files or external repos
153+
* symlink to files outside aforementioned Bazel-managed directories. For example, C compilation
154+
* by the host compiler may depend on /usr/bin/gcc. Bazel makes a best-effort attempt to detect
155+
* changes in such files.
156+
*/
157+
EXTERNAL_OTHER,
156158
}
157159

158160
/**
@@ -164,37 +166,37 @@ static class NonexistentImmutableExternalFileException extends Exception {
164166

165167
static class ExternalFilesKnowledge {
166168
final boolean anyOutputFilesSeen;
167-
final Set<RootedPath> nonOutputExternalFilesSeen;
169+
final Set<RootedPath> externalOtherFilesSeen;
168170
final boolean anyFilesInExternalReposSeen;
169-
final boolean tooManyNonOutputExternalFilesSeen;
171+
final boolean tooManyExternalOtherFilesSeen;
170172

171173
private ExternalFilesKnowledge(
172174
boolean anyOutputFilesSeen,
173-
Set<RootedPath> nonOutputExternalFilesSeen,
175+
Set<RootedPath> externalOtherFilesSeen,
174176
boolean anyFilesInExternalReposSeen,
175-
boolean tooManyNonOutputExternalFilesSeen) {
177+
boolean tooManyExternalOtherFilesSeen) {
176178
this.anyOutputFilesSeen = anyOutputFilesSeen;
177-
this.nonOutputExternalFilesSeen = nonOutputExternalFilesSeen;
179+
this.externalOtherFilesSeen = externalOtherFilesSeen;
178180
this.anyFilesInExternalReposSeen = anyFilesInExternalReposSeen;
179-
this.tooManyNonOutputExternalFilesSeen = tooManyNonOutputExternalFilesSeen;
181+
this.tooManyExternalOtherFilesSeen = tooManyExternalOtherFilesSeen;
180182
}
181183
}
182184

183185
@ThreadCompatible
184186
ExternalFilesKnowledge getExternalFilesKnowledge() {
185187
return new ExternalFilesKnowledge(
186188
anyOutputFilesSeen,
187-
nonOutputExternalFilesSeen,
189+
externalOtherFilesSeen,
188190
anyFilesInExternalReposSeen,
189-
tooManyNonOutputExternalFilesSeen);
191+
tooManyExternalOtherFilesSeen);
190192
}
191193

192194
@ThreadCompatible
193195
void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) {
194196
anyOutputFilesSeen = externalFilesKnowledge.anyOutputFilesSeen;
195-
nonOutputExternalFilesSeen = externalFilesKnowledge.nonOutputExternalFilesSeen;
197+
externalOtherFilesSeen = externalFilesKnowledge.externalOtherFilesSeen;
196198
anyFilesInExternalReposSeen = externalFilesKnowledge.anyFilesInExternalReposSeen;
197-
tooManyNonOutputExternalFilesSeen = externalFilesKnowledge.tooManyNonOutputExternalFilesSeen;
199+
tooManyExternalOtherFilesSeen = externalFilesKnowledge.tooManyExternalOtherFilesSeen;
198200
}
199201

200202
ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
@@ -208,11 +210,11 @@ public FileType getAndNoteFileType(RootedPath rootedPath) {
208210

209211
private Pair<FileType, RepositoryName> getFileTypeAndRepository(RootedPath rootedPath) {
210212
FileType fileType = detectFileType(rootedPath);
211-
if (FileType.EXTERNAL == fileType) {
212-
if (nonOutputExternalFilesSeen.size() >= MAX_EXTERNAL_FILES_TO_TRACK) {
213-
tooManyNonOutputExternalFilesSeen = true;
213+
if (fileType == FileType.EXTERNAL_OTHER) {
214+
if (externalOtherFilesSeen.size() >= MAX_EXTERNAL_FILES_TO_TRACK) {
215+
tooManyExternalOtherFilesSeen = true;
214216
} else {
215-
nonOutputExternalFilesSeen.add(rootedPath);
217+
externalOtherFilesSeen.add(rootedPath);
216218
}
217219
}
218220
if (FileType.EXTERNAL_REPO == fileType) {
@@ -239,7 +241,7 @@ private FileType detectFileType(RootedPath rootedPath) {
239241
// The outputBase may be null if we're not actually running a build.
240242
Path outputBase = packageLocator.getOutputBase();
241243
if (outputBase == null) {
242-
return FileType.EXTERNAL;
244+
return FileType.EXTERNAL_OTHER;
243245
}
244246
if (rootedPath.asPath().startsWith(outputBase)) {
245247
Path externalRepoDir = outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION);
@@ -249,7 +251,7 @@ private FileType detectFileType(RootedPath rootedPath) {
249251
return FileType.OUTPUT;
250252
}
251253
}
252-
return FileType.EXTERNAL;
254+
return FileType.EXTERNAL_OTHER;
253255
}
254256

255257
/**
@@ -269,7 +271,7 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment
269271
case BUNDLED:
270272
case INTERNAL:
271273
break;
272-
case EXTERNAL:
274+
case EXTERNAL_OTHER:
273275
if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) {
274276
logger.atInfo().log("Encountered an external path %s", rootedPath);
275277
}

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

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3450,17 +3450,23 @@ public boolean hasDiffAwareness() {
34503450
return diffAwarenessManager != null;
34513451
}
34523452

3453-
/** Uses diff awareness on all the package paths to invalidate changed files. */
34543453
@VisibleForTesting
34553454
public void handleDiffsForTesting(ExtendedEventHandler eventHandler)
34563455
throws InterruptedException, AbruptExitException {
3456+
handleDiffsForTesting(eventHandler, Options.getDefaults(PackageOptions.class));
3457+
}
3458+
3459+
/** Uses diff awareness on all the package paths to invalidate changed files. */
3460+
@VisibleForTesting
3461+
public void handleDiffsForTesting(
3462+
ExtendedEventHandler eventHandler, PackageOptions packageOptions)
3463+
throws InterruptedException, AbruptExitException {
34573464
if (lastAnalysisDiscarded) {
34583465
// Values were cleared last build, but they couldn't be deleted because they were needed for
34593466
// the execution phase. We can delete them now.
34603467
dropConfiguredTargetsNow(eventHandler);
34613468
lastAnalysisDiscarded = false;
34623469
}
3463-
PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
34643470
packageOptions.checkOutputFiles = false;
34653471
ClassToInstanceMap<OptionsBase> options =
34663472
ImmutableClassToInstanceMap.of(PackageOptions.class, packageOptions);
@@ -3555,12 +3561,14 @@ protected WorkspaceInfoFromDiff handleDiffs(
35553561
}
35563562
RepositoryOptions repoOptions = options.getOptions(RepositoryOptions.class);
35573563
try (SilentCloseable c = Profiler.instance().profile("handleDiffsWithMissingDiffInformation")) {
3564+
PackageOptions packageOptions = options.getOptions(PackageOptions.class);
35583565
handleDiffsWithMissingDiffInformation(
35593566
eventHandler,
35603567
tsgm,
35613568
pathEntriesWithoutDiffInformation,
3562-
options.getOptions(PackageOptions.class).checkOutputFiles,
3569+
packageOptions.checkOutputFiles,
35633570
repoOptions != null && repoOptions.checkExternalRepositoryFiles,
3571+
packageOptions.checkExternalOtherFiles,
35643572
fsvcThreads);
35653573
}
35663574
handleClientEnvironmentChanges();
@@ -3632,6 +3640,7 @@ protected void handleDiffsWithMissingDiffInformation(
36323640
Set<Pair<Root, ProcessableModifiedFileSet>> pathEntriesWithoutDiffInformation,
36333641
boolean checkOutputFiles,
36343642
boolean checkExternalRepositoryFiles,
3643+
boolean checkExternalOtherFiles,
36353644
int fsvcThreads)
36363645
throws InterruptedException, AbruptExitException {
36373646

@@ -3640,7 +3649,7 @@ protected void handleDiffsWithMissingDiffInformation(
36403649
|| (checkOutputFiles && externalFilesKnowledge.anyOutputFilesSeen)
36413650
|| (checkExternalRepositoryFiles && repositoryHelpersHolder != null)
36423651
|| (checkExternalRepositoryFiles && externalFilesKnowledge.anyFilesInExternalReposSeen)
3643-
|| externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
3652+
|| (checkExternalOtherFiles && externalFilesKnowledge.tooManyExternalOtherFilesSeen)) {
36443653
// We freshly compute knowledge of the presence of external files in the skyframe graph. We
36453654
// use a fresh ExternalFilesHelper instance and only set the real instance's knowledge *after*
36463655
// we are done with the graph scan, lest an interrupt during the graph scan causes us to
@@ -3684,9 +3693,10 @@ protected void handleDiffsWithMissingDiffInformation(
36843693
if (checkExternalRepositoryFiles) {
36853694
fileTypesToCheck = EnumSet.of(FileType.EXTERNAL_REPO);
36863695
}
3687-
if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen
3688-
|| !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
3689-
fileTypesToCheck.add(FileType.EXTERNAL);
3696+
if (checkExternalOtherFiles
3697+
&& (externalFilesKnowledge.tooManyExternalOtherFilesSeen
3698+
|| !externalFilesKnowledge.externalOtherFilesSeen.isEmpty())) {
3699+
fileTypesToCheck.add(FileType.EXTERNAL_OTHER);
36903700
}
36913701
// See the comment for FileType.OUTPUT for why we need to consider output files here.
36923702
if (checkOutputFiles) {
@@ -3720,10 +3730,10 @@ protected void handleDiffsWithMissingDiffInformation(
37203730
// think the graph needs to be scanned.
37213731
externalFilesHelper.setExternalFilesKnowledge(
37223732
tmpExternalFilesHelper.getExternalFilesKnowledge());
3723-
} else if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
3733+
} else if (checkExternalOtherFiles
3734+
&& !externalFilesKnowledge.externalOtherFilesSeen.isEmpty()) {
37243735
logger.atInfo().log(
3725-
"About to scan %d external files",
3726-
externalFilesKnowledge.nonOutputExternalFilesSeen.size());
3736+
"About to scan %d external files", externalFilesKnowledge.externalOtherFilesSeen.size());
37273737
FilesystemValueChecker fsvc =
37283738
new FilesystemValueChecker(
37293739
tsgm,
@@ -3735,7 +3745,7 @@ protected void handleDiffsWithMissingDiffInformation(
37353745
ImmutableBatchDirtyResult batchDirtyResult;
37363746
try (SilentCloseable c = Profiler.instance().profile("fsvc.getDirtyExternalKeys")) {
37373747
Map<SkyKey, SkyValue> externalDirtyNodes = new ConcurrentHashMap<>();
3738-
for (RootedPath path : externalFilesKnowledge.nonOutputExternalFilesSeen) {
3748+
for (RootedPath path : externalFilesKnowledge.externalOtherFilesSeen) {
37393749
SkyKey key = FileStateValue.key(path);
37403750
SkyValue value = memoizingEvaluator.getExistingValue(key);
37413751
if (value != null) {
@@ -3750,7 +3760,8 @@ protected void handleDiffsWithMissingDiffInformation(
37503760
batchDirtyResult =
37513761
fsvc.getDirtyKeys(
37523762
externalDirtyNodes,
3753-
new ExternalDirtinessChecker(externalFilesHelper, EnumSet.of(FileType.EXTERNAL)));
3763+
new ExternalDirtinessChecker(
3764+
externalFilesHelper, EnumSet.of(FileType.EXTERNAL_OTHER)));
37543765
}
37553766
handleChangedFiles(
37563767
ImmutableList.of(), batchDirtyResult, batchDirtyResult.getNumKeysChecked());

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -405,8 +405,13 @@ def b():
405405
tester.addSymlink("a/b.bzl", "/b.bzl");
406406
tester.sync();
407407
tester.getTarget("//a:BUILD");
408+
PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
409+
packageOptions.checkExternalOtherFiles = false;
408410
tester.modifyFile("/b.bzl", "ERROR ERROR");
409-
tester.sync();
411+
tester.syncWithOptions(packageOptions);
412+
tester.getTarget("//a:BUILD");
413+
packageOptions.checkExternalOtherFiles = true;
414+
tester.syncWithOptions(packageOptions);
410415

411416
assertThrows(NoSuchThingException.class, () -> tester.getTarget("//a:BUILD"));
412417
}
@@ -611,10 +616,14 @@ private ModifiedFileSet getModifiedFileSet() {
611616
}
612617

613618
void sync() throws InterruptedException, AbruptExitException {
619+
syncWithOptions(Options.getDefaults(PackageOptions.class));
620+
}
621+
622+
void syncWithOptions(PackageOptions packageOptions)
623+
throws InterruptedException, AbruptExitException {
614624
clock.advanceMillis(1);
615625

616626
modifiedFileSet = getModifiedFileSet();
617-
PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
618627
packageOptions.defaultVisibility = RuleVisibility.PUBLIC;
619628
packageOptions.showLoadingProgress = true;
620629
packageOptions.globbingThreads = 7;
@@ -635,7 +644,7 @@ void sync() throws InterruptedException, AbruptExitException {
635644
skyframeExecutor.invalidateFilesUnderPathForTesting(
636645
new Reporter(new EventBus()), modifiedFileSet, Root.fromPath(workspace));
637646
((SequencedSkyframeExecutor) skyframeExecutor)
638-
.handleDiffsForTesting(new Reporter(new EventBus()));
647+
.handleDiffsForTesting(new Reporter(new EventBus()), packageOptions);
639648

640649
changes.clear();
641650
}

src/test/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtilsTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ public void check_usesSyscallCache_andReturnsNewValue(
105105
? new DirtinessCheckerUtils.ExternalDirtinessChecker(
106106
externalFilesHelper,
107107
EnumSet.of(
108-
ExternalFilesHelper.FileType.INTERNAL, ExternalFilesHelper.FileType.EXTERNAL))
108+
ExternalFilesHelper.FileType.INTERNAL,
109+
ExternalFilesHelper.FileType.EXTERNAL_OTHER))
109110
: createMissingDiffChecker();
110111

111112
boolean shouldCheck = underTest.applies(rootedPath);

0 commit comments

Comments
 (0)