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

8 files changed, 106 insertions, 42 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
index 7d187f9900..3f2f360259 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
@@ -305,17 +305,17 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
       // Source exec paths cannot escape the source root.
       return null;
     }
-    Artifact artifact = sourceArtifactCache.getArtifactIfValid(execPath);
-    if (artifact != null) {
-      return artifact;
-    }
     // Don't create an artifact if it's derived.
     if (isDerivedArtifact(execPath)) {
       return null;
     }
-
-    return createArtifactIfNotValid(
-        findSourceRoot(execPath, baseExecPath, baseRoot, repositoryName), execPath);
+    Root sourceRoot = findSourceRoot(execPath, baseExecPath, baseRoot, repositoryName);
+    Artifact artifact = sourceArtifactCache.getArtifactIfValid(execPath);
+    if (artifact != null) {
+      Preconditions.checkState(sourceRoot == null || sourceRoot.equals(artifact.getRoot()));
+      return artifact;
+    }
+    return createArtifactIfNotValid(sourceRoot, execPath);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Root.java b/src/main/java/com/google/devtools/build/lib/actions/Root.java
index 59010e9968..859a8b6344 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Root.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Root.java
@@ -15,16 +15,15 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
 import java.io.Serializable;
 import java.util.Objects;
-
 import javax.annotation.Nullable;
 
 /**
@@ -57,6 +56,20 @@ public final class Root implements Comparable<Root>, Serializable {
     return new Root(null, path, false, isMainRepo);
   }
 
+  // This must always be consistent with Package.getSourceRoot; otherwise computing source roots
+  // from exec paths does not work, which can break the action cache for input-discovering actions.
+  public static Root computeSourceRoot(Path packageRoot, RepositoryName repository) {
+    if (repository.isMain()) {
+      return Root.asSourceRoot(packageRoot, true);
+    } else {
+      Path actualRoot = packageRoot;
+      for (int i = 0; i < repository.getSourceRoot().segmentCount(); i++) {
+        actualRoot = actualRoot.getParentDirectory();
+      }
+      return Root.asSourceRoot(actualRoot, false);
+    }
+  }
+
   /**
    * testonly until {@link #asSourceRoot(Path, boolean)} is deleted.
    */
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 1e2e3d0385..5688a3184f 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.concurrent.BlazeInterners;
 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 @@ public final class PackageIdentifier implements Comparable<PackageIdentifier>, S
   }
 
   /**
+   * 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".
    */
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 45fa5cab50..e4f726104f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -260,6 +260,8 @@ public class Package {
     defaultRestrictedTo = environments;
   }
 
