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 #2490.
--
PiperOrigin-RevId: 148439309
MOS_MIGRATED_REVID=148439309
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
index 1e2e3d0..5688a31 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -20,10 +20,8 @@
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Canonicalizer;
import com.google.devtools.build.lib.vfs.PathFragment;
-
import java.io.Serializable;
import java.util.Objects;
-
import javax.annotation.concurrent.Immutable;
/**
@@ -59,6 +57,41 @@
}
/**
+ * Tries to infer the package identifier from the given exec path. This method does not perform
+ * any I/O, but looks solely at the structure of the exec path. The resulting identifier may
+ * actually be a subdirectory of a package rather than a package, e.g.:
+ * <pre><code>
+ * + WORKSPACE
+ * + foo/BUILD
+ * + foo/bar/bar.java
+ * </code></pre>
+ *
+ * In this case, this method returns a package identifier for foo/bar, even though that is not a
+ * package. Callers need to look up the actual package if needed.
+ *
+ * @throws LabelSyntaxException if the exec path seems to be for an external repository that doe
+ * not have a valid repository name (see {@link RepositoryName#create})
+ */
+ public static PackageIdentifier discoverFromExecPath(PathFragment execPath, boolean forFiles)
+ throws LabelSyntaxException {
+ Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
+ PathFragment tofind = forFiles
+ ? Preconditions.checkNotNull(
+ execPath.getParentDirectory(), "Must pass in files, not root directory")
+ : execPath;
+ if (tofind.startsWith(new PathFragment(Label.EXTERNAL_PATH_PREFIX))) {
+ // TODO(ulfjack): Remove this when kchodorow@'s exec root rearrangement has been rolled out.
+ RepositoryName repository = RepositoryName.create("@" + tofind.getSegment(1));
+ return PackageIdentifier.create(repository, tofind.subFragment(2, tofind.segmentCount()));
+ } else if (!tofind.normalize().isNormalized()) {
+ RepositoryName repository = RepositoryName.create("@" + tofind.getSegment(1));
+ return PackageIdentifier.create(repository, tofind.subFragment(2, tofind.segmentCount()));
+ } else {
+ return PackageIdentifier.createInMainRepo(tofind);
+ }
+ }
+
+ /**
* The identifier for this repository. This is either "" or prefixed with an "@",
* e.g., "@myrepo".
*/
