Only depend on the WORKSPACE file for external files that are under t…

…he external/ directory, i.e. were created by Bazel.

This avoids a cycle that arose when a file is load()ed from the WORKSPACE file that is reached through a symlink to an external directory:

* The WORKSPACE file depends on the package lookup node of the .bzl file
* The package lookup node (transitively) depends on wherever the symlink points
* The target of the symlink is an external file and as such, it depends on the WORKSPACE file

This will probably be, erm, interesting to solve when we get as far as to load stuff from external repositories in the WORKSPACE file, but we are just not there yet.

--
MOS_MIGRATED_REVID=110344658

src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java

@@ -23,7 +23,6 @@
 
 /** Common utilities for dealing with files outside the package roots. */
 public class ExternalFilesHelper {
-
   private final AtomicReference<PathPackageLocator> pkgLocator;
   private final ExternalFileAction externalFileAction;
 
@@ -78,27 +77,31 @@ public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environme
     }
 
     externalFileSeen = true;
-    if (externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG) {
-      // For files outside the package roots, add a dependency on the //external package so that if
-      // the WORKSPACE file changes, the File/DirectoryStateValue will be re-evaluated.
-      //
-      // Note that:
-      // - We don't add a dependency on the parent directory at the package root boundary, so
-      // the only transitive dependencies from files inside the package roots to external files
-      // are through symlinks. So the upwards transitive closure of external files is small.
-      // - The only way other than external repositories for external source files to get into the
-      // skyframe graph in the first place is through symlinks outside the package roots, which we
-      // neither want to encourage nor optimize for since it is not common. So the set of external
-      // files is small.
-      // TODO(kchodorow): check that the path is under output_base/external before adding the dep.
-      PackageValue pkgValue = (PackageValue) env.getValue(PackageValue.key(
-              Label.EXTERNAL_PACKAGE_IDENTIFIER));
-      if (pkgValue == null) {
-        return;
-      }
-      Preconditions.checkState(!pkgValue.getPackage().containsErrors());
-    } else {
+    if (externalFileAction == ExternalFileAction.ERROR_OUT) {
       throw new FileOutsidePackageRootsException(rootedPath);
     }
+
+    if (!rootedPath.asPath().startsWith(
+        pkgLocator.get().getOutputBase().getRelative(Label.EXTERNAL_PATH_PREFIX))) {
+      return;
+    }
+
+    // For files that are under $OUTPUT_BASE/external, add a dependency on the //external package
+    // so that if the WORKSPACE file changes, the File/DirectoryStateValue will be re-evaluated.
+    //
+    // Note that:
+    // - We don't add a dependency on the parent directory at the package root boundary, so
+    // the only transitive dependencies from files inside the package roots to external files
+    // are through symlinks. So the upwards transitive closure of external files is small.
+    // - The only way other than external repositories for external source files to get into the
+    // skyframe graph in the first place is through symlinks outside the package roots, which we
+    // neither want to encourage nor optimize for since it is not common. So the set of external
+    // files is small.
+    PackageValue pkgValue = (PackageValue) env.getValue(PackageValue.key(
+            Label.EXTERNAL_PACKAGE_IDENTIFIER));
+    if (pkgValue == null) {
+      return;
+    }
+    Preconditions.checkState(!pkgValue.getPackage().containsErrors());
   }
 }

src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java

@@ -239,7 +239,7 @@ protected void update(EventBus eventBus, FlagBuilder config, String... labels) t
     PackageCacheOptions packageCacheOptions = optionsParser.getOptions(PackageCacheOptions.class);
 
     PathPackageLocator pathPackageLocator = PathPackageLocator.create(
-        null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory);
+        outputBase, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory);
     skyframeExecutor.preparePackageLoading(pathPackageLocator,
         packageCacheOptions.defaultVisibility, true,
         7, ruleClassProvider.getDefaultsPackageContent(), UUID.randomUUID());

src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java