+  // This must always be consistent with Root.computeSourceRoot; otherwise computing source roots
+  // from exec paths does not work, which can break the action cache for input-discovering actions.
   private static Path getSourceRoot(Path buildFile, PathFragment packageFragment) {
     Path current = buildFile.getParentDirectory();
     for (int i = 0, len = packageFragment.segmentCount();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 3f8437087f..7f433726c6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -334,9 +334,11 @@ public class CppCompileAction extends AbstractAction
     // the inputs are as declared, hence known, and remain so.
     this.shouldScanIncludes = shouldScanIncludes;
     this.shouldPruneModules = shouldPruneModules;
+    // We can only prune modules if include scanning is enabled.
+    Preconditions.checkArgument(!shouldPruneModules || shouldScanIncludes, this);
     this.usePic = usePic;
     this.useHeaderModules = useHeaderModules;
-    this.inputsKnown = !shouldScanIncludes;
+    this.inputsKnown = !shouldScanIncludes && !cppSemantics.needsDotdInputPruning();
     this.cppCompileCommandLine =
         new CppCompileCommandLine(
             sourceFile, dotdFile, copts, coptsFilter, features, variables, actionName);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
index 5b59a0601f..5ae21d1b03 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
@@ -20,6 +20,8 @@ import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactResolver;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -122,7 +124,15 @@ public class HeaderDiscovery {
       }
       Artifact artifact = allowedDerivedInputsMap.get(execPathFragment);
       if (artifact == null) {
-        artifact = artifactResolver.resolveSourceArtifact(execPathFragment, RepositoryName.MAIN);
+        try {
+          RepositoryName repository =
+              PackageIdentifier.discoverFromExecPath(execPathFragment, false).getRepository();
+          artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repository);
+        } catch (LabelSyntaxException e) {
+          throw new ActionExecutionException(
+              String.format("Could not find the external repository for %s", execPathFragment),
+              e, action, false);
+        }
       }
       if (artifact != null) {
         inputs.add(artifact);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index fc7db60296..538ef90008 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.causes.Cause;
 import com.google.devtools.build.lib.causes.LabelCause;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
@@ -296,10 +297,15 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         PathFragment parent = Preconditions.checkNotNull(
             path.getParentDirectory(), "Must pass in files, not root directory");
         Preconditions.checkArgument(!parent.isAbsolute(), path);
-        SkyKey depKey =
-            ContainingPackageLookupValue.key(PackageIdentifier.createInMainRepo(parent));
-        depKeys.put(path, depKey);
-        keysRequested.add(depKey);
+        try {
+          SkyKey depKey =
+              ContainingPackageLookupValue.key(PackageIdentifier.discoverFromExecPath(path, true));
+          depKeys.put(path, depKey);
+          keysRequested.add(depKey);
+        } catch (LabelSyntaxException e) {
+          throw new PackageRootResolutionException(
+              String.format("Could not find the external repository for %s", path), e);
+        }
       }
 
       Map<SkyKey,
@@ -315,8 +321,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         try {
           value = (ContainingPackageLookupValue) values.get(depKeys.get(path)).get();
         } catch (NoSuchPackageException | InconsistentFilesystemException e) {
-          throw new PackageRootResolutionException("Could not determine containing package for "
-              + path, e);
+          throw new PackageRootResolutionException(
+              String.format("Could not determine containing package for %s", path), e);
         }
 
         if (value == null) {
@@ -325,8 +331,10 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         }
         if (value.hasContainingPackage()) {
           // We have found corresponding root for current execPath.
-          result.put(path, Root.asSourceRoot(value.getContainingPackageRoot(),
-              value.getContainingPackageName().getRepository().isMain()));
+          result.put(path,
+              Root.computeSourceRoot(
+                  value.getContainingPackageRoot(),
+                  value.getContainingPackageName().getRepository()));
         } else {
           // We haven't found corresponding root for current execPath.
           result.put(path, null);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index a8acf8805a..59a0bbc770 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -72,6 +72,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigurationFactory;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
@@ -744,7 +745,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return ImmutableList.of(value.getStableArtifact(), value.getVolatileArtifact());
   }
 
-  // TODO(bazel-team): Make this take a PackageIdentifier.
   public Map<PathFragment, Root> getArtifactRootsForFiles(
       final EventHandler eventHandler, Iterable<PathFragment> execPaths)
       throws PackageRootResolutionException, InterruptedException {
@@ -760,28 +760,23 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   private Map<PathFragment, Root> getArtifactRoots(
       final EventHandler eventHandler, Iterable<PathFragment> execPaths, boolean forFiles)
       throws PackageRootResolutionException, InterruptedException {
-
-    final List<SkyKey> packageKeys = new ArrayList<>();
-    if (forFiles) {
-      for (PathFragment execPath : execPaths) {
-        PathFragment parent = Preconditions.checkNotNull(
-            execPath.getParentDirectory(), "Must pass in files, not root directory");
-        Preconditions.checkArgument(!parent.isAbsolute(), execPath);
-        packageKeys.add(ContainingPackageLookupValue.key(
-            PackageIdentifier.createInMainRepo(parent)));
-      }
-    } else {
-      for (PathFragment execPath : execPaths) {
-        Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
-        packageKeys.add(ContainingPackageLookupValue.key(
-            PackageIdentifier.createInMainRepo(execPath)));
+    final Map<PathFragment, SkyKey> packageKeys = new HashMap<>();
+    for (PathFragment execPath : execPaths) {
+      try {
+        PackageIdentifier pkgIdentifier =
+            PackageIdentifier.discoverFromExecPath(execPath, forFiles);
+        packageKeys.put(execPath, ContainingPackageLookupValue.key(pkgIdentifier));
+      } catch (LabelSyntaxException e) {
+        throw new PackageRootResolutionException(
+            String.format("Could not find the external repository for %s", execPath), e);
       }
     }
 
     EvaluationResult<ContainingPackageLookupValue> result;
     synchronized (valueLookupLock) {
       result =
-          buildDriver.evaluate(packageKeys, /*keepGoing=*/ true, /*numThreads=*/ 1, eventHandler);
+          buildDriver.evaluate(
+              packageKeys.values(), /*keepGoing=*/ true, /*numThreads=*/ 1, eventHandler);
     }
 
     if (result.hasError()) {
@@ -791,11 +786,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
     Map<PathFragment, Root> roots = new HashMap<>();
     for (PathFragment execPath : execPaths) {
-      ContainingPackageLookupValue value = result.get(ContainingPackageLookupValue.key(
-          PackageIdentifier.createInMainRepo(forFiles ? execPath.getParentDirectory() : execPath)));
+      ContainingPackageLookupValue value = result.get(packageKeys.get(execPath));
       if (value.hasContainingPackage()) {
-        roots.put(execPath, Root.asSourceRoot(value.getContainingPackageRoot(),
-            value.getContainingPackageName().getRepository().isMain()));
+        roots.put(execPath,
+            Root.computeSourceRoot(
+                value.getContainingPackageRoot(),
+                value.getContainingPackageName().getRepository()));
       } else {
         roots.put(execPath, null);
       }