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 295c8c2e6e2703..77104d996b1ee3 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); + // If we expect a symlink, we can readlink it directly and handle errors appropriately - there + // is no need for the stat below. + 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 965d51ca567455..6ba4671e0d5e90 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 @@ -98,6 +98,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; @@ -1541,8 +1542,13 @@ private boolean checkOutputs( success = false; if (output.isTreeArtifact()) { reportOutputTreeArtifactErrors(action, output, reporter, e); + } else if (output.isSymlink() && e instanceof NotASymlinkException) { + reporter.handle( + Event.error( + action.getOwner().getLocation(), + String.format("declared output '%s' is not a symlink", output.prettyPrint()))); } else { - // Are all exceptions caught due to missing files? + // 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 36447c18f3b51d..924841448ff23b 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 @@ -49,10 +49,8 @@ public DigestHashFunction getDigestFunction() { return digestFunction; } - /** - * An exception thrown when attempting to resolve an ordinary file as a symlink. - */ - protected static final class NotASymlinkException extends IOException { + /** An exception thrown when attempting to resolve an ordinary file as a symlink. */ + 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 c92afbda0d877f..27cf9ae494806b 100755 --- a/src/test/shell/bazel/bazel_symlink_test.sh +++ b/src/test/shell/bazel/bazel_symlink_test.sh @@ -44,6 +44,8 @@ source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ #### SETUP ############################################################# +add_to_bazelrc "startup --windows_enable_symlinks" + # `uname` returns the current platform, e.g "MSYS_NT-10.0" or "Linux". # `tr` converts all upper case letters to lower case. # `case` matches the result if the `uname | tr` expression to string prefixes @@ -69,11 +71,6 @@ fi function expect_symlink() { local file=$1 - if "$is_windows"; then - # By default, WindowsFileSystem#createSymbolicLink() copies instead of symlinking: - # https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java;l=96;drc=e86a93b9fba865c3374a5a7ccdabc9863035ef78 - return 0 - fi if [[ ! -L "$file" ]]; then fail "expected '$file' to be a symlink" fi @@ -104,18 +101,29 @@ def _write_impl(ctx): write = rule(implementation = _write_impl, attrs = {"contents": attr.string()}) EOF + # We use python rather than a simple ln since the latter doesn't handle dangling symlinks on + # Windows. + mkdir -p symlink_helper + cat > symlink_helper/BUILD < symlink_helper/symlink_helper.py < 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" @@ -141,15 +144,10 @@ EOF } 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" @@ -160,15 +158,10 @@ EOF } 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 @@ -177,7 +170,7 @@ EOF 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 @@ -186,15 +179,10 @@ EOF } 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 @@ -203,7 +191,7 @@ EOF 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 @@ -213,15 +201,10 @@ EOF } 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 @@ -230,7 +213,7 @@ EOF 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 @@ -239,15 +222,10 @@ EOF } 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 @@ -256,7 +234,7 @@ EOF 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 @@ -265,11 +243,6 @@ EOF } 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/a.bzl <<'EOF' def _bad_symlink_impl(ctx): @@ -298,17 +271,23 @@ EOF cat > a/BUILD <<'EOF' load(":a.bzl", "bad_symlink", "bad_write") -bad_symlink(name="bs", link_target="/badsymlink") +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"], + cmd = "$(location //symlink_helper) bad/symlink $@", + tools = ["//symlink_helper"], +) EOF bazel build --experimental_allow_unresolved_symlinks //a:bsg >& $TEST_log && fail "build succeeded" - expect_log "a/bs is not a symlink" + expect_log "declared output 'a/bs' is not a symlink" bazel 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 file or directory" @@ -348,20 +327,16 @@ EOF } 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/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._link, + arguments = [ctx.attr.link_target, symlink.path], inputs = depset([]), - command = "ln -s " + ctx.attr.link_target + " " + symlink.path, ) ctx.actions.run_shell( outputs = [output], @@ -370,25 +345,30 @@ 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(), + "_link": attr.label( + default = "//symlink_helper", + executable = True, + cfg = "exec", + ), + } +) EOF cat > a/BUILD <<'EOF' load(":a.bzl", "a") -a(name="a", link_target="/somewhere/over/the/rainbow") +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 + 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 +391,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 + assert_contains "input link is .*[/\\]somewhere/in/my/heart" bazel-bin/a/a.file } function test_symlink_file_to_file_created_from_symlink_action() { @@ -728,7 +708,7 @@ genrule( ) EOF - bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:exec || fail "build failed" + bazel build --experimental_allow_unresolved_symlinks //a:exec || fail "build failed" } run_suite "Tests for symlink artifacts"