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

populate parent directories of process outputs locally to match remote execution api #7133

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Jan 23, 2019

Problem

Resolves #7084.

Solution

straightforward

Result

We now match this corner of the remote execution API in local process executions in the v2 engine.

@cosmicexplorer cosmicexplorer force-pushed the output-directories-local-process-execution branch from 829069d to b3d81b9 Compare January 23, 2019 09:26
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.

You should be able to test this in the test module lower down in this file, by trying to run something like:

touch foo/bar

and seeing that it succeeds because foo exists (whereas before this change, it would fail because file not found)

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Jan 23, 2019

You should be able to test this

Thanks! I have done so in 2ab4633, wasn't thinking too hard last night. Also had to fix a couple other tests with now-unnecessary mkdirs!

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.

Thanks! One small restructure and I think we're good to go :)

@Eric-Arellano
Copy link
Contributor

I'm not sure how this relates to #7048. Did you mean a different issue number in the problem section?

@cosmicexplorer
Copy link
Contributor Author

Thanks, changed to #7084! Typical 2am hack sesh...

@cosmicexplorer
Copy link
Contributor Author

Thanks! One small restructure and I think we're good to go :)

@illicitonion Sorry, could you describe the restructure necessary? I can't seem to find it in the PR comments, my eyes could be missing something.

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.

Ugh, github dropped my comment somehow :(

// create_dir_all() will ensure all parents are created. At that point, we might consider
// explicitly enumerating all the directories to be created and just using create_dir(),
// unless there is some optimization in create_dir_all() that makes that less efficient.
.collect::<HashSet<_>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be more clear as:

let parent_paths_to_create: HashSet<_> = stuff.collect();
for path in parent_paths_to_create {
  create_dir_all(&path).map_err(...)?;
}
Ok(())

rather than using a chain of two iter-collect pairs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

github lost my reply somehow! I wanted to say thank you because as much as I love .collect() this is significantly easier to write and read and I wasn't really remembering the utility of ?!

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.

LGTM - thanks!

@cosmicexplorer cosmicexplorer merged commit fa899b1 into pantsbuild:master Jan 25, 2019
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.

3 participants