From 6ca90ac808016fe8af3793e854a775bd3532fd8f Mon Sep 17 00:00:00 2001 From: ichern Date: Wed, 29 May 2019 07:51:17 -0700 Subject: [PATCH] Allow WORKSPACE file to be a symlink if no managed directories is used. Fixes #8475. Introduced in 0.26 with managed directories support changes. Closes #8480. PiperOrigin-RevId: 250490084 --- .../skyframe/SequencedSkyframeExecutor.java | 24 ++++++------ .../ManagedDirectoriesBlackBoxTest.java | 37 +++++++++++++++++++ .../workspace/WorkspaceBlackBoxTest.java | 27 ++++++++++++++ 3 files changed, 75 insertions(+), 13 deletions(-) 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 c5b2ba888f1690..6aa4637f8f5b2d 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,35 @@ 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) 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 56a004d42c0b49..8a72fea1c6f58c 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 @@ -23,6 +23,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; @@ -340,6 +341,42 @@ 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 { for (String fileName : FILES) { String text = ResourceFileLoader.loadResource(ManagedDirectoriesBlackBoxTest.class, fileName); 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..93d353b0657368 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,33 @@ 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()); }