Populate output_directory for remote execution responses

[illicitonion]

Currently, ExecuteProcessRequest specifies a list of output file paths that it wants to know about if they're generated by the process, and ExecuteProcessResult has a Digest of a Directory representing these outputs.

Local process execution reads these files from disk, and populates the output Directory Digest.

Remote process execution does not.

The ActionResult protocol buffer message has a list of OutputFile messages which are populated by the remote server.

We need to take this list, turn it into a Directory, and populate it on the ExecuteProcessResult.

The steps to do this are, roughly:

1. If the content is inline in the OutputFile, store it in the store and get the digest.
2. Construct a Directory from the list of file names, digests, and is_executable bits. We already have code to do this from a list of PathStats - maybe we want to re-use this, constructing PathStats for each File (easy to do), and passing in a custom StoreFileByDigest implementation which just looks up the (already known) digest from the filename. Alternatively, maybe we just want to write fresh code to do this, as it shouldn't be too much code.
3. Store that Directory in the Store (so it can be looked up in the future), and set the field on the ExecuteProcessResult.

[stuhood]

What are the thoughts behind eager fetching of outputs? As it stands, the v1 product exposed by JVMCompile requires that the files exist locally, but I figured that we'd want to deprecate that guarantee soon. An explicit synchronous call to warm/pre-fetch might make sense for the JVM compile usecase... I could also imagine fetching by default, but only in the background?

…

On Mon, Apr 16, 2018, 5:32 PM Daniel Wagner-Hall ***@***.***> wrote: Currently, ExecuteProcessRequest <https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/lib.rs#L25-L48> doesn't specify the list of output file paths it expects to be generated by the process execution, the local <https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/local.rs#L9-L30> and remote <https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/remote.rs#L51-L144> executors don't fetch them and put them in the Store, and ExecuteProcessResult <https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/lib.rs#L53-L58> doesn't expose them. We should fix all of the above. Once we have the ability to recursively download Directories <#5708>, we should: 1. Add expected output files to ExecuteProcessRequest <https://github.com/pantsbuild/pants/blob/628c83d5ab2706b0f64d69568c57a718ec7c5e2a/src/rust/engine/process_execution/src/lib.rs#L27> . 2. Populate it on the Action <https://github.com/pantsbuild/pants/blob/628c83d5ab2706b0f64d69568c57a718ec7c5e2a/src/rust/engine/process_execution/src/remote.rs#L183-L185> . 3. Call the fetch code from #5708 <#5708> when we get a response <https://github.com/pantsbuild/pants/blob/628c83d5ab2706b0f64d69568c57a718ec7c5e2a/src/rust/engine/process_execution/src/remote.rs#L219-L223> to prime the Store with the output. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5709>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC2lF_HboGm_E7D8_jhAvDq3ImwPmE2ks5tpLmhgaJpZM4TWw0E> .

[illicitonion]

My goal is to make remoting be a drop-in replacement for what currently exists, and then to make optimisations in the future. I find it incredibly likely we'll want to make output fetching lazy in the future, but it's a pure optimisation and is not on the critical path for getting something working end-to-end :)

[stuhood]

Hm, ok. I suppose eagerly fetching doesn't mean eagerly materializing.