From fcef37931af341c93de70c2b804a4d588e6b10c6 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 19 Jun 2022 17:30:20 +0200 Subject: [PATCH] Fix artifact/symlink mismatch detection The check in ActionMetadataHandler that an output declared to be a symlink is indeed created as a symlink by the action was ineffective as it would only run if a stat of the output showed that is already was a symlink. The test only passed by accident since it runs sandboxed and the sandbox setup would call Path.readSymbolicLink on what it expects to be a symlink. As this does not happen on Windows, the test correctly fails there. This is fixed by calling Path.readSymbolicLink on the output path of an expected symlink before rather than after stat-ing it and trusting the file type contained in the stat info. With this issue fixed, bazel_symlink_test can be enabled on Windows with the following test-only changes: * Pass --windows_enable_symlinks as a startup option. * Use relative instead of absolute symlink targets as these have consistent shape under Unix and Windows. * Use a helper java_binary to create symlinks rather than `ln -s`, which doesn't seem to be able to create unresolved symlinks on Windows. --- .../lib/skyframe/ActionMetadataHandler.java | 10 +- .../lib/skyframe/SkyframeActionExecutor.java | 12 +- .../devtools/build/lib/vfs/FileSystem.java | 2 +- .../google/devtools/build/lib/vfs/Path.java | 8 +- src/test/shell/bazel/bazel_symlink_test.sh | 190 +++++++++--------- 5 files changed, 116 insertions(+), 106 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 018fea486156fe..fe35b7941d5dd2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -600,6 +600,12 @@ private static FileArtifactValue fileArtifactValueFromArtifact( checkState(!artifact.isTreeArtifact() && !artifact.isMiddlemanArtifact(), artifact); Path pathNoFollow = artifactPathResolver.toPath(artifact); + // Handle the case where a symlink is expected before stat-ing the output so that the case where + // it incorrectly is a file triggers the expected NotASymlinkException. + if (artifact.isSymlink()) { + return FileArtifactValue.createForUnresolvedSymlink(pathNoFollow); + } + RootedPath rootedPathNoFollow = RootedPath.toRootedPath( artifactPathResolver.transformRoot(artifact.getRoot().getRoot()), @@ -618,10 +624,6 @@ private static FileArtifactValue fileArtifactValueFromArtifact( rootedPathNoFollow, statNoFollow, digestWillBeInjected, xattrProvider, tsgm); } - if (artifact.isSymlink()) { - return FileArtifactValue.createForUnresolvedSymlink(pathNoFollow); - } - // We use FileStatus#isSymbolicLink over Path#isSymbolicLink to avoid the unnecessary stat // done by the latter. We need to protect against symlink cycles since // ArtifactFileMetadata#value assumes it's dealing with a file that's not in a symlink cycle. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 5b0610b441fb6c..740987ff39177c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -97,6 +97,7 @@ import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException; import com.google.devtools.build.lib.vfs.OutputService; import com.google.devtools.build.lib.vfs.OutputService.ActionFileSystemType; import com.google.devtools.build.lib.vfs.Path; @@ -1534,8 +1535,15 @@ private boolean checkOutputs( if (output.isTreeArtifact()) { reportOutputTreeArtifactErrors(action, output, reporter, e); } else { - // Are all exceptions caught due to missing files? - reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink(), e); + if (e instanceof NotASymlinkException) { + reporter.handle(Event.error( + action.getOwner().getLocation(), + String.format("declared output '%s' is not a symlink", output.prettyPrint()))); + } else { + // Are all other exceptions caught due to missing files? + reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink(), + e); + } } } } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java index 67a225b5740888..b63e97625c75b9 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java @@ -51,7 +51,7 @@ public DigestHashFunction getDigestFunction() { /** * An exception thrown when attempting to resolve an ordinary file as a symlink. */ - protected static final class NotASymlinkException extends IOException { + public static final class NotASymlinkException extends IOException { public NotASymlinkException(PathFragment path) { super(path.getPathString() + " is not a symlink"); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java index cdec2d70f382fb..d0fa82d2e59531 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java @@ -492,8 +492,8 @@ public void createSymbolicLink(PathFragment target) throws IOException { * an {@link UnsupportedOperationException} if the link points to a non-existent file. * * @return the content (i.e. target) of the symbolic link - * @throws IOException if the current path is not a symbolic link, or the contents of the link - * could not be read for any reason + * @throws FileSystem.NotASymlinkException if the current path is not a symbolic link. + * @throws IOException if the contents of the link could not be read for any reason */ public PathFragment readSymbolicLink() throws IOException { return fileSystem.readSymbolicLink(asFragment()); @@ -504,8 +504,8 @@ public PathFragment readSymbolicLink() throws IOException { * are intentionally left underspecified otherwise to permit efficient implementations. * * @return the content (i.e. target) of the symbolic link - * @throws IOException if the current path is not a symbolic link, or the contents of the link - * could not be read for any reason + * @throws FileSystem.NotASymlinkException if the current path is not a symbolic link. + * @throws IOException if the contents of the link could not be read for any reason */ public PathFragment readSymbolicLinkUnchecked() throws IOException { return fileSystem.readSymbolicLinkUnchecked(asFragment()); diff --git a/src/test/shell/bazel/bazel_symlink_test.sh b/src/test/shell/bazel/bazel_symlink_test.sh index 5e0772c247781d..27daef622142d3 100755 --- a/src/test/shell/bazel/bazel_symlink_test.sh +++ b/src/test/shell/bazel/bazel_symlink_test.sh @@ -107,170 +107,141 @@ EOF } function test_smoke() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/BUILD < a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent") +dangling_symlink(name="a", link_target="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_not_log "running genrule" } function test_on_disk_cache_symlinks() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent") +dangling_symlink(name="a", link_target="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" bazel shutdown - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_not_log "running genrule" } function test_no_inmemory_cache_symlinks() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent") +dangling_symlink(name="a", link_target="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent2") +dangling_symlink(name="a", link_target="non/existent2") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" } function test_no_on_disk_cache_symlinks() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent") +dangling_symlink(name="a", link_target="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent2") +dangling_symlink(name="a", link_target="non/existent2") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF bazel shutdown - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" } function test_replace_symlink_with_file() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent") +dangling_symlink(name="a", link_target="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "write") -write(name="a", contents="/nonexistent") +write(name="a", contents="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" } function test_replace_file_with_symlink() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "write") -write(name="a", contents="/nonexistent") +write(name="a", contents="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" cat > a/BUILD <<'EOF' load("//symlink:symlink.bzl", "dangling_symlink") -dangling_symlink(name="a", link_target="/nonexistent") +dangling_symlink(name="a", link_target="non/existent") genrule(name="g", srcs=[":a"], outs=["go"], cmd="echo running genrule; echo GO > $@") EOF - bazel build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:g >& $TEST_log || fail "build failed" expect_log "running genrule" } function test_file_instead_of_symlink() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a + cat > a/MakeSymlink.java <<'EOF' +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +public class MakeSymlink { + public static void main(String[] args) throws IOException { + Files.createSymbolicLink(Paths.get(args[0]), Paths.get(args[1])); + } +} +EOF + cat > a/a.bzl <<'EOF' def _bad_symlink_impl(ctx): symlink = ctx.actions.declare_symlink(ctx.label.name) @@ -298,22 +269,34 @@ EOF cat > a/BUILD <<'EOF' load(":a.bzl", "bad_symlink", "bad_write") -bad_symlink(name="bs", link_target="/badsymlink") +java_binary( + name = "MakeSymlink", + srcs = ["MakeSymlink.java"], + main_class = "MakeSymlink", +) + +bad_symlink(name="bs", link_target="bad/symlink") genrule(name="bsg", srcs=[":bs"], outs=["bsgo"], cmd="echo BSGO > $@") bad_write(name="bw", contents="badcontents") genrule(name="bwg", srcs=[":bw"], outs=["bwgo"], cmd="echo BWGO > $@") -genrule(name="bg", srcs=[], outs=["bgo"], cmd = "ln -s /badsymlink $@") +genrule( + name="bg", + srcs=[], + outs=["bgo"], + exec_tools = [":MakeSymlink"], + cmd = "$(execpath :MakeSymlink) $@ bad/symlink", +) EOF - bazel build --experimental_allow_unresolved_symlinks //a:bsg >& $TEST_log && fail "build succeeded" - expect_log "a/bs is not a symlink" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:bsg >& $TEST_log && fail "build succeeded" + expect_log "declared output 'a/bs' is not a symlink" - bazel build --experimental_allow_unresolved_symlinks //a:bwg >& $TEST_log && fail "build succeeded" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:bwg >& $TEST_log && fail "build succeeded" expect_log "symlink() with \"target_path\" param requires that \"output\" be declared as a symlink, not a regular file" - bazel build --experimental_allow_unresolved_symlinks //a:bg >& $TEST_log && fail "build succeeded" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:bg >& $TEST_log && fail "build succeeded" expect_log "declared output 'a/bgo' is a dangling symbolic link" } @@ -343,25 +326,32 @@ load(":a.bzl", "bad_symlink") bad_symlink(name="bs") EOF - bazel build --experimental_allow_unresolved_symlinks //a:bs >& $TEST_log && fail "build succeeded" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:bs >& $TEST_log && fail "build succeeded" expect_log "symlink() with \"target_file\" param requires that \"output\" be declared as a regular file, not a symlink" } function test_symlink_created_from_spawn() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a + cat > a/MakeSymlink.java <<'EOF' +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +public class MakeSymlink { + public static void main(String[] args) throws IOException { + Files.createSymbolicLink(Paths.get(args[0]), Paths.get(args[1])); + } +} +EOF + cat > a/a.bzl <<'EOF' def _a_impl(ctx): symlink = ctx.actions.declare_symlink(ctx.label.name + ".link") output = ctx.actions.declare_file(ctx.label.name + ".file") - ctx.actions.run_shell( + ctx.actions.run( outputs = [symlink], + executable = ctx.executable._make_symlink, + arguments = [symlink.path, ctx.attr.link_target], inputs = depset([]), - command = "ln -s " + ctx.attr.link_target + " " + symlink.path, ) ctx.actions.run_shell( outputs = [output], @@ -370,25 +360,35 @@ def _a_impl(ctx): ) return DefaultInfo(files = depset([output])) -a = rule(implementation = _a_impl, attrs = {"link_target": attr.string()}) +a = rule( + implementation = _a_impl, + attrs = { + "link_target": attr.string(), + "_make_symlink": attr.label( + default = ":MakeSymlink", + executable = True, + cfg = "exec", + ) + } +) EOF cat > a/BUILD <<'EOF' load(":a.bzl", "a") -a(name="a", link_target="/somewhere/over/the/rainbow") +java_binary( + name = "MakeSymlink", + srcs = ["MakeSymlink.java"], + main_class = "MakeSymlink", +) +a(name="a", link_target="somewhere/over/the/rainbow") EOF - bazel build --experimental_allow_unresolved_symlinks //a:a || fail "build failed" - assert_contains "input link is /somewhere/over/the/rainbow" bazel-bin/a/a.file + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:a || fail "build failed" + assert_contains "input link is somewhere/over/the/rainbow" bazel-bin/a/a.file } function test_dangling_symlink_created_from_symlink_action() { - if "$is_windows"; then - # TODO(#10298): Support unresolved symlinks on Windows. - return 0 - fi - mkdir -p a cat > a/a.bzl <<'EOF' def _a_impl(ctx): @@ -411,11 +411,11 @@ EOF cat > a/BUILD <<'EOF' load(":a.bzl", "a") -a(name="a", link_target="/somewhere/in/my/heart") +a(name="a", link_target="somewhere/in/my/heart") EOF - bazel build --experimental_allow_unresolved_symlinks //a:a || fail "build failed" - assert_contains "input link is /somewhere/in/my/heart" bazel-bin/a/a.file + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:a || fail "build failed" + assert_contains "input link is .*[/\\]somewhere/in/my/heart" bazel-bin/a/a.file } function test_symlink_created_from_symlink_action() { @@ -444,7 +444,7 @@ load(":a.bzl", "a") a(name="a") EOF - bazel build //a:a || fail "build failed" + bazel --windows_enable_symlinks build //a:a || fail "build failed" assert_contains "Hello, World!" bazel-bin/a/a.link expect_symlink bazel-bin/a/a.link } @@ -470,7 +470,7 @@ load(":a.bzl", "a") a(name="a") EOF - bazel build --experimental_allow_unresolved_symlinks //a:a >& $TEST_log && fail "build succeeded" + bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:a >& $TEST_log && fail "build succeeded" expect_log "\"is_executable\" cannot be True when using \"target_path\"" } @@ -502,7 +502,7 @@ load(":a.bzl", "a") a(name="a") EOF - bazel build //a:a || fail "build failed" + bazel --windows_enable_symlinks build //a:a || fail "build failed" assert_contains "Hello, World!" bazel-bin/a/a.link expect_symlink bazel-bin/a/a.link } @@ -547,7 +547,7 @@ EOF Hello, World! EOF - bazel build //a:a >& $TEST_log && fail "build succeeded" + bazel --windows_enable_symlinks build //a:a >& $TEST_log && fail "build succeeded" expect_log "failed to create symbolic link 'bazel-out/[^/]*/bin/a/a.link': file 'a/foo.txt' is not executable" } @@ -580,7 +580,7 @@ a( ) EOF - bazel build //a:a >& $TEST_log && fail "build succeeded" + bazel --windows_enable_symlinks build //a:a >& $TEST_log && fail "build succeeded" expect_log "cycle in dependency graph" }