Only depend on the WORKSPACE file for external files that are under the 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
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index bd93fd2..d469208 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -239,7 +239,7 @@
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());
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index b50b7df..1e8f0f1 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -299,7 +299,7 @@
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),
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
index 24d0c5d..04ab650 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
@@ -109,7 +109,7 @@
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(),
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
index 4fe5227..2cebe83 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
@@ -383,7 +383,69 @@
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;
@@ -391,6 +453,7 @@
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 @@
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 @@
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();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index f7cb450..5ca1354 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -93,8 +93,8 @@
@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 =
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index ca7de18..873d48d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/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 @@
getFilesSeenAndAssertValueChangesIfContentsOfFileChanges("../outside", true, "a"));
assertThat(seenFiles)
.containsExactly(
- rootedPath("WORKSPACE"),
rootedPath("a"),
rootedPath(""),
RootedPath.toRootedPath(fs.getRootDirectory(), PathFragment.EMPTY_FRAGMENT),
@@ -298,7 +298,6 @@
getFilesSeenAndAssertValueChangesIfContentsOfFileChanges("/absolute", true, "a"));
assertThat(seenFiles)
.containsExactly(
- rootedPath("WORKSPACE"),
rootedPath("a"),
rootedPath(""),
RootedPath.toRootedPath(fs.getRootDirectory(), PathFragment.EMPTY_FRAGMENT),
@@ -306,6 +305,30 @@
}
@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");
symlink("f", "a/b/c");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
index 933d5ad..01111af 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -99,8 +99,8 @@
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));
diff --git a/src/test/shell/bazel/skylark_repository_test.sh b/src/test/shell/bazel/skylark_repository_test.sh
index 0dcc4c5..35511f4 100755
--- a/src/test/shell/bazel/skylark_repository_test.sh
+++ b/src/test/shell/bazel/skylark_repository_test.sh
@@ -120,6 +120,26 @@
}
+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
}
