executor/dirtools: ensure UploadTree set result symlinks

[sluongng]

Currently we only support OutputDirs and OutputFiles. Therefore actions
which produce only symlinks would fail when executed remotely with
BuildBuddy.

See bazel-contrib/rules_go#3539 as an example.

Implement support for OutputFileSymlinks and OutputDirectorySymlinks.

Related issues: Fixes https://github.com/buildbuddy-io/buildbuddy-internal/issues/2234

[sluongng]

To summarize my recent finding:

From Bazel's RemoteExecutionService.java, it seems like Bazel has never made the switch to use output_symlinks field in RE proto.

Instead, it continues using output_directory_symlinks and output_file_symlinks, which led to many confusion on my end why the second iteration of this PR did not work.

Digging up historical records revealed that bazelbuild/remote-apis#96 and bazelbuild/remote-apis#75 were proposed by Googler, but the proto in Bazel was never updated accordingly and only later bump to catchup with some later specs. So it's safe to say that there was very little effort from Bazel side in adopting output_symlinks.

---

Leveraging the clause in RE API spec:

// DEPRECATED as of v2.1. Servers that wish to be compatible with v2.0 API
// should still populate this field in addition to output_symlinks.

Let's support both new and old fields.

[sluongng]

I was considering adding it here

				if !(dirHelper.ShouldUploadFile(fqfn) || dirHelper.IsOutputPath(fqfn)) {
					continue
				}

But I was worried about breaking existing behavior.

It's unclear to me how often people create/include symlink outputs in their actions without declaring them in Command.output_path, and if we should start explicitly requiring folks to declare them.

[sluongng]

Added some basic tests to make this easier to maintain in the long-run 🙏

[bduffany]

The documentation of output_symlinks says:

  // New in v2.1: this field will only be populated if the command
  // `output_paths` field was used, and not the pre v2.1 `output_files` or
  // `output_directories` fields.

So based on this part: this field will only be populated if the command output_paths field was used

I think there should be a check around this line like

if IsOutputPath(fqfn) {
  actionResult.OutputSymlinks = append(actionResult.OutputSymlinks, ospb)
}

However, it looks like IsOutputPath is not technically checking the output_paths field 😕 . In workspace.go > SetTask, we are setting outputPaths to either output_paths if set, else concat(output_files, output_directories). This is pretty confusing IMO - we might want to refactor that. But for now we could maybe just pass in the command proto so that we have access to the original output_paths field.

[bduffany]

The documentation of output_directory_symlinks says:

  // The output directories of the action that are symbolic links to other
  // directories.

similarly for output_file_symlinks:

  // The output files of the action that are symbolic links to other files.

I am interpreting "the output files of the action" to mean Action.command.output_files and "the output directories of the action" to mean Action.command.output_directories.

If that's correct then we need to check that this symlink file's path has an exact match in output_files or output_directories before we add them to the ActionResult.output_file_symlinks or ActionResult.output_directory_symlinks (respectively).

(Note that this function begins directory traversal starting from the workspace root, not any of the specific output directories or output file/symlink paths, otherwise we wouldn't need an explicit check.)

[bduffany]

Looking a lot cleaner! Just a few style/testing-related comments

[bduffany]

I might be misunderstanding but I think an OutputDirectory won't include an OutputFile as its child, otherwise the child will get uploaded twice. I think a more representative example would be to have "b" as an output directory, then in the fileContents map below, create a few files fileB.txt and fileC.txt under b

[bduffany]

Looks great!