ctx.actions.symlink doesn't make relative symlink, contrary to docs

[pauldraper]

ctx.actions.symlink always makes absolute symlinks when target_path is used, regardless of whether target_path is absolute.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

WORKSAPCE.bazel

BUILD.bazel

load(":rules.bzl", "example")

example(
  name = "example",
)

rules.bzl

def _example_impl(ctx):
    example1 = ctx.actions.declare_symlink("example1")
    ctx.actions.symlink(output = example1, target_path = "../..")

    example2 = ctx.actions.declare_symlink("example2")
    ctx.actions.run(executable = "ln", arguments = ["-s", "../..", example2.path], outputs = [example2])

    return DefaultInfo(
        files = depset([example1, example2])
    )

example = rule(
  implementation = _example_impl
)

$ bazel build --experimental_allow_unresolved_symlinks :example
$ ls -l bazel-bin/
lrwxrwxrwx 1 paul paul 68 Nov  4 01:06 example1 -> /home/paul/.cache/bazel/_bazel_paul/fc54cd89f1e9b3d4a2b86f2842cad4ca
lrwxrwxrwx 1 paul paul  5 Nov  4 01:06 example2 -> ../..

I expected both symlinks to be ''../.." but only the latter (using executable ln) is.

Documentation: https://docs.bazel.build/versions/main/skylark/lib/actions.html#symlink

(Experimental) The exact path that the output symlink will point to. No normalization or other processing is applied. Access to this feature requires setting --experimental_allow_unresolved_symlinks.

What operating system are you running Bazel on?

Ubuntu 18.04

What's the output of bazel info release?

4.2.1

[philsc]

I ran into this as well. The issue is that, despite the documentation saying otherwise, the first thing SymlinkAction does is to turn the unresolved path into a PathFragment. The first thing a PathFragment does is to normalize the path.

I don't know what the right answer to fix this is, but I think that's the reason for the issue.

[pauldraper]

My fix was to use the ln executable, and everything worked fine. (Platform-specific of course.)

[brandjon]

This is surprising and contrary to documentation, though I will note that making symlinks to paths (as opposed to artifacts) is still experimental.

[fmeum]

@lberki @brandjon I'm currently working on resolving the cluster of issues around symlinks. Would you support adding a new basic operation to Bazel's file system abstraction that allows creating unnormalized symlinks (taking a String rather than a PathFragment)? Without this, I don't see a way to achieve the correct behavior

[lberki]

@fmeum I think there isn't another way to fix this bug, think so please go ahead (unless @haxorz objects but I don't think he will)

My guess is that this comes from the time when PathFragment wasn't forcibly normalized and thus it worked at that time, and we didn't test this case so it got accidentally broken at some point in time.

I'd much rather you replace FileSystem.createSymbolicLink(PathFragment) than add an overload. That will make merging a bit more difficult (we have an internal implementation of FileSystem), but carries less risk of having a more complicated interface forever.

[fmeum]

@lberki The only alternative I see is to specify the allowed operations on a PathFragment created with createAlreadyNormalized and mandate implementations of createSymbolicLink to use only these operations. That approach may not even require any changes to code, at the cost of encoding usage restrictions in documentation only. What do you think?

[lberki]

Yeah, strictly speaking, that's an alternative, but that one has a larger blast radius than a change localized to FileSystem.

The normalization was forcibly added in a729b9b with the reasoning that it would help RAM use. I don't know more history. @meisterT maybe you remember something I don't?

There is also 775d3a9 which replaced PathFragment with String specifically to support non-normalized paths, which makes me thing that doing so it the right thing to do here, too.

[fmeum]

@meteorcloudy While working on @lberki's recommendation to make FileSystem.createSymbolicLink take target in as a String rather than a PathFragment, I stumbled upon 6c07525 as well as the file/directory type check. Do you see any problems with the following approach that would allow for non-normalized symlink targets?

1. If createSymbolicLinks is true, let WindowsFileSystem.createSymbolicLink use WindowsFileOperations.createSymlink directly without any file type or existence checks.
2. If it is not set, proceed with the current logic to either create a junction or a copy.

Do you know whether using symlinks for directories (e.g., under the Bazel convenience symlinks) instead of junctions has any drawbacks?

[fmeum]

While working on a PR for this, I noticed https://cs.opensource.google/bazel/bazel/+/5d75c08d9542d35d90047369059cad2e03c6a1d4:src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java;l=229, which is an error raised when trying to create a symlink to a directory. https://cs.opensource.google/bazel/bazel/+/cffd417a4c4ec4d082f815f57a771ad1010972dd does not contain any explanation for why that should be an error though.

[meteorcloudy]

@fmeum Please see https://docs.google.com/document/d/17YIqUdffxpwcKP-0whHM6TFELN8VohTpjiiEIbbRfts/edit#heading=h.hw82qobcal5z

Basically, creating symlinks on Windows requires some extra privilege, so we decided to always use junction for "directory symlink" and only when --windows_enable_symlinks is on, we try to create file symlink (otherwise, we copy the file).

In generally, I think it's safe to change the behaviour to what you described (IIUC):

• If --windows_enable_symlinks is on, create symlink for both file and directory.
• If --windows_enable_symlinks is off, create junction for directory and copy file.

[fmeum]

Thanks for the information, I got with that approach then.

@meteorcloudy Do you know whether symlinks on Windows can use forward slash paths? If not, then I should probably translate the path style in WindowsFileSystem.{create,read}SymbolicLink or it will be very difficult to make declare_symlink work cross-platform.

[meteorcloudy]

@fmeum I think the CreateSymbolicLink API itself doesn't support forward slash paths. But we do translate the paths first before passing to the JNI functions. See https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java;l=225

[fmeum]

@meteorcloudy Thanks for pointing me to that function, that was the missing piece.

Unfortunately, I don't think that the current ctx.actions.symlink(target_path=...) API can work on Windows. According to the CreateSymbolicLink docs and as verified by my tests, the type of the eventual target of a symlink has to be specified during creation. Since it isn't known to Bazel in the case of an unresolved symlink, I think that we would have to introduce an additional boolean flag indicating the desired type of the target.

[meteorcloudy]

I think that we would have to introduce an additional boolean flag indicating the desired type of the target.

Can we use Files.isDirectory(target.toRealPath());
at https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java;l=90 to tell if the resolved path is a directory or not?

Related: https://docs.oracle.com/javase/7/docs/api/java/nio/file/Path.html#toRealPath(java.nio.file.LinkOption...)

[fmeum]

We could if we wanted to create a resolvable symlink, but in this particular case we might very well want to create a symlink pointing to ../../node-fetch without that path even existing yet.

I actually think that we should just commit to the following restriction: On Windows, ctx.actions.symlink(target_path=...) will always create a directory symlink if the target doesn't exist. This is because:

1. It doesn't require adding a type attribute to ctx.actions.symlink and, more importantly, the remote execution API, just to deal with a Windows oddity.
2. Given how recent of an addition symlinks are on Windows, allowing a strict superset of what dangling junctions could do (namely, allowing relative paths) should cover the needs of Windows specific tooling.
3. Directories seem like much more interesting targets for unresolved symlinks to begin with (see PNPM, which is my main driver for working on this feature) and directory symlinks can at least somewhat emulate file symlinks.

@gregmagolan Die PNPM rely on unresolved file symlinks in any way?

[meteorcloudy]

I did some experiments with mklink

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# touch file

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# mklink /D link_s file
symbolic link created for link_s <<===>> file

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# mklink /J link_j file
Junction created for link_j <<===>> file

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# echo hi > file

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# cat link_s
hi

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# cat link_j
hi

pcloudy@PCLOUDY2-W C:\Users\pcloudy\tmp
# dir
...
 Directory of C:\Users\pcloudy\tmp

07/06/2022  11:30 AM    <DIR>          .
07/06/2022  11:30 AM    <DIR>          ..
07/06/2022  11:30 AM                 7 file
07/06/2022  11:30 AM    <JUNCTION>     link_j [C:\Users\pcloudy\tmp\file]
07/06/2022  11:30 AM    <SYMLINKD>     link_s [file]
               1 File(s)              7 bytes

It looks like it actually works if a directory symlink or junction pointing to a file. So I guess yes, I agree with "On Windows, ctx.actions.symlink(target_path=...) will always create a directory symlink if the target doesn't exist". But maybe we keep the current behaviour of creating a junction since it doesn't require admin right?

[fmeum]

@meteorcloudy Thanks for the experiment, that's really helpful.

But maybe we keep the current behaviour of creating a junction since it doesn't require admin right?

Can a junction point to a relative path though? I thought that wasn't possible, but could you maybe give it a try (I don't have access to a Windows machine).

Admin access should no longer be required if we use SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE, at least using developer mode.

[meteorcloudy]

Oh, yeah, junction can only take absolute path, see https://superuser.com/questions/343074/directory-junction-vs-directory-symbolic-link

Admin access should no longer be required if we use SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE, at least using developer mode.

Correct, but IIRC turning on developer mode requires admin right 😅.

Anyway, I'm fine with creating directory symlink if --windows_enable_symlinks is on. But if that flag is off, we have to fallback to create junction I guess? (which means ctx.actions.symlink cannot work correctly on Windows).