Handle exceptions encountered resolving packages during the execution phase

Currently we may do lookups of not-already-cached packages during the
execution phase for actions that discover inputs. Exceptions encountered
during this would go unhandled and result in a crash. Here we introduce
PackageRootResolutionException which wraps these exceptions and triggers
an ActionExecutionException which is cleanly handled in the exec phase.

As part of this change SkyframeActionExecutor#getArtifactRoots(...) will
fail properly on errors getting package roots.

--
MOS_MIGRATED_REVID=95578891

4 files changed, 39 insertions, 23 deletions

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 ded0cd39ec..b9db265ba3 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.PackageRootResolutionException;
 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;
@@ -105,12 +106,12 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
   private static final int VALIDATION_DEBUG = 0;  // 0==none, 1==warns/errors, 2==all
   private static final boolean VALIDATION_DEBUG_WARN = VALIDATION_DEBUG >= 1;
-  
+
   /**
    * A string constant for the c compilation action.
    */
   public static final String C_COMPILE = "c-compile";
-  
+
   /**
    * A string constant for the c++ compilation action.
    */
@@ -120,18 +121,18 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
    * A string constant for the c++ header parsing.
    */
   public static final String CPP_HEADER_PARSING = "c++-header-parsing";
-  
+
   /**
    * A string constant for the c++ header preprocessing.
    */
   public static final String CPP_HEADER_PREPROCESSING = "c++-header-preprocessing";
-  
+
   /**
    * A string constant for the c++ module compilation action.
    * Note: currently we don't support C module compilation.
    */
   public static final String CPP_MODULE_COMPILE = "c++-module-compile";
-  
+
   /**
    * A string constant for the preprocessing assembler action.
    */
@@ -199,7 +200,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
    */
   protected CppCompileAction(ActionOwner owner,
       // TODO(bazel-team): Eventually we will remove 'features'; all functionality in 'features'
-      // will be provided by 'featureConfiguration'. 
+      // will be provided by 'featureConfiguration'.
       ImmutableList<String> features,
       FeatureConfiguration featureConfiguration,
       Artifact sourceFile,
@@ -521,7 +522,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     }
     return cmdlineIncludes.build();
   }
-  
+
   @Override
   public Artifact getMainIncludeScannerSource() {
     return CppFileTypes.CPP_MODULE_MAP.matches(getSourceFile().getPath())
@@ -914,7 +915,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
   @Override
   public Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver, PackageRootResolver resolver,
-      Collection<PathFragment> inputPaths) {
+      Collection<PathFragment> inputPaths) throws PackageRootResolutionException {
     // 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().
@@ -931,7 +932,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       }
     }
 
-    Map<PathFragment, Artifact> resolvedArtifacts = 
+    Map<PathFragment, Artifact> resolvedArtifacts =
         artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
     if (resolvedArtifacts == null) {
       // We are missing some dependencies. We need to rerun this update later.
@@ -1175,7 +1176,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
     // The value of the BUILD_FDO_TYPE macro to be defined on command line
     @Nullable private final String fdoBuildStamp;
-    
+
     public CppCompileCommandLine(Artifact sourceFile, DotdFile dotdFile, CppModuleMap cppModuleMap,
         ImmutableList<String> copts, Predicate<String> coptsFilter,
         ImmutableList<String> pluginOpts, boolean isInstrumented,
@@ -1212,7 +1213,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
       return commandLine;
     }
-    
+
     private String getActionName() {
       PathFragment sourcePath = sourceFile.getExecPath();
       if (CppFileTypes.CPP_MODULE_MAP.matches(sourcePath)) {
@@ -1243,7 +1244,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     public List<String> getCompilerOptions() {
       List<String> options = new ArrayList<>();
       CppConfiguration toolchain = cppConfiguration;
-      
+
       // pluginOpts has to be added before defaultCopts because -fplugin must precede -plugin-arg.
       options.addAll(pluginOpts);
       addFilteredOptions(options, toolchain.getCompilerOptions(features));
@@ -1289,7 +1290,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       }
       if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
         buildVariables.addSequenceVariable("module_files", getHeaderModulePaths());
-      }      
+      }
       if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
         buildVariables.addSequenceVariable("include_paths",
             getSafePathStrings(context.getIncludeDirs()));
@@ -1389,7 +1390,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
         // from targets that are built in both modes.
         if (usePic == filename.endsWith(".pic.pcm")) {
           result.add(artifact.getExecPathString());
-        }          
+        }
       }
       return result;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 78570210b0..15128fae8e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -28,6 +28,7 @@ import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
@@ -1795,8 +1796,14 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     // TODO(bazel-team): Remove the "relative" guard. sysroot should always be relative, and this
     // should be enforced in the creation of CppConfiguration.
     if (getSysroot() != null && !getSysroot().isAbsolute()) {
-      Root sysrootRoot = Iterables.getOnlyElement(
+      Root sysrootRoot;
+      try {
+        sysrootRoot = Iterables.getOnlyElement(
           resolver.findPackageRoots(ImmutableList.of(getSysroot())).entrySet()).getValue();
+      } catch (PackageRootResolutionException prre) {
+        throw new ViewCreationFailedException("Failed to determine sysroot", prre);
+      }
+
       PathFragment sysrootExecPath = sysroot.getRelative(BUILT_IN_INCLUDE_PATH_FRAGMENT);
       if (sysrootRoot.getPath().getRelative(sysrootExecPath).exists()) {
         builtInIncludeFile = Preconditions.checkNotNull(
@@ -1808,7 +1815,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       getFdoSupport().prepareToBuild(execRoot, genfilesPath, artifactFactory, resolver);
     } catch (ZipException e) {
       throw new ViewCreationFailedException("Error reading provided FDO zip file", e);
-    } catch (FdoException | IOException e) {
+    } catch (FdoException | IOException | PackageRootResolutionException e) {
       throw new ViewCreationFailedException("Error while initializing FDO support", e);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
index dfabbed280..c10eea9edc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
@@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
+import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
@@ -255,11 +256,12 @@ public class FdoSupport implements Serializable {
    * FDO gcda zip file into a clean working directory under execRoot.
    *
    * @throws FdoException if the FDO ZIP contains a file of unknown type
+   * @throws PackageRootResolutionException
    */
   @ThreadHostile // must be called before starting the build
   public void prepareToBuild(Path execRoot, PathFragment genfilesPath,
       ArtifactFactory artifactDeserializer, PackageRootResolver resolver)
-      throws IOException, FdoException {
+      throws IOException, FdoException, PackageRootResolutionException {
     // The execRoot != null case is only there for testing. We cannot provide a real ZIP file in
     // tests because ZipFileSystem does not work with a ZIP on an in-memory file system.
     // IMPORTANT: Keep in sync with #declareSkyframeDependencies to avoid incrementality issues.
@@ -310,11 +312,13 @@ public class FdoSupport implements Serializable {
    *
    * @throws IOException if any of the I/O operations failed
    * @throws FdoException if the FDO ZIP contains a file of unknown type
+   * @throws PackageRootResolutionException on exception resolving package roots for imports
    */
   private void extractFdoZip(ArtifactFactory artifactFactory, Path sourceDir,
       Path targetDir, ImmutableSet.Builder<PathFragment> gcdaFilesBuilder,
       ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder,
-      PackageRootResolver resolver) throws IOException, FdoException {
+      PackageRootResolver resolver)
+          throws IOException, FdoException, PackageRootResolutionException {
     for (Path sourceFile : sourceDir.getDirectoryEntries()) {
       Path targetFile = targetDir.getRelative(sourceFile.getBaseName());
       if (sourceFile.isDirectory()) {
@@ -340,10 +344,12 @@ public class FdoSupport implements Serializable {
    * Reads a .gcda.imports file and stores the imports information.
    *
    * @throws FdoException if an auxiliary LIPO input was not found
+   * @throws PackageRootResolutionException on exception resolving package roots for imports
    */
   private void readCoverageImports(ArtifactFactory artifactFactory, Path importsFile,
       ImmutableMultimap.Builder<PathFragment, Artifact> importsBuilder,
-      PackageRootResolver resolver) throws IOException, FdoException {
+      PackageRootResolver resolver)
+          throws IOException, FdoException, PackageRootResolutionException {
     PathFragment key = importsFile.asFragment().relativeTo(ZIP_ROOT);
     String baseName = key.getBaseName();
     String ext = Iterables.getOnlyElement(CppFileTypes.COVERAGE_DATA_IMPORTS.getExtensions());
@@ -370,11 +376,12 @@ public class FdoSupport implements Serializable {
 
   /**
    * Reads a .afdo.imports file and stores the imports information.
+   * @throws PackageRootResolutionException
    */
   private ImmutableMultimap<PathFragment, Artifact> readAutoFdoImports(
       ArtifactFactory artifactFactory, Path importsFile, PathFragment genFilePath,
       PackageRootResolver resolver)
-          throws IOException, FdoException {
+          throws IOException, FdoException, PackageRootResolutionException {
     ImmutableMultimap.Builder<PathFragment, Artifact> importBuilder = ImmutableMultimap.builder();
     for (String line : FileSystemUtils.iterateLinesAsLatin1(importsFile)) {
       if (!line.isEmpty()) {
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 78b76ca0af..9aa9eb910b 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
@@ -29,6 +29,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.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
@@ -155,12 +156,12 @@ public final class ExtraAction extends SpawnAction {
   @Nullable
   @Override
   public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver,
-      PackageRootResolver resolver, Collection<PathFragment> inputPaths) {
+      PackageRootResolver resolver, Collection<PathFragment> inputPaths)
+          throws PackageRootResolutionException {
     // We update the inputs directly from the shadowed action.
     Set<PathFragment> extraActionPathFragments =
         ImmutableSet.copyOf(Artifact.asPathFragments(extraActionInputs));
-    return shadowedAction.resolveInputsFromCache(artifactResolver,
-        resolver,
+    return shadowedAction.resolveInputsFromCache(artifactResolver, resolver,
         Collections2.filter(inputPaths, Predicates.in(extraActionPathFragments)));
   }