Skip to content

Commit a28b540

Browse files
ulfjackiirina
ulfjack
authored and
iirina
committed
Fix Cpp action caching
This combines both previous changes and extends them to work both with and without kchodorow@'s rollout of the exec root rearrangement. Unfortunately, each of these changes individually breaks something somewhere, so they must all go into a single commit. Change 1: CppCompileAction must return false from inputsKnown for .d pruning This is necessary (but not sufficient) for the action cache to work correctly. Consider the following sequence of events: 1. action is executed 2. .d pruning is performed 3. action cache writes entry with post-pruning inputs list 4. action gets regenerated (e.g., due to server restart) 5. the action cache calls inputsKnown(), which returns true 6. action cache checks entry from step 3 against pre-pruning inputs list, which results in a cache miss The action cache needs to know that the current list is not the final list, so inputsKnown() in step 5 must return false if .d pruning is enabled. Change 2: Fix artifact root discovery for external artifacts The SkyframeExecutor was assuming that all exec paths were coming from the main repository. Instead, rely on external exec paths to start with "../". Additional change 3: In addition, update the PackageRootResolverWithEnvironment and the HeaderDiscovery to use the single unified repository name guessing implementation. Previously, the PackageRootResolverWithEnvironment was poisoning the source artifact cache, which then caused subsequent lookups to return a bad artifact. Add a precondition to double-check that artifacts looked up by exec path have the correct source root. For compatibility with kchodorow@'s upcoming exec root refactor, if the exec path starts with "external", then assume it's coming from an external repository. This must be removed when that change is successfully rolled out, or it will break if anyone creates a package called 'external'. Additional change 4: On top of all of that, PackageRootResolverWithEnvironment and SkyframeExecutor must use the same source root computation as the Package class itself. I extracted the corresponding code to Root, and added comments both there and in Package to clearly indicate that these methods have to always be modified in sync. Fixes bazelbuild#2490. -- PiperOrigin-RevId: 148439309 MOS_MIGRATED_REVID=148439309
1 parent e50388e commit a28b540

File tree

8 files changed

+106
-42
lines changed

8 files changed

+106
-42
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -305,17 +305,17 @@ public synchronized Artifact resolveSourceArtifactWithAncestor(
305305
// Source exec paths cannot escape the source root.
306306
return null;
307307
}
308-
Artifact artifact = sourceArtifactCache.getArtifactIfValid(execPath);
309-
if (artifact != null) {
310-
return artifact;
311-
}
312308
// Don't create an artifact if it's derived.
313309
if (isDerivedArtifact(execPath)) {
314310
return null;
315311
}
316-
317-
return createArtifactIfNotValid(
318-
findSourceRoot(execPath, baseExecPath, baseRoot, repositoryName), execPath);
312+
Root sourceRoot = findSourceRoot(execPath, baseExecPath, baseRoot, repositoryName);
313+
Artifact artifact = sourceArtifactCache.getArtifactIfValid(execPath);
314+
if (artifact != null) {
315+
Preconditions.checkState(sourceRoot == null || sourceRoot.equals(artifact.getRoot()));
316+
return artifact;
317+
}
318+
return createArtifactIfNotValid(sourceRoot, execPath);
319319
}
320320

321321
/**

‎src/main/java/com/google/devtools/build/lib/actions/Root.java‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,15 @@
1515
package com.google.devtools.build.lib.actions;
1616

1717
import com.google.common.annotations.VisibleForTesting;
18+
import com.google.devtools.build.lib.cmdline.RepositoryName;
1819
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
1920
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
2021
import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
2122
import com.google.devtools.build.lib.util.Preconditions;
2223
import com.google.devtools.build.lib.vfs.Path;
2324
import com.google.devtools.build.lib.vfs.PathFragment;
24-
2525
import java.io.Serializable;
2626
import java.util.Objects;
27-
2827
import javax.annotation.Nullable;
2928

3029
/**
@@ -57,6 +56,20 @@ public static Root asSourceRoot(Path path, boolean isMainRepo) {
5756
return new Root(null, path, false, isMainRepo);
5857
}
5958

59+
// This must always be consistent with Package.getSourceRoot; otherwise computing source roots
60+
// from exec paths does not work, which can break the action cache for input-discovering actions.
61+
public static Root computeSourceRoot(Path packageRoot, RepositoryName repository) {
62+
if (repository.isMain()) {
63+
return Root.asSourceRoot(packageRoot, true);
64+
} else {
65+
Path actualRoot = packageRoot;
66+
for (int i = 0; i < repository.getSourceRoot().segmentCount(); i++) {
67+
actualRoot = actualRoot.getParentDirectory();
68+
}
69+
return Root.asSourceRoot(actualRoot, false);
70+
}
71+
}
72+
6073
/**
6174
* testonly until {@link #asSourceRoot(Path, boolean)} is deleted.
6275
*/

