From a77939a00f6997aad7c6b8bc161ac8ec43eb60f6 Mon Sep 17 00:00:00 2001 From: ichern Date: Tue, 28 May 2019 17:10:10 +0200 Subject: [PATCH 1/5] Allow WORKSPACE file to be a symlink if no managed directories is used. Fixes #8475. Introduced in 0.26 with managed directories support changes. --- .../skyframe/SequencedSkyframeExecutor.java | 23 ++++++------ .../ManagedDirectoriesBlackBoxTest.java | 35 +++++++++++++++++++ .../workspace/WorkspaceBlackBoxTest.java | 22 ++++++++++++ 3 files changed, 67 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 a760a30b578f41..a70dca687829c7 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.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 0c949c8391575e..0a688a452fde62 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 @@ -20,8 +20,10 @@ import com.google.devtools.build.lib.blackbox.framework.PathUtils; import com.google.devtools.build.lib.blackbox.framework.ProcessResult; import com.google.devtools.build.lib.blackbox.junit.AbstractBlackBoxTest; +import com.google.devtools.build.lib.blackbox.tests.workspace.WorkspaceBlackBoxTest; 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 +333,39 @@ public void testRepositoryOverrideChangeToConflictWithManagedDirectories() throw + " have managed directories: @generated_node_modules"); } + /** + * The test to verify the assertion 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\",\n" + + "managed_directories = {'@generated_node_modules': ['node_modules']})\n" + + "\n" + + "load(\":use_node_modules.bzl\", \"generate_fine_grained_node_modules\")\n" + + "\n" + + "generate_fine_grained_node_modules(\n" + + " name = \"generated_node_modules\",\n" + + " package_json = \"//:package.json\",\n" + + ")\n"); + 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..f4422ce5bc5aff 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,28 @@ public void testPathWithSpace() throws Exception { bazel.help(); } + @Test + public void testWorkspaceFileIsSymlink() throws Exception { + 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()); } From a1ef42bc8ce27a80c9e7f095bc5da5d665790fad Mon Sep 17 00:00:00 2001 From: ichern Date: Tue, 28 May 2019 17:54:53 +0200 Subject: [PATCH 2/5] Correct review comments --- .../ManagedDirectoriesBlackBoxTest.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) 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 0a688a452fde62..e8ac438ac39468 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 @@ -334,8 +334,7 @@ public void testRepositoryOverrideChangeToConflictWithManagedDirectories() throw } /** - * The test to verify the assertion that WORKSPACE file can not be a symlink, - * when managed directories are used. + * 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()} @@ -349,15 +348,15 @@ public void testWorkspaceSymlinkThrowsWithManagedDirectories() throws Exception Path tempWorkspace = Files.createTempFile(context().getTmpDir(), WORKSPACE, ""); PathUtils.writeFile(tempWorkspace, - "workspace(name = \"fine_grained_user_modules\",\n" - + "managed_directories = {'@generated_node_modules': ['node_modules']})\n" - + "\n" - + "load(\":use_node_modules.bzl\", \"generate_fine_grained_node_modules\")\n" - + "\n" - + "generate_fine_grained_node_modules(\n" - + " name = \"generated_node_modules\",\n" - + " package_json = \"//:package.json\",\n" - + ")\n"); + "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("//..."); From f7c21f7df92b8f62d2c6d630e8f2d39e43fb12bf Mon Sep 17 00:00:00 2001 From: ichern Date: Tue, 28 May 2019 19:03:18 +0200 Subject: [PATCH 3/5] Workspace file state value can be null if there was a problem calculating it. --- .../devtools/build/lib/skyframe/SequencedSkyframeExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a70dca687829c7..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 @@ -904,7 +904,7 @@ private boolean refreshWorkspaceHeader(ExtendedEventHandler eventHandler) FileStateValue newFileStateValue = maybeInvalidateWorkspaceFileStateValue(workspacePath); WorkspaceFileValue newValue = (WorkspaceFileValue) evaluateSingleValue(workspaceFileKey, eventHandler); - if (!newValue.getManagedDirectories().isEmpty() + 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.", From 5dc3c7b24104fbfdab2fe3aee3a09635dbe1d773 Mon Sep 17 00:00:00 2001 From: ichern Date: Tue, 28 May 2019 19:36:48 +0200 Subject: [PATCH 4/5] remove not needed import of WorkspaceBlackBoxTest --- .../tests/manageddirs/ManagedDirectoriesBlackBoxTest.java | 1 - 1 file changed, 1 deletion(-) 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 e8ac438ac39468..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 @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.blackbox.framework.PathUtils; import com.google.devtools.build.lib.blackbox.framework.ProcessResult; import com.google.devtools.build.lib.blackbox.junit.AbstractBlackBoxTest; -import com.google.devtools.build.lib.blackbox.tests.workspace.WorkspaceBlackBoxTest; import com.google.devtools.build.lib.util.ResourceFileLoader; import java.io.IOException; import java.nio.file.Files; From 0417885f7169929ccc011adaba7761fa1d517ecf Mon Sep 17 00:00:00 2001 From: ichern Date: Wed, 29 May 2019 13:18:24 +0200 Subject: [PATCH 5/5] Do not test WORKSPACE as symlink on Windows. --- .../lib/blackbox/tests/workspace/WorkspaceBlackBoxTest.java | 4 ++++ 1 file changed, 4 insertions(+) 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 f4422ce5bc5aff..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 @@ -227,6 +227,10 @@ public void testPathWithSpace() throws Exception { @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();