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

[remoting] Store raw bytes for stdout and stderr if received inline. #5855

Merged
merged 7 commits into from May 24, 2018

Conversation

Projects
None yet
3 participants
@kwlzn
Copy link
Member

kwlzn commented May 22, 2018

Fixes #5849

.map_err(move |error| {
ExecutionError::Fatal(format!("Error storing raw stdout: {:?}", error))
})
.wait();

This comment has been minimized.

@stuhood

stuhood May 22, 2018

Member

As discussed in slack, if you replace these waits with:

.map(|_| stdout_copy)
.to_boxed()

...then you should be good to go!

Adding a test would be good of course though.

This comment has been minimized.

@kwlzn

kwlzn May 22, 2018

Member

ah, nice. the map(|_| stdout_copy) was the bit I was missing. thanks!

will circle back with tests.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented May 22, 2018

Looks great, thanks! Everything should be in place to enable you to write that test - there's already a mock server which can return you inline stdout/stderr, and you should be able to make a store and assert that it contains some content :)

@kwlzn kwlzn changed the title [WIP] Store raw bytes for stdout and stderr if received inline. [remoting] Store raw bytes for stdout and stderr if received inline. May 22, 2018

@kwlzn kwlzn requested review from stuhood , illicitonion and ity May 22, 2018

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented May 22, 2018

added a test - this should be ready for review now.

};

let store_dir = TempDir::new("store").unwrap();
let cas = mock::StubCAS::with_content(1024, vec![], vec![TestDirectory::containing_roland()]);

This comment has been minimized.

@illicitonion

illicitonion May 23, 2018

Contributor

I would probably use a mock::StubCAS::empty() here, and maybe an action which doesn't have an input directory. Otherwise it's a little unclear/confounding to work out what was added and what was already there.

This comment has been minimized.

@kwlzn

kwlzn May 23, 2018

Member

fixed!


{
assert_eq!(
cmd_runner

This comment has been minimized.

@illicitonion

illicitonion May 23, 2018

Contributor

This test would possibly be a little more obviously correct if the store used for this check was a newly-constructed Store::local_only rather than re-using the Store which can backfill from the remote CAS, because it would guarantee that the bytes already exist on disk.

This comment has been minimized.

@kwlzn

kwlzn May 23, 2018

Member

fixed! thanks for the tip(s).

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good!

.map_err(move |error| {
ExecutionError::Fatal(format!("Error storing raw stdout: {:?}", error))
})
.map(|_| stdout_copy)

This comment has been minimized.

@stuhood

stuhood May 23, 2018

Member

@illicitonion : Based on what you were saying about the necessity of move for closures, this shouldn't work... but it seems to be. I think that this implies that unambiguous moves are executed automatically?

@kwlzn kwlzn force-pushed the twitter:kwlzn/5849 branch from ff015de to c40e020 May 24, 2018

@kwlzn kwlzn merged commit 3d7908c into pantsbuild:master May 24, 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