@@ -299,7 +299,7 @@ protected Target getTarget(Label label)
 
   private void setUpSkyframe() {
     PathPackageLocator pkgLocator = PathPackageLocator.create(
-        null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory);
+        outputBase, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory);
     skyframeExecutor.preparePackageLoading(pkgLocator,
         packageCacheOptions.defaultVisibility, true,
         7, ruleClassProvider.getDefaultsPackageContent(optionsParser),

src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java

@@ -109,7 +109,7 @@ protected Iterable<EnvironmentExtension> getEnvironmentExtensions() {
 
   private void setUpSkyframe(PackageCacheOptions packageCacheOptions) {
     PathPackageLocator pkgLocator = PathPackageLocator.create(
-        null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory);
+        outputBase, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory);
     skyframeExecutor.preparePackageLoading(pkgLocator,
         packageCacheOptions.defaultVisibility, true,
         7, ruleClassProvider.getDefaultsPackageContent(),

src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java

@@ -383,14 +383,77 @@ public void testMissingPackages() throws Exception {
     tester.getTarget("//a:a");
   }
 
+  @Test
+  public void testChangedExternalFile() throws Exception {
+    tester.addFile("a/BUILD",
+        "load('/a/b', 'b')",
+        "b()");
+
+    tester.addFile("/b.bzl",
+        "def b():",
+        "  pass");
+    tester.addSymlink("a/b.bzl", "/b.bzl");
+    tester.sync();
+    tester.getTarget("//a:BUILD");
+    tester.modifyFile("/b.bzl", "ERROR ERROR");
+    tester.sync();
+
+    try {
+      tester.getTarget("//a:BUILD");
+      fail();
+    } catch (NoSuchThingException e) {
+      // expected
+    }
+  }
+
+
   static class PackageCacheTester {
+    private class ManualDiffAwareness implements DiffAwareness {
+      private View lastView;
+      private View currentView;
+
+      @Override
+      public View getCurrentView() {
+        lastView = currentView;
+        currentView = new View() {};
+        return currentView;
+      }
+
+      @Override
+      public ModifiedFileSet getDiff(View oldView, View newView) {
+        if (oldView == lastView && newView == currentView) {
+          return Preconditions.checkNotNull(modifiedFileSet);
+        } else {
+          return ModifiedFileSet.EVERYTHING_MODIFIED;
+        }
+      }
+
+      @Override
+      public String name() {
+        return "PackageCacheTester.DiffAwareness";
+      }
+
+      @Override
+      public void close() {
+      }
+    }
+
+    private class ManualDiffAwarenessFactory implements DiffAwareness.Factory {
+      @Nullable
+      @Override
+      public DiffAwareness maybeCreate(Path pathEntry) {
+        return pathEntry == workspace ? new ManualDiffAwareness() : null;
+      }
+    }
+
     private final ManualClock clock;
     private final Path workspace;
     private final Path outputBase;
     private final Reporter reporter = new Reporter();
     private final SkyframeExecutor skyframeExecutor;
     private final List<Path> changes = new ArrayList<>();
     private boolean everythingModified = false;
+    private ModifiedFileSet modifiedFileSet;
 
     public PackageCacheTester(
         FileSystem fs, ManualClock clock, Preprocessor.Factory.Supplier supplier)
@@ -410,7 +473,7 @@ public PackageCacheTester(
               null, /* BinTools */
               null, /* workspaceStatusActionFactory */
               TestRuleClassProvider.getRuleClassProvider().getBuildInfoFactories(),
-              ImmutableList.<DiffAwareness.Factory>of(),
+              ImmutableList.of(new ManualDiffAwarenessFactory()),
               Predicates.<PathFragment>alwaysFalse(),
               supplier,
               ImmutableMap.<SkyFunctionName, SkyFunction>of(),
@@ -494,12 +557,13 @@ private ModifiedFileSet getModifiedFileSet() {
     void sync() throws InterruptedException {
       clock.advanceMillis(1);
 
+      modifiedFileSet = getModifiedFileSet();
       skyframeExecutor.preparePackageLoading(
           new PathPackageLocator(outputBase, ImmutableList.of(workspace)),
           ConstantRuleVisibility.PUBLIC, true, 7, "",
           UUID.randomUUID());
       skyframeExecutor.invalidateFilesUnderPathForTesting(
-          new Reporter(), getModifiedFileSet(), workspace);
+          new Reporter(), modifiedFileSet, workspace);
       ((SequencedSkyframeExecutor) skyframeExecutor).handleDiffs(new Reporter());
 
       changes.clear();

src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java

@@ -93,8 +93,8 @@ public class ArtifactFunctionTest {
   @Before
   public final void setUp() throws Exception  {
     setupRoot(new CustomInMemoryFs());
-    AtomicReference<PathPackageLocator> pkgLocator =
-        new AtomicReference<>(new PathPackageLocator(root));
+    AtomicReference<PathPackageLocator> pkgLocator = new AtomicReference<>(new PathPackageLocator(
+        root.getFileSystem().getPath("/outputbase"), ImmutableList.of(root)));
     ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper(pkgLocator, false);
     differencer = new RecordingDifferencer();
     evaluator =

src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java

@@ -33,6 +33,7 @@
 import com.google.common.collect.Sets;
 import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
@@ -280,7 +281,6 @@ public void testExternalRelativeSymlink() throws Exception {
         getFilesSeenAndAssertValueChangesIfContentsOfFileChanges("../outside", true, "a"));
     assertThat(seenFiles)
         .containsExactly(
-            rootedPath("WORKSPACE"),
             rootedPath("a"),
             rootedPath(""),
             RootedPath.toRootedPath(fs.getRootDirectory(), PathFragment.EMPTY_FRAGMENT),
@@ -298,13 +298,36 @@ public void testAbsoluteSymlink() throws Exception {
         getFilesSeenAndAssertValueChangesIfContentsOfFileChanges("/absolute", true, "a"));
     assertThat(seenFiles)
         .containsExactly(
-            rootedPath("WORKSPACE"),
             rootedPath("a"),
             rootedPath(""),
             RootedPath.toRootedPath(fs.getRootDirectory(), PathFragment.EMPTY_FRAGMENT),
             RootedPath.toRootedPath(fs.getRootDirectory(), new PathFragment("absolute")));
   }
 
+  @Test
+  public void testAbsoluteSymlinkToExternal() throws Exception {
+    String externalPath =
+        outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX).getRelative("a/b").getPathString();
+    symlink("a", externalPath);
+    file("b");
+    file(externalPath);
+    Set<RootedPath> seenFiles = Sets.newHashSet();
+    seenFiles.addAll(getFilesSeenAndAssertValueChangesIfContentsOfFileChanges("b", false, "a"));
+    seenFiles.addAll(
+        getFilesSeenAndAssertValueChangesIfContentsOfFileChanges(externalPath, true, "a"));
+    Path root = fs.getRootDirectory();
+    assertThat(seenFiles)
+        .containsExactly(
+            rootedPath("WORKSPACE"),
+            rootedPath("a"),
+            rootedPath(""),
+            RootedPath.toRootedPath(root, PathFragment.EMPTY_FRAGMENT),
+            RootedPath.toRootedPath(root, new PathFragment("output_base")),
+            RootedPath.toRootedPath(root, new PathFragment("output_base/external")),
+            RootedPath.toRootedPath(root, new PathFragment("output_base/external/a")),
+            RootedPath.toRootedPath(root, new PathFragment("output_base/external/a/b")));
+  }
+
   @Test
   public void testSymlinkAsAncestor() throws Exception {
     file("a/b/c/d");

src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java

@@ -99,8 +99,8 @@ public final void setUp() throws Exception  {
     FileSystemUtils.createEmptyFile(pkgRoot.getRelative("WORKSPACE"));
 
     tsgm = new TimestampGranularityMonitor(BlazeClock.instance());
-    AtomicReference<PathPackageLocator> pkgLocator =
-        new AtomicReference<>(new PathPackageLocator(pkgRoot));
+    AtomicReference<PathPackageLocator> pkgLocator = new AtomicReference<>(new PathPackageLocator(
+        fs.getPath("/output_base"), ImmutableList.of(pkgRoot)));
     ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper(pkgLocator, false);
     skyFunctions.put(SkyFunctions.FILE_STATE, new FileStateFunction(tsgm, externalFilesHelper));
     skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator));

src/test/shell/bazel/skylark_repository_test.sh

@@ -120,6 +120,26 @@ EOF
 
 }
 
+function test_load_from_symlink_to_outside_of_workspace() {
+  OTHER=$TEST_TMPDIR/other
+
+  cat > WORKSPACE<<EOF
+load("/a/b/c", "c")
+EOF
+
+  mkdir -p $OTHER/a/b
+  touch $OTHER/a/b/BUILD
+  cat > $OTHER/a/b/c.bzl <<EOF
+def c():
+  pass
+EOF
+
+  touch BUILD
+  ln -s $TEST_TMPDIR/other/a a
+  bazel build //:BUILD || fail "Failed to build"
+  rm -fr $TEST_TMPDIR/other
+}
+
 function tear_down() {
   true
 }