Directory outputs contain symlinks in 0.17.1rc1

[jayconrod]

Description of the problem / feature request:

Originally reported by @mhlopko in bazel-contrib/rules_go#1669

rules_go has a rule called go_path which can produce a directory tree in a couple of different modes. In copy mode, the contents of the directory are expected to be regular files and subdirectories. They are written as regular files, but other rules that consume this output see them as symbolic links in the sandbox.

This can break tools like tar if rule authors aren't careful to dereference symbolic links. This is new behavior in 0.17.1rc1, and I don't see any mention of it in the release notes, so I think it's a regression. If this is intended behavior going forward, I'll likely need to remove copy mode from go_path.

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

This gist defines a very simple rule that reproduces the problem and consumes the output of the rule in a test.

What operating system are you running Bazel on?

Linux, amd64

What's the output of bazel info release?

0.17.1rc1

Any other information, logs, or outputs that you want to share?

@mhlopko pointed to #5721 as the point where this issue came up, but it happens for me locally without using a Docker image and doesn't seem related.

cc @philwo

[philwo]

I ran a git bisect with your repro example (thanks!) and found this commit as the culprit: d4d3d50

@buchgr Any idea?

[philwo]

I had a look at this. The difference in behavior is basically: Where Bazel's sandbox before the change would symlink the directory of runfiles, it now traverses the directory structure and symlinks each file individually. For Bazel without sandboxing this doesn't make a difference and indeed, the repro works fine with Bazel HEAD with disabled sandboxing.

The fact that Bazel's sandbox represents input files as symbolic links is absolutely not great, but working as intended (because we don't know of any better mechanism to make input files appear in a sandbox directory) and has always been the case (except for this edge case where Bazel would not know the contents of certain directories). It's known that this is risky - e.g. your example of "tar" above. The solution is to write rules and tools that can correctly deal with symlinks.

See this, for example:

$ touch WORKSPACE

$ cat > BUILD <<'EOF'
genrule(
    name = "tar",
    srcs = glob(["inputs/*"]),
    outs = ["output.tar"],
    cmd = "tar cvf \"$@\" $(SRCS)",
)
EOF

$ mkdir inputs
$ touch inputs/{1,2,3}.txt

$ bazel clean; bazel build :tar
INFO: Clean command is running, shutting down worker pool...
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.
INFO: Analysed target //:tar (7 packages loaded).
INFO: Found 1 target...
INFO: From Executing genrule //:tar:
inputs/1.txt
inputs/2.txt
inputs/3.txt
Target //:tar up-to-date:
  bazel-genfiles/output.tar
INFO: Elapsed time: 2.648s, Critical Path: 0.10s
INFO: 1 process: 1 linux-sandbox.
INFO: Build completed successfully, 2 total actions

$ tar tvf bazel-genfiles/output.tar 
lrwxrwxrwx philwo/primarygroup 0 2018-08-28 15:26 inputs/1.txt -> /usr/local/google/home/philwo/.cache/bazel/_bazel_philwo/292af7c156526bec758bef1eb31303e3/execroot/__main__/inputs/1.txt
lrwxrwxrwx philwo/primarygroup 0 2018-08-28 15:26 inputs/2.txt -> /usr/local/google/home/philwo/.cache/bazel/_bazel_philwo/292af7c156526bec758bef1eb31303e3/execroot/__main__/inputs/2.txt
lrwxrwxrwx philwo/primarygroup 0 2018-08-28 15:26 inputs/3.txt -> /usr/local/google/home/philwo/.cache/bazel/_bazel_philwo/292af7c156526bec758bef1eb31303e3/execroot/__main__/inputs/3.txt

We might be able to revert to the previous behavior for the "I have a directory in my runfiles" case. I'll investigate, but also wanted to point out that the symlink stuff is (and has always been) like this for normal inputs.

[jayconrod]

@philwo Thanks for looking into this. I opened this issue because I wasn't sure this change was intended. Since it is, I'll consider this resolved, but I'll leave the issue open if you want to investigate further.

I'll disable the failing test in bazel-contrib/rules_go#1669 for now, and I'll reconsider the copy mode in go_path.

[laurentlb]

If a change in Bazel is breaking existing rules, shall we roll it back?

[philwo]

I still don't know what the right thing to do is. I think it's possible to symlink directories instead of the files that they contain in the sandbox. This would even give us a performance boost (because creating potentially lots of individual symlinks is slow). We have a nice repro here thanks to @jayconrod that we can also turn into an integration test.

I have to think about it a few more days and look at the code. I don't think we should rollback the culprit commit unless we think that it's wrong in general. AFAIK it's an improvement for other things - we just didn't think about the impact on the sandboxing code.

[laurentlb]

It's breaking users, so it's not a net improvement. I would suggest a rollback. Then you can take your time and decide what's the best way forward. But I don't think it's okay to introduce a regression and break users, without warning them or having a clear migration plan.

[philwo]

@laurentlb This is all fine, but I'm the wrong person to talk to - the culprit is not my change and I only have a rough idea of what it does and absolutely no context of why it was created.

It's absolutely possible that rolling back that change will unbreak this use case and completely break remote execution for everyone out there, for example.

Could you discuss this with @buchgr (author of the change)?

Personally I don't mind. We can keep it as is, fix the sandbox to work with the new behavior of TreeArtifacts or roll back the culprit CL if that is deemed the best option considering the big picture.

I just want to point out again that you cannot rely on files in the sandbox being regular files. It's not in the API contract. Files in a sandbox (or remote) environment could be anything. Symlinks. Hardlinks. Bind mounts. The whole thing could be a FUSE filesystem. They could be read-only. I have no clue. There are simply no guarantees and we even have multiple strategies implementing various of the just mentioned.

[jayconrod]

I just want to point out again that you cannot rely on files in the sandbox being regular files. It's not in the API contract. Files in a sandbox (or remote) environment could be anything. Symlinks. Hardlinks. Bind mounts. The whole thing could be a FUSE filesystem. They could be read-only. I have no clue. There are simply no guarantees and we even have multiple strategies implementing various of the just mentioned.

This is the part that was unclear to me before. I think this may be a case of Hiram's law. :)