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, 62 insertions, 0 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
index b174296907..82d0b12780 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
+import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
 import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.ErrorInfo;
@@ -213,4 +214,31 @@ public class SkylarkImportLookupFunctionTest extends BuildViewTestCase {
     assertEquals("invalid target name 'oops<?>.bzl': "
         + "target names may not contain non-printable characters: '\\x00'", errorMessage);
   }
+
+  @Test
+  public void testLoadFromExternalRepoInWorkspaceFileDisallowed() throws Exception {
+    scratch.deleteFile("tools/build_rules/prelude_blaze");
+    scratch.overwriteFile("WORKSPACE",
+        "local_repository(",
+        "    name = 'a_remote_repo',",
+        "    path = '/a_remote_repo'",
+        ")");
+    scratch.file("/a_remote_repo/remote_pkg/BUILD");
+    scratch.file("/a_remote_repo/remote_pkg/ext.bzl",
+        "CONST = 17");
+
+    SkyKey skylarkImportLookupKey =
+        SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked(
+            "@a_remote_repo//remote_pkg:ext.bzl"), /*inWorkspace=*/ true);
+    EvaluationResult<SkylarkImportLookupValue> result =
+        SkyframeExecutorTestUtils.evaluate(
+            getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter);
+
+    assertTrue(result.hasError());
+    ErrorInfo errorInfo = result.getError(skylarkImportLookupKey);
+    String errorMessage = errorInfo.getException().getMessage();
+    String expectedMsgTemplate = SkylarkImportFailedException.NO_EXT_WORKSPACE_LOAD_MSG_TEMPLATE;
+    assertEquals(String.format(expectedMsgTemplate, "@a_remote_repo//remote_pkg:ext.bzl"),
+        errorMessage);
+  }
 }
diff --git a/src/test/shell/bazel/skylark_repository_test.sh b/src/test/shell/bazel/skylark_repository_test.sh
index 35511f4b6b..3d59bda4be 100755
--- a/src/test/shell/bazel/skylark_repository_test.sh
+++ b/src/test/shell/bazel/skylark_repository_test.sh
@@ -140,6 +140,40 @@ EOF
   rm -fr $TEST_TMPDIR/other
 }
 
+# Loading a skylark file located in an external repo from a WORKSPACE file
+# is disallowed.
+function test_external_load_from_workspace_file_invalid() {
+  create_new_workspace
+  external_repo=${new_workspace_dir}
+
+  cat > ${WORKSPACE_DIR}/WORKSPACE <<EOF
+local_repository(name = "external_repo", path = "${external_repo}")
+load("@external_repo//external_pkg:ext.bzl", "CONST")
+EOF
+
+  mkdir ${WORKSPACE_DIR}/local_pkg
+  cat > ${WORKSPACE_DIR}/local_pkg/BUILD <<EOF
+genrule(
+  name = "shouldnt_be_built",
+  cmd = "echo echo"
+)
+EOF
+
+  mkdir ${external_repo}/external_pkg
+  touch ${external_repo}/external_pkg/BUILD
+  cat > ${external_repo}/external_pkg/ext.bzl <<EOF
+CONST = 17
+EOF
+
+  cd ${WORKSPACE_DIR}
+  bazel build local_pkg:shouldnt_be_built >& $TEST_log && \
+    fail "Expected build to fail" || true
+
+  expect_log "Extension file '@external_repo//external_pkg:ext.bzl' may not be \
+loaded from a WORKSPACE file since the extension file is located in an \
+external repository."
+}
+
 function tear_down() {
   true
 }