‎src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,8 @@
2020
import com.google.devtools.build.lib.util.Preconditions;
2121
import com.google.devtools.build.lib.vfs.Canonicalizer;
2222
import com.google.devtools.build.lib.vfs.PathFragment;
23-
2423
import java.io.Serializable;
2524
import java.util.Objects;
26-
2725
import javax.annotation.concurrent.Immutable;
2826

2927
/**
@@ -58,6 +56,41 @@ public static PackageIdentifier createInMainRepo(PathFragment name) {
5856
return create(RepositoryName.MAIN, name);
5957
}
6058

59+
/**
60+
* Tries to infer the package identifier from the given exec path. This method does not perform
61+
* any I/O, but looks solely at the structure of the exec path. The resulting identifier may
62+
* actually be a subdirectory of a package rather than a package, e.g.:
63+
* <pre><code>
64+
* + WORKSPACE
65+
* + foo/BUILD
66+
* + foo/bar/bar.java
67+
* </code></pre>
68+
*
69+
* In this case, this method returns a package identifier for foo/bar, even though that is not a
70+
* package. Callers need to look up the actual package if needed.
71+
*
72+
* @throws LabelSyntaxException if the exec path seems to be for an external repository that doe
73+
* not have a valid repository name (see {@link RepositoryName#create})
74+
*/
75+
public static PackageIdentifier discoverFromExecPath(PathFragment execPath, boolean forFiles)
76+
throws LabelSyntaxException {
77+
Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
78+
PathFragment tofind = forFiles
79+
? Preconditions.checkNotNull(
80+
execPath.getParentDirectory(), "Must pass in files, not root directory")
81+
: execPath;
82+
if (tofind.startsWith(new PathFragment(Label.EXTERNAL_PATH_PREFIX))) {
83+
// TODO(ulfjack): Remove this when kchodorow@'s exec root rearrangement has been rolled out.
84+
RepositoryName repository = RepositoryName.create("@" + tofind.getSegment(1));
85+
return PackageIdentifier.create(repository, tofind.subFragment(2, tofind.segmentCount()));
86+
} else if (!tofind.normalize().isNormalized()) {
87+
RepositoryName repository = RepositoryName.create("@" + tofind.getSegment(1));
88+
return PackageIdentifier.create(repository, tofind.subFragment(2, tofind.segmentCount()));
89+
} else {
90+
return PackageIdentifier.createInMainRepo(tofind);
91+
}
92+
}
93+
6194
/**
6295
* The identifier for this repository. This is either "" or prefixed with an "@",
6396
* e.g., "@myrepo".

‎src/main/java/com/google/devtools/build/lib/packages/Package.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,8 @@ protected void setDefaultRestrictedTo(Set<Label> environments) {
260260
defaultRestrictedTo = environments;
261261
}
262262

263+
// This must always be consistent with Root.computeSourceRoot; otherwise computing source roots
264+
// from exec paths does not work, which can break the action cache for input-discovering actions.
263265
private static Path getSourceRoot(Path buildFile, PathFragment packageFragment) {
264266
Path current = buildFile.getParentDirectory();
265267
for (int i = 0, len = packageFragment.segmentCount();

‎src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,9 +334,11 @@ protected CppCompileAction(
334334
// the inputs are as declared, hence known, and remain so.
335335
this.shouldScanIncludes = shouldScanIncludes;
336336
this.shouldPruneModules = shouldPruneModules;
337+
// We can only prune modules if include scanning is enabled.
338+
Preconditions.checkArgument(!shouldPruneModules || shouldScanIncludes, this);
337339
this.usePic = usePic;
338340
this.useHeaderModules = useHeaderModules;
339-
this.inputsKnown = !shouldScanIncludes;
341+
this.inputsKnown = !shouldScanIncludes && !cppSemantics.needsDotdInputPruning();
340342
this.cppCompileCommandLine =
341343
new CppCompileCommandLine(
342344
sourceFile, dotdFile, copts, coptsFilter, features, variables, actionName);

‎src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import com.google.devtools.build.lib.actions.ActionExecutionException;
2121
import com.google.devtools.build.lib.actions.Artifact;
2222
import com.google.devtools.build.lib.actions.ArtifactResolver;
23+
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
24+
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
2325
import com.google.devtools.build.lib.cmdline.RepositoryName;
2426
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2527
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -122,7 +124,15 @@ public NestedSet<Artifact> discoverInputsFromDotdFiles(
122124
}
123125
Artifact artifact = allowedDerivedInputsMap.get(execPathFragment);
124126
if (artifact == null) {
125-
artifact = artifactResolver.resolveSourceArtifact(execPathFragment, RepositoryName.MAIN);
127+
try {
128+
RepositoryName repository =
129+
PackageIdentifier.discoverFromExecPath(execPathFragment, false).getRepository();
130+
artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repository);
131+
} catch (LabelSyntaxException e) {
132+
throw new ActionExecutionException(
133+
String.format("Could not find the external repository for %s", execPathFragment),
134+
e, action, false);
135+
}
126136
}
127137
if (artifact != null) {
128138
inputs.add(artifact);

‎src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.google.devtools.build.lib.actions.Root;
3434
import com.google.devtools.build.lib.causes.Cause;
3535
import com.google.devtools.build.lib.causes.LabelCause;
36+
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
3637
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
3738
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
3839
import com.google.devtools.build.lib.events.Event;
@@ -296,10 +297,15 @@ public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> e
296297
PathFragment parent = Preconditions.checkNotNull(
297298
path.getParentDirectory(), "Must pass in files, not root directory");
298299
Preconditions.checkArgument(!parent.isAbsolute(), path);
299-
SkyKey depKey =
300-
ContainingPackageLookupValue.key(PackageIdentifier.createInMainRepo(parent));
301-
depKeys.put(path, depKey);
302-
keysRequested.add(depKey);
300+
try {
301+
SkyKey depKey =
302+
ContainingPackageLookupValue.key(PackageIdentifier.discoverFromExecPath(path, true));
303+
depKeys.put(path, depKey);
304+
keysRequested.add(depKey);
305+
} catch (LabelSyntaxException e) {
306+
throw new PackageRootResolutionException(
307+
String.format("Could not find the external repository for %s", path), e);
308+
}
303309
}
304310

305311
Map<SkyKey,
@@ -315,8 +321,8 @@ public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> e
315321
try {
316322
value = (ContainingPackageLookupValue) values.get(depKeys.get(path)).get();
317323
} catch (NoSuchPackageException | InconsistentFilesystemException e) {
318-
throw new PackageRootResolutionException("Could not determine containing package for "
319-
+ path, e);
324+
throw new PackageRootResolutionException(
325+
String.format("Could not determine containing package for %s", path), e);
320326
}
321327

322328
if (value == null) {
@@ -325,8 +331,10 @@ public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> e
325331
}
326332
if (value.hasContainingPackage()) {
327333
// We have found corresponding root for current execPath.
328-
result.put(path, Root.asSourceRoot(value.getContainingPackageRoot(),
329-
value.getContainingPackageName().getRepository().isMain()));
334+
result.put(path,
335+
Root.computeSourceRoot(
336+
value.getContainingPackageRoot(),
337+
value.getContainingPackageName().getRepository()));
330338
} else {
331339
// We haven't found corresponding root for current execPath.
332340
result.put(path, null);

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

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
7373
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
7474
import com.google.devtools.build.lib.cmdline.Label;
75+
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
7576
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
7677
import com.google.devtools.build.lib.cmdline.TargetParsingException;
7778
import com.google.devtools.build.lib.concurrent.ThreadSafety;
@@ -744,7 +745,6 @@ public Collection<Artifact> getWorkspaceStatusArtifacts(EventHandler eventHandle
744745
return ImmutableList.of(value.getStableArtifact(), value.getVolatileArtifact());
745746
}
746747

747-
// TODO(bazel-team): Make this take a PackageIdentifier.
748748
public Map<PathFragment, Root> getArtifactRootsForFiles(
749749
final EventHandler eventHandler, Iterable<PathFragment> execPaths)
750750
throws PackageRootResolutionException, InterruptedException {
@@ -760,28 +760,23 @@ public Map<PathFragment, Root> getArtifactRoots(
760760
private Map<PathFragment, Root> getArtifactRoots(
761761
final EventHandler eventHandler, Iterable<PathFragment> execPaths, boolean forFiles)
762762
throws PackageRootResolutionException, InterruptedException {
763-
764-
final List<SkyKey> packageKeys = new ArrayList<>();
765-
if (forFiles) {
766-
for (PathFragment execPath : execPaths) {
767-
PathFragment parent = Preconditions.checkNotNull(
768-
execPath.getParentDirectory(), "Must pass in files, not root directory");
769-
Preconditions.checkArgument(!parent.isAbsolute(), execPath);
770-
packageKeys.add(ContainingPackageLookupValue.key(
771-
PackageIdentifier.createInMainRepo(parent)));
772-
}
773-
} else {
774-
for (PathFragment execPath : execPaths) {
775-
Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
776-
packageKeys.add(ContainingPackageLookupValue.key(
777-
PackageIdentifier.createInMainRepo(execPath)));
763+
final Map<PathFragment, SkyKey> packageKeys = new HashMap<>();
764+
for (PathFragment execPath : execPaths) {
765+
try {
766+
PackageIdentifier pkgIdentifier =
767+
PackageIdentifier.discoverFromExecPath(execPath, forFiles);
768+
packageKeys.put(execPath, ContainingPackageLookupValue.key(pkgIdentifier));
769+
} catch (LabelSyntaxException e) {
770+
throw new PackageRootResolutionException(
771+
String.format("Could not find the external repository for %s", execPath), e);
778772
}
779773
}
780774

781775
EvaluationResult<ContainingPackageLookupValue> result;
782776
synchronized (valueLookupLock) {
783777
result =
784-
buildDriver.evaluate(packageKeys, /*keepGoing=*/ true, /*numThreads=*/ 1, eventHandler);
778+
buildDriver.evaluate(
779+
packageKeys.values(), /*keepGoing=*/ true, /*numThreads=*/ 1, eventHandler);
785780
}
786781

787782
if (result.hasError()) {
@@ -791,11 +786,12 @@ private Map<PathFragment, Root> getArtifactRoots(
791786

792787
Map<PathFragment, Root> roots = new HashMap<>();
793788
for (PathFragment execPath : execPaths) {
794-
ContainingPackageLookupValue value = result.get(ContainingPackageLookupValue.key(
795-
PackageIdentifier.createInMainRepo(forFiles ? execPath.getParentDirectory() : execPath)));
789+
ContainingPackageLookupValue value = result.get(packageKeys.get(execPath));
796790
if (value.hasContainingPackage()) {
797-
roots.put(execPath, Root.asSourceRoot(value.getContainingPackageRoot(),
798-
value.getContainingPackageName().getRepository().isMain()));
791+
roots.put(execPath,
792+
Root.computeSourceRoot(
793+
value.getContainingPackageRoot(),
794+
value.getContainingPackageName().getRepository()));
799795
} else {
800796
roots.put(execPath, null);
801797
}

0 commit comments

Comments
 (0)