Replace some calls to ArtifactFactory.resolveSourceArtifact(PathFragment execPath), with a skyframe native implementation ArtifactFactory.resolveSourceArtifact(Iterable<PathFragment> execPaths, PackageRootResolver resolver).

--
MOS_MIGRATED_REVID=86062289

9 files changed, 173 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 87105d549c..95c943bfa5 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -98,8 +98,8 @@ public abstract class AbstractAction implements Action {
   }
 
   @Override
-  public void updateInputsFromCache(
-      ArtifactResolver artifactResolver, Collection<PathFragment> inputPaths) {
+  public boolean updateInputsFromCache(ArtifactResolver artifactResolver,
+      PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
     throw new IllegalStateException(
         "Method must be overridden for actions that may have unknown inputs.");
   }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java
index 4970203ece..efaa0dc3c8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Action.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java
@@ -101,7 +101,7 @@ public interface Action extends ActionMetadata, Describable {
    * Method used to find inputs before execution for an action that
    * {@link ActionMetadata#discoversInputs}.
    */
-  public void discoverInputs(ActionExecutionContext actionExecutionContext)
+  void discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException;
 
   /**
@@ -116,9 +116,13 @@ public interface Action extends ActionMetadata, Describable {
    *
    * @param artifactResolver the artifact factory that can be used to manufacture artifacts
    * @param inputPaths List of relative (to the execution root) input paths
+   * @param resolver object which helps to resolve some of the artifacts
+   * @return false if some dependencies are missing and we need to update again later,
+   * otherwise true.
    */
-  public void updateInputsFromCache(
-      ArtifactResolver artifactResolver, Collection<PathFragment> inputPaths);
+  boolean updateInputsFromCache(
+      ArtifactResolver artifactResolver, PackageRootResolver resolver,
+      Collection<PathFragment> inputPaths);
 
   /**
    * Return a best-guess estimate of the operation's resource consumption on the
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index 2525c8dea8..4b970e3540 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -134,7 +134,7 @@ public class ActionCacheChecker {
   // Note: the handler should only be used for DEPCHECKER events; there's no
   // guarantee it will be available for other events.
   public Token getTokenIfNeedToExecute(Action action, EventHandler handler,
-      MetadataHandler metadataHandler) {
+      MetadataHandler metadataHandler, PackageRootResolver resolver) {
     // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that
     // produce only symlinks we should not check whether inputs are valid at all - all that matters
     // that inputs and outputs are still exist (and new inputs have not appeared). All other checks
@@ -157,7 +157,15 @@ public class ActionCacheChecker {
     if (!inputsKnown) {
       Preconditions.checkState(action.discoversInputs());
       entry = getCacheEntry(action);
-      updateActionInputs(action, entry);
+      boolean ranSuccessfully = updateActionInputs(action, entry, resolver);
+      // If during update of inputs skyframe was missing some dependencies (for example,
+      // ContainingPackageLookupValue inside of ArtifactFactory.resolveSourceArtifact), we need to
+      // wait for those dependencies to be resolved. So next time when we will call corresponding
+      // ActionExecutionFunction all those dependencies will have been resolved and we can continue
+      // action execution process.
+      if (!ranSuccessfully) {
+        return Token.NEED_TO_RERUN;
+      }
     }
     if (mustExecute(action, entry, handler, metadataHandler)) {
       return new Token(getKeyString(action));
@@ -220,9 +228,10 @@ public class ActionCacheChecker {
     actionCache.put(key, entry);
   }
 
-  protected void updateActionInputs(Action action, ActionCache.Entry entry) {
+  protected boolean updateActionInputs(Action action, ActionCache.Entry entry,
+      PackageRootResolver resolver) {
     if (entry == null || entry.isCorrupted()) {
-      return;
+      return true;
     }
 
     List<PathFragment> outputs = new ArrayList<>();
@@ -238,7 +247,7 @@ public class ActionCacheChecker {
         inputs.add(execPath);
       }
     }
-    action.updateInputsFromCache(artifactResolver, inputs);
+    return action.updateInputsFromCache(artifactResolver, resolver, inputs);
   }
 
   /**
@@ -332,6 +341,7 @@ public class ActionCacheChecker {
 
   /** Wrapper for all context needed by the ActionCacheChecker to handle a single action. */
   public static final class Token {
+    public static final Token NEED_TO_RERUN = new Token("need to rerun");
     private final String cacheKey;
 
     private Token(String cacheKey) {
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 99a53cbc62..72e29c9232 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
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.packages.PackageIdentifier;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
@@ -248,6 +249,44 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
     return null;  // not a path that we can find...
   }
 
+  @Override
+  public synchronized Map<PathFragment, Artifact> resolveSourceArtifacts(
+      Iterable<PathFragment> execPaths, PackageRootResolver resolver) {
+    Map<PathFragment, Artifact> result = new HashMap<>();
+    ArrayList<PathFragment> unresolvedPaths = new ArrayList<>();
+
+    for (PathFragment execPath : execPaths) {
+      PathFragment execPathNormalized = execPath.normalize();
+      // First try a quick map lookup to see if the artifact already exists.
+      Artifact a = pathToSourceArtifact.get(execPathNormalized);
+      if (a != null) {
+        result.put(execPath, a);
+      } else if (findDerivedRoot(execRoot.getRelative(execPathNormalized)) != null) {
+        // Don't create an artifact if it's derived.
+        result.put(execPath, null);
+      } else {
+        // Remember this path, maybe we can resolve it with the help of PackageRootResolver.
+        unresolvedPaths.add(execPath);
+      }
+    }
+    Map<PathFragment, Root> sourceRoots = resolver.findPackageRoots(unresolvedPaths);
+    // We are missing some dependencies. We need to rerun this method later.
+    if (sourceRoots == null) {
+      return null;
+    }
+    for (PathFragment path : unresolvedPaths) {
+      Root sourceRoot = sourceRoots.get(path);
+      if (sourceRoot != null) {
+        // We have found corresponding source root, so we should create a new source artifact.
+        result.put(path, getSourceArtifact(path.normalize(), sourceRoot, ArtifactOwner.NULL_OWNER));
+      } else {
+        // Not a path that we can find...
+        result.put(path, null);
+      }
+    }
+    return result;
+  }
+
   /**
    * Finds the derived root for a full path by comparing against the known
    * derived artifact roots.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
index b74c17b0f9..baebfec0f5 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
@@ -16,6 +16,10 @@ package com.google.devtools.build.lib.actions;
 
 import com.google.devtools.build.lib.vfs.PathFragment;
 
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
 /**
  * An interface for resolving artifact names to {@link Artifact} objects. Should only be used
  * in the internal machinery of Blaze: rule implementations are not allowed to do this.
@@ -53,4 +57,23 @@ public interface ArtifactResolver {
    *         the root can not be determined and the artifact did not exist before.
    */
   Artifact resolveSourceArtifact(PathFragment execPath);
+
+  /**
+   * Resolves source Artifacts given execRoot-relative paths.
+   *
+   * <p>Never creates or returns derived artifacts, only source artifacts.
+   *
+   * <p>Note: this method should only be used when the roots are unknowable, such as from the
+   * post-compile .d or manifest scanning methods.
+   *
+   * @param execPaths list of exec paths of the artifacts to resolve
+   * @param resolver object that helps to resolve package root of given paths
+   * @return map which contains list of execPaths and corresponding Artifacts. Map can contain
+   *         existing or new source Artifacts for the given execPaths. The artifact is null if the
+   *         root cannot be determined and the artifact did not exist before. Return null if any
+   *         dependencies are missing.
+   */
+  @Nullable
+  Map<PathFragment, Artifact> resolveSourceArtifacts(Iterable<PathFragment> execPaths,
+      PackageRootResolver resolver);
 }
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 550eecb48d..4d145826f9 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
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.MiddlemanExpander;
 import com.google.devtools.build.lib.actions.ArtifactResolver;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.extra.CppCompileInfo;
 import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -827,21 +828,34 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
   }
 
   @Override
-  public void updateInputsFromCache(
-      ArtifactResolver artifactResolver, Collection<PathFragment> inputPaths) {
+  public boolean updateInputsFromCache(
+      ArtifactResolver artifactResolver, PackageRootResolver resolver,
+      Collection<PathFragment> inputPaths) {
     // Note that this method may trigger a violation of the desirable invariant that getInputs()
     // is a superset of getMandatoryInputs(). See bug about an "action not in canonical form"
     // error message and the integration test test_crosstool_change_and_failure().
-
     Map<PathFragment, Artifact> allowedDerivedInputsMap = getAllowedDerivedInputsMap();
     List<Artifact> inputs = new ArrayList<>();
+    List<PathFragment> unresolvedPaths = new ArrayList<>();
     for (PathFragment execPath : inputPaths) {
-      // The artifact may be a derived artifact, and if it has been created already, then we still
-      // want to keep it to preserve incrementality.
       Artifact artifact = allowedDerivedInputsMap.get(execPath);
-      if (artifact == null) {
-        artifact = artifactResolver.resolveSourceArtifact(execPath);
+      if (artifact != null) {
+        inputs.add(artifact);
+      } else {
+        // Remember this execPath, we will try to resolve it as a source artifact.
+        unresolvedPaths.add(execPath);
       }
+    }
+
+    Map<PathFragment, Artifact> resolvedArtifacts = 
+        artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
+    if (resolvedArtifacts == null) {
+      // We are missing some dependencies. We need to rerun this update later.
+      return false;
+    }
+
+    for (PathFragment execPath : unresolvedPaths) {
+      Artifact artifact = resolvedArtifacts.get(execPath);
       // If PathFragment cannot be resolved into the artifact - ignore it. This could happen if
       // rule definition has changed and action no longer depends on, e.g., additional source file
       // in the separate package and that package is no longer referenced anywhere else.
@@ -855,6 +869,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     synchronized (this) {
       setInputs(inputs);
     }
+    return true;
   }
 
   private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
index 250235d2c8..9be7dc3464 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.actions.ArtifactResolver;
 import com.google.devtools.build.lib.actions.DelegateSpawn;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
 import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -152,15 +153,20 @@ public final class ExtraAction extends SpawnAction {
   }
 
   @Override
-  public void updateInputsFromCache(ArtifactResolver artifactResolver,
-      Collection<PathFragment> inputPaths) {
+  public boolean updateInputsFromCache(ArtifactResolver artifactResolver,
+      PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
     // We update the inputs directly from the shadowed action.
     Set<PathFragment> extraActionPathFragments =
         ImmutableSet.copyOf(Artifact.asPathFragments(extraActionInputs));
-    shadowedAction.updateInputsFromCache(artifactResolver,
+    boolean noMissingDependencies = shadowedAction.updateInputsFromCache(artifactResolver, resolver,
         Collections2.filter(inputPaths, Predicates.in(extraActionPathFragments)));
+    if (!noMissingDependencies) {
+      // This update needs to be rerun.
+      return false;
+    }
     Preconditions.checkState(shadowedAction.inputsKnown(), "%s %s", this, shadowedAction);
     updateInputs(shadowedAction.getInputs());
+    return true;
   }
 
   /**
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 14208602d4..ba24480456 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
@@ -26,12 +26,16 @@ import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionExcep
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
+import com.google.devtools.build.lib.actions.PackageRootResolver;
+import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.PackageIdentifier;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -115,6 +119,49 @@ public class ActionExecutionFunction implements SkyFunction {
 
     return result;
   }
+  
+  /**
+   * Skyframe implementation of {@link PackageRootResolver}. Should be used only from SkyFunctions,
+   * because it uses SkyFunction.Environment for evaluation of ContainingPackageLookupValue.
+   */
+  private static class PackageRootResolverWithEnvironment implements PackageRootResolver {
+    private final Environment env;
+
+    public PackageRootResolverWithEnvironment(Environment env) {
+      this.env = env;
+    }
+
+    @Override
+    public Map<PathFragment, Root> findPackageRoots(Iterable<PathFragment> execPaths) {
+      Map<PathFragment, SkyKey> depKeys = new HashMap<>(); 
+      // Create SkyKeys list based on execPaths.
+      for (PathFragment path : execPaths) {
+        depKeys.put(path,
+            ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(path)));
+      }
+      Map<SkyKey, SkyValue> values = env.getValues(depKeys.values());
+      if (env.valuesMissing()) {
+        // Some values are not computed yet.
+        return null;
+      }
+      Map<PathFragment, Root> result = new HashMap<>();
+      for (PathFragment path : execPaths) {
+        // TODO(bazel-team): Add check for errors here, when loading phase will be removed.
+        // For now all possible errors that ContainingPackageLookupFunction can generate
+        // are caught in previous phases.
+        ContainingPackageLookupValue value =
+            (ContainingPackageLookupValue) values.get(depKeys.get(path));
+        if (value.hasContainingPackage()) {
+          // We have found corresponding root for current execPath.
+          result.put(path, Root.asSourceRoot(value.getContainingPackageRoot()));
+        } else {
+          // We haven't found corresponding root for current execPath.
+          result.put(path, null);
+        }
+      }
+      return result;
+    }
+  }
 
   private ActionExecutionValue checkCacheAndExecuteIfNeeded(
       Action action,
@@ -146,7 +193,11 @@ public class ActionExecutionFunction implements SkyFunction {
           tsgm);
       metadataHandler =
           skyframeActionExecutor.constructMetadataHandler(fileAndMetadataCache);
-      token = skyframeActionExecutor.checkActionCache(action, metadataHandler, actionStartTime);
+      token = skyframeActionExecutor.checkActionCache(action, metadataHandler,  
+          new PackageRootResolverWithEnvironment(env), actionStartTime);
+      if (token == Token.NEED_TO_RERUN) {
+        return null;
+      }
     }
     if (token == null && inputArtifactData != null) {
       // We got a hit from the action cache -- no need to execute.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index bd591e1b11..e7f626a1ea 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -46,6 +46,7 @@ import com.google.devtools.build.lib.actions.MapBasedActionGraph;
 import com.google.devtools.build.lib.actions.MutableActionGraph;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
+import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.TargetOutOfDateException;
@@ -550,10 +551,11 @@ public final class SkyframeActionExecutor {
    * if the action is up to date, and non-null if it needs to be executed, in which case that token
    * should be provided to the ActionCacheChecker after execution.
    */
-  Token checkActionCache(Action action, MetadataHandler metadataHandler, long actionStartTime) {
+  Token checkActionCache(Action action, MetadataHandler metadataHandler,
+      PackageRootResolver resolver, long actionStartTime) {
     profiler.startTask(ProfilerTask.ACTION_CHECK, action);
     Token token = actionCacheChecker.getTokenIfNeedToExecute(
-        action, explain ? reporter : null, metadataHandler);
+        action, explain ? reporter : null, metadataHandler, resolver);
     profiler.completeTask(ProfilerTask.ACTION_CHECK);
     if (token == null) {
       boolean eventPosted = false;