New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate output_directory in ExecuteProcessResponse #5896

Merged
merged 6 commits into from Jun 4, 2018

Conversation

Projects
None yet
2 participants
@dotordogh
Copy link
Contributor

dotordogh commented Jun 1, 2018

Problem

See #5709
tl;dr: We don't currently populate the output_files field in the response from remote.

Solution

Now we populate the output files and tests have been added to verify the behavior.

@dotordogh dotordogh requested review from illicitonion , stuhood and ity Jun 1, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Woo!

output_file_path_buf.clone(),
output_file.get_digest().into(),
);
PathStat::file(

This comment has been minimized.

@illicitonion

illicitonion Jun 1, 2018

Contributor

This PathStat is constructed identically in both branches, so do it outside the if rather than twice?

},
)
} else {
let raw_content = output_file.get_content().into();

This comment has been minimized.

@illicitonion

illicitonion Jun 1, 2018

Contributor

&[u8].into() actually copies the underlying bytes here to make a new Bytes. Instead, if you: output_file.contents.clone() it will just do a reference-count increment of a shared buffer :)

match self.map_of_paths_to_digests.get(&file.path) {
Some(digest) => future::ok(digest.clone()),
None => future::err(format!(
"Error when trying to find digest for path {:?}",

This comment has been minimized.

@illicitonion

illicitonion Jun 1, 2018

Contributor

Maybe "Didn't know digest for path in remote execution response"?

let store = self.store.clone();
future::join_all(futures)
.and_then(|_| {
let path_wrap_mutex = Arc::try_unwrap(path_map_2).unwrap();

This comment has been minimized.

@illicitonion

illicitonion Jun 1, 2018

Contributor

Throw in a comment to justify that this unwrap is safe (because we've joined on all of the futures that had references to the Arc)

@@ -1267,6 +1379,11 @@ mod tests {
}
}

#[test]
fn extract_output_files_from_request() {
// to be implemented once everything compiles

This comment has been minimized.

@illicitonion

illicitonion Jun 1, 2018

Contributor

TODO :)

@illicitonion illicitonion merged commit b558b01 into pantsbuild:master Jun 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment