reject symlink outputs from cachable actions

[benjaminp]

The remote caching protocol doesn't know about symlinks, and the remote spawn strategy currently converts all symlink into regular files when uploading to the cache. This behavior is insidious because local execution preserves symlinks. So, local and remote execution may yield different results if symlinks are involved:

$ mkdir /tmp/my-cache
$ cat BUILD
genrule(
    name = 'emit-symlink',
    outs = [':a', ':b'],
    cmd = '''
touch $(location :a)
ln -s a $(location :b)
''',
)
$ bazel build --experimental_local_disk_cache --experimental_local_disk_cache_path /tmp/my-cache :emit-symlink
$ ls -lh bazel-genfiles/
total 0
-r-xr-xr-x 1 benjamin benjamin 0 Mar 13 02:35 a
lrwxrwxrwx 1 benjamin benjamin 1 Mar 13 02:35 b -> a
$ bazel clean --expunge
$ bazel build --experimental_local_disk_cache --experimental_local_disk_cache_path /tmp/my-cache :emit-symlink
$ ls -lh bazel-genfiles/
total 0
-r-xr-xr-x 1 benjamin benjamin 0 Mar 14 02:36 a
-r-xr-xr-x 1 benjamin benjamin 0 Mar 14 02:36 b

Handling symlinks in remote caching seems tricky. (What if the link target is absolute?) Therefore, I propose that trying to upload a symlink output to the cache should be an error.

[nicolov]

Agree, this impacted us too until I sprinkled some "no-cache" tags.

[buchgr]

That sounds good to me. We should probably output a decent error message indicating that this action should be tagged "no-cache". Do you want to work on this @benjaminp?

[buchgr]

@nicolov for which rules did you encounter this behavior?

[nicolov]

This applies for all rules that try to symlink. For example, tensorflow has some genrules that gather headers from the system and symlink them into the execroot.

…

On Thu, Mar 15, 2018 at 4:58 AM Jakob Buchgraber ***@***.***> wrote: @nicolov <https://github.com/nicolov> for which rules did you encounter this behavior? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKO4BxnS-hmGeY0KYOWwLw4Xwi4FUQKRks5teldYgaJpZM4SpwOT> .

[benjaminp]

Sure, I can work on a patch.

[buchgr]

Thank you @benjaminp

[buchgr]

Hey @benjaminp,

I have discussed this issue with a few people on the Bazel team:

• We must not cache actions that output symlinks to absolute paths.
• We may cache relative symlinks to inputs or outputs of that particular action, but that's an optimization that doesn't need to be included in your patch (and might make things just more complicated).
• Downstream actions (actions having a symlink output as an input) should resolve the symlink and use the content hash in their action key computation.
• There are mixed opinions about whether trying to upload a symlink should be an error or a warning. I personally lean more towards printing a warning and not caching the action, as it seems more user friendly. What do you think?

[benjaminp]

I have discussed this issue with a few people on the Bazel team:

Thanks for the guidance.

There are mixed opinions about whether trying to upload a symlink should be an error or a warning. I personally lean more towards printing a warning and not caching the action, as it seems more user friendly. What do you think?

I think it should be an error. Warnings are fine for informing users about transient conditions like the remote cache being down. An action producing symlinks is similar to an action not producing promised artifacts. It's an unambiguous deterministic function of the (gen)rule implementation.

[buchgr]

Sounds reasonable :). Thanks.

[ola-rozenfeld]

Am I understanding correctly that this behavior will need to be changed when https://docs.google.com/document/d/1gnOYszitgrLVet3sQk-TKGqIcpkkDsc6aw-izoo-d64/edit# is implemented?

[benjaminp]

Ah, I wasn't aware of that proposal. Yes, if the remote cache correctly handles symlinks, it shouldn't be necessary to ban them.

[buchgr]

@ola-rozenfeld is there a time frame for implementing this?

[ola-rozenfeld]

Yes, we actually plan to do it this quarter. We plan to implement all the outstanding proposals in https://docs.google.com/document/d/12c3oAPgedckLpue2yj0xGgJTEOyUm4mXWWBJ157J-8I/edit?ts=5aabd397# as a major API version change. We are also moving the API protos to our own repo (out of googleapis), to facilitate discussion.

…

On Tue, Apr 10, 2018 at 4:12 AM Jakob Buchgraber ***@***.***> wrote: @ola-rozenfeld <https://github.com/ola-rozenfeld> is there a time frame for implementing this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYoKuE62SCeE6jjmSVK-R_s6ot8AwrPBks5tnGmFgaJpZM4SpwOT> .

[benjaminp]

Can we reopen this since the change was reverted?

[buchgr]

I ll roll the change forward once the no-cache flag has been fixed. Should land sometime next week.

[benjaminp]

I assume you mean the fix for #4343? Can I ask what the new semantics will be?

When preparing #5122, I noticed the roll-forward wasn't clean, so I may need to send a PR anyway.