Skip to content
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

Add flags to prosecc_executor that say where to materialize output and what is output #7201

Conversation

cattibrie
Copy link
Contributor

#7180

Problem

The problem is described in #7180

Result

Added flags: --materialize-output-to, --output-file-path and --output-directory-path.
If --materialize-output-to is set the output tree will be copied there.

@illicitonion illicitonion self-requested a review February 4, 2019 13:31
Copy link
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few naming things :)

.long("materialize-output-to")
.takes_value(true)
.required(false)
.help("The name of a directory (which may or may not exist), and will copy the output tree there")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.help("The name of a directory (which may or may not exist), and will copy the output tree there")
.help("The name of a directory (which may or may not exist), where the output tree will be materialized.")

)) as Box<dyn process_execution::CommandRunner>,
};
let mut rt = tokio::runtime::Runtime::new().unwrap();
let result = rt.block_on(runner.run(request)).unwrap();

if let Some(materialize_directory) = args.value_of("materialize-output-to").map(PathBuf::from) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably call this variable output or materialize_directory_destination, rather than materialize_directory

@@ -241,8 +274,8 @@ fn main() {
argv,
env,
input_files,
output_files: BTreeSet::new(),
output_directories: BTreeSet::new(),
output_files: output_file_path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call these variables output_files and output_directories instead of output_file_path and output_directory_path, you can omit the : output_file_path part of this (like with input_files above), which is probably worth doing here because output_files and output_directories are completely meaningful names :)

@illicitonion illicitonion merged commit 89cd939 into pantsbuild:master Feb 4, 2019
stuhood pushed a commit that referenced this pull request Feb 19, 2019
…d what is output (#7201)

Fixes #7180

### Problem
The problem is described in #7180 

### Result

Added flags: `--materialize-output-to`, `--output-file-path` and `--output-directory-path`.
If `--materialize-output-to` is set the output tree will be copied  there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants