Repository: bazelbuild/bazel
Issue: "WORKSPACE file can not be a symlink" exception thrown: backward-incompatible bug
Labels: P1
breakage

Bazel 0.26,
if the WORKSPACE file is a symlink, an exception is thrown.
This is not intended change, it should be thrown only if managed directories are used in the project.

Issue comments:
Thanks for working on the fix! :)
FYI, this issue also breaks tulsi (https://buildkite.com/bazel/tulsi-bazel-darwin/builds/161). CC @DavidGoldman. Patch release with the fix is being prepared.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index a760a30b578f41..0e474f66a79c44 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -901,37 +901,34 @@ private boolean refreshWorkspaceHeader(ExtendedEventHandler eventHandler)
 
     WorkspaceFileValue oldValue =
         (WorkspaceFileValue) memoizingEvaluator.getExistingValue(workspaceFileKey);
-    maybeInvalidateWorkspaceFileStateValue(workspacePath);
+    FileStateValue newFileStateValue = maybeInvalidateWorkspaceFileStateValue(workspacePath);
     WorkspaceFileValue newValue =
         (WorkspaceFileValue) evaluateSingleValue(workspaceFileKey, eventHandler);
+    if (newValue != null && !newValue.getManagedDirectories().isEmpty()
+        && FileStateType.SYMLINK.equals(newFileStateValue.getType())) {
+      throw new AbruptExitException(
+          "WORKSPACE file can not be a symlink if incrementally updated directories are used.",
+          ExitCode.PARSING_FAILURE);
+    }
     return managedDirectoriesKnowledge.workspaceHeaderReloaded(oldValue, newValue);
   }
 
   // We only check the FileStateValue of the WORKSPACE file; we do not support the case
   // when the WORKSPACE file is a symlink.
-  private void maybeInvalidateWorkspaceFileStateValue(RootedPath workspacePath)
+  private FileStateValue maybeInvalidateWorkspaceFileStateValue(RootedPath workspacePath)
       throws InterruptedException, AbruptExitException {
     SkyKey workspaceFileStateKey = FileStateValue.key(workspacePath);
     SkyValue oldWorkspaceFileState = memoizingEvaluator.getExistingValue(workspaceFileStateKey);
-    if (oldWorkspaceFileState == null) {
-      // no need to invalidate if not cached
-      return;
-    }
     FileStateValue newWorkspaceFileState;
     try {
       newWorkspaceFileState = FileStateValue.create(workspacePath, tsgm.get());
-      if (FileStateType.SYMLINK.equals(newWorkspaceFileState.getType())) {
-        throw new AbruptExitException(
-            "WORKSPACE file can not be a symlink if incrementally"
-                + " updated directories feature is enabled.",
-            ExitCode.PARSING_FAILURE);
-      }
     } catch (IOException e) {
       throw new AbruptExitException("Can not read WORKSPACE file.", ExitCode.PARSING_FAILURE, e);
     }
-    if (!oldWorkspaceFileState.equals(newWorkspaceFileState)) {
+    if (oldWorkspaceFileState != null && !oldWorkspaceFileState.equals(newWorkspaceFileState)) {
       recordingDiffer.invalidate(ImmutableSet.of(workspaceFileStateKey));
     }
+    return newWorkspaceFileState;
   }
 
   private SkyValue evaluateSingleValue(SkyKey key, ExtendedEventHandler eventHandler)


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java b/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java
index 0c949c8391575e..e3ea58f30afc65 100644
--- a/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java
+++ b/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java
@@ -22,6 +22,7 @@
 import com.google.devtools.build.lib.blackbox.junit.AbstractBlackBoxTest;
 import com.google.devtools.build.lib.util.ResourceFileLoader;
 import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.List;
@@ -331,6 +332,38 @@ public void testRepositoryOverrideChangeToConflictWithManagedDirectories() throw
                 + " have managed directories: @generated_node_modules");
   }
 
+  /**
+   * The test to verify that WORKSPACE file can not be a symlink when managed directories are used.
+   *
+   * The test of the case, when WORKSPACE file is a symlink, but not managed directories are used,
+   * is in {@link WorkspaceBlackBoxTest#testWorkspaceFileIsSymlink()}
+   */
+  @Test
+  public void testWorkspaceSymlinkThrowsWithManagedDirectories() throws Exception {
+    generateProject();
+
+    Path workspaceFile = context().getWorkDir().resolve(WORKSPACE);
+    assertThat(workspaceFile.toFile().delete()).isTrue();
+
+    Path tempWorkspace = Files.createTempFile(context().getTmpDir(), WORKSPACE, "");
+    PathUtils.writeFile(tempWorkspace,
+        "workspace(name = \"fine_grained_user_modules\",",
+            "managed_directories = {'@generated_node_modules': ['node_modules']})",
+            "",
+            "load(\":use_node_modules.bzl\", \"generate_fine_grained_node_modules\")",
+            "",
+            "generate_fine_grained_node_modules(",
+            "    name = \"generated_node_modules\",",
+            "    package_json = \"//:package.json\",",
+            ")");
+    Files.createSymbolicLink(workspaceFile, tempWorkspace);
+
+    ProcessResult result = bazel().shouldFail().build("//...");
+    assertThat(findPattern(result,
+        "WORKSPACE file can not be a symlink if incrementally updated directories are used."))
+        .isTrue();
+  }
+
   private void generateProject() throws IOException {
     writeProjectFile("BUILD.test", "BUILD");
     writeProjectFile("WORKSPACE.test", "WORKSPACE");
diff --git a/src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace/WorkspaceBlackBoxTest.java b/src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace/WorkspaceBlackBoxTest.java
index 177500e6a94fdb..8700a0e815a515 100644
--- a/src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace/WorkspaceBlackBoxTest.java
+++ b/src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace/WorkspaceBlackBoxTest.java
@@ -225,6 +225,32 @@ public void testPathWithSpace() throws Exception {
     bazel.help();
   }
 
+  @Test
+  public void testWorkspaceFileIsSymlink() throws Exception {
+    if (isWindows()) {
+      // Do not test file symlinks on Windows.
+      return;
+    }
+    Path repo = context().getTmpDir().resolve(testName.getMethodName());
+    new RepoWithRuleWritingTextGenerator(repo).withOutputText("hi").setupRepository();
+
+    Path workspaceFile = context().getWorkDir().resolve(WORKSPACE);
+    assertThat(workspaceFile.toFile().delete()).isTrue();
+
+    Path tempWorkspace = Files.createTempFile(context().getTmpDir(), WORKSPACE, "");
+    PathUtils.writeFile(tempWorkspace, "workspace(name = 'abc')",
+        String.format(
+            "local_repository(name = 'ext', path = '%s',)",
+            PathUtils.pathForStarlarkFile(repo)));
+    Files.createSymbolicLink(workspaceFile, tempWorkspace);
+
+    BuilderRunner bazel = WorkspaceTestUtils.bazel(context());
+    bazel.build("@ext//:all");
+    PathUtils.append(workspaceFile, "# comment");
+    // At this point, there is already some cache workspace file/file state value.
+    bazel.build("@ext//:all");
+  }
+
   private boolean isWindows() {
     return OS.WINDOWS.equals(OS.getCurrent());
   }