RemoteExecutionService: support output_symlinks in ActionResult

[sluongng]

Since Remote API v2.1, both

• output_file_symlinks
• output_directory_symlinks
  were marked as deprecated in favor of output_symlinks.

However, Bazel downloadOutputs has only support the 2 deprecated fields
and not output_symlinks.

Add support for output_symlinks.

[sluongng]

cc: @fmeum as we discussed about this on Slack

[fmeum]

@sluongng https://github.com/bazelbuild/remote-apis/blob/35aee1c4a4250d3df846f7ba3e4a4e66cb014ecd/build/bazel/remote/execution/v2/remote_execution.proto#L1054 reads as if servers could add the same symlinks to both the old fields and the new one, which would cause the current implementation to throw.

[sluongng]

which would cause the current implementation to throw.

I switched from using an Iterable to a Set so I think it should work...
I added a test to demonstrate compatibility just in case.

@fmeum what am I missing? 🤔

[fmeum]

@sluongng Sorry, I missed the fact that you are now building a set. I still see a problem though: The set will deduplicate OutputSymlink messages by equality, but that could mean having different targets or node_properties per path with latest wins semantics. Instead, how about collecting the instances in a map keyed by path and throwing an error if two unequal instances share a key?

[sluongng]

@fmeum I think throwing an error could be a bit hostile toward both users and remote execution server implementations.

The difference between throwing an error vs the current approach should only be how hard is it to deprecate and remove the old fields. In my mind, I would imagine the following steps are required:

• Bazel supporting both ActionResult.output_symlinks (this PR) and Command.output_paths (RemoteExecutionService: Action.Command to set output_paths #18202)
• REAPI removes deprecated fields in a newer version
• Bazel updates to a newer version of REAPI
• Bazel removes usage of old fields

So I would push back and say the current approach of accepting both old and new fields without throwing errors, should be more friendly toward the current ecosystem.

[fmeum]

We may be misunderstanding each other: I fully agree that Bazel should ideally support a mix of both the old and the new fields as long as the values in both fields don't contradict each other.

What I am worried about is a situation where e.g. output_symlinks says that pkg/file should point to foo, whereas output_file_symlinks says that it should point to bar. Within a single field, latest wins semantics may be okay, but across fields, this would potentially introduce discrepancies in behavior between clients. That's why I would find this safer if OutputSymlink instances with identical paths but different other properties across old and new fields would result in an error.

@sluongng Do you have a better understanding of my point now?

[sluongng]

Oh I see 🤔

I don't expect that to happen in reality, but perhaps it would not hurt to do a check for it.
Let me see to it

[sluongng]

Failed MacOS Android test seems to be unrelated to this PR

@tjgq PTAL

[sluongng]

@bazel-io flag

[iancha1992]

@bazel-io fork 6.3.0