Disallow loads from external repos in WORKSPACE files. Currently, attempting to do such a load will result in a skyframe circular dependency exception.

As a side effect of this change, SkylarkImportFailedExceptions thrown by SkylarkImportLookupFunction are now caught by PackageFunction and wrapped and rethrown as a PackageFunctionException. Previously, the first exception wasn't caught, generating an uncaught exception error at top level.

--
MOS_MIGRATED_REVID=112328755

2 files changed, 23 insertions, 7 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 62e4cf0664..0a69fdd50a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -339,10 +339,10 @@ public class PackageFunction implements SkyFunction {
     PackageValue workspace = null;
     try {
       workspace = (PackageValue) env.getValueOrThrow(workspaceKey, IOException.class,
-          FileSymlinkException.class, InconsistentFilesystemException.class,
-          EvalException.class);
+      FileSymlinkException.class, InconsistentFilesystemException.class,
+      EvalException.class, SkylarkImportFailedException.class);
     } catch (IOException | FileSymlinkException | InconsistentFilesystemException
-        | EvalException e) {
+          | EvalException | SkylarkImportFailedException e) {
       throw new PackageFunctionException(new BadWorkspaceFileException(e.getMessage()),
           Transience.PERSISTENT);
     }
@@ -566,7 +566,7 @@ public class PackageFunction implements SkyFunction {
     Map<String, Extension> importMap = Maps.newHashMapWithExpectedSize(imports.size());
     ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
     ImmutableMap<String, Label> importPathMap;
-    
+
     // Find the labels corresponding to the load statements.
     Label labelForCurrBuildFile;
     try {
@@ -640,12 +640,12 @@ public class PackageFunction implements SkyFunction {
       throw new PackageFunctionException(
           new InternalInconsistentFilesystemException(packageId, e), Transience.PERSISTENT);
     }
-    
+
     if (valuesMissing) {
       // Some imports are unavailable.
       return null;
     }
-    
+
     // Process the loaded imports.
     for (Entry<String, Label> importEntry : importPathMap.entrySet()) {
       String importString = importEntry.getKey();
@@ -656,7 +656,7 @@ public class PackageFunction implements SkyFunction {
       importMap.put(importString, importLookupValue.getEnvironmentExtension());
       fileDependencies.add(importLookupValue.getDependency());
     }
-    
+
     return new SkylarkImportResult(importMap, transitiveClosureOfLabels(fileDependencies.build()));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index 801fbb3372..0898b3617f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -113,6 +113,13 @@ public class SkylarkImportLookupFunction implements SkyFunction {
       throws InconsistentFilesystemException, SkylarkImportFailedException, InterruptedException {
     PathFragment filePath = fileLabel.toPathFragment();
 
+    boolean loadingFromDefaultlRepo = fileLabel.getPackageIdentifier().getRepository().isDefault();
+    if (inWorkspace && !loadingFromDefaultlRepo) {
+      // Loads of files in external repos are currently prohibited in a WORKSPACE file to prevent
+      // circular skyframe dependencies.
+      throw SkylarkImportFailedException.noExternalLoadsFromWorkspace(fileLabel);
+    }
+
     // Load the AST corresponding to this file.
     ASTFileLookupValue astLookupValue;
     try {
@@ -422,6 +429,15 @@ public class SkylarkImportLookupFunction implements SkyFunction {
     static SkylarkImportFailedException skylarkErrors(PathFragment file) {
       return new SkylarkImportFailedException(String.format("Extension '%s' has errors", file));
     }
+
+   static final String NO_EXT_WORKSPACE_LOAD_MSG_TEMPLATE =
+       "Extension file '%s' may not be loaded from a WORKSPACE file "
+           + "since the extension file is located in an external repository.";
+   static SkylarkImportFailedException noExternalLoadsFromWorkspace(Label fileLabel) {
+      return new SkylarkImportFailedException(String.format(NO_EXT_WORKSPACE_LOAD_MSG_TEMPLATE,
+          fileLabel));
+    }
+
   }
 
   private static final class SkylarkImportLookupFunctionException extends SkyFunctionException {