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

PosixFS can get PathStats for a set of PathBufs #5783

Merged
merged 3 commits into from May 9, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented May 4, 2018

Currently, it can only do so for globs. For local process execution, we
don't want to glob across the output files, we want to use a list of
literal paths.

@illicitonion illicitonion requested review from stuhood and ity May 4, 2018

@@ -573,9 +616,10 @@ pub trait VFS<E: Send + Sync + 'static>: Clone + Send + Sync + 'static {
///
/// TODO: Should handle symlink loops (which would exhibit as an infinite loop in expand).
///
fn canonicalize(&self, symbolic_path: PathBuf, link: Link) -> BoxFuture<Option<PathStat>, E> {
fn canonicalize(&self, link: Link) -> BoxFuture<Option<PathStat>, E> {

This comment has been minimized.

@illicitonion

illicitonion May 4, 2018

Contributor

As far as I can tell symbolic_path is always identical to link.0

If this isn't correct, please let me know how to populate symbolic_path in my new caller :)

This comment has been minimized.

@stuhood

stuhood May 4, 2018

Member

See the failing unit test in https://travis-ci.org/pantsbuild/pants/jobs/374986336:

E   AssertionError: Lists differ: ['a/4.txt.ln'] != [u'a/4.txt.ln', u'd.ln/4.txt.l...
E   
E   Second list contains 1 additional elements.
E   First extra element 1:
E   d.ln/4.txt.ln
E   
E   - ['a/4.txt.ln']
E   + [u'a/4.txt.ln', u'd.ln/4.txt.ln']
_ FSTest.test_walk_recursive_trailing_doublestar _
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 4, 2018

For local process execution, we don't want to glob across the output files, we want to use a list of literal paths.

Is there a good reason not to support globs? I've always assumed we would be supporting output globs.

illicitonion added some commits May 4, 2018

PosixFS can get PathStats for a set of PathBufs
Currently, it can only do so for globs. For local process execution, we
don't want to glob across the output files, we want to use a list of
literal paths.
@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented May 8, 2018

For local process execution, we don't want to glob across the output files, we want to use a list of literal paths.

Is there a good reason not to support globs? I've always assumed we would be supporting output globs.

[ETA: Let's continue this discussion on https://github.com//pull/5784 because this is use-case driven]

The remote execution API supports capturing individual files, and capturing whole directories, but nothing in between. My plan is to add directory capturing when we have a concrete use case for it.

If we find this isn't flexible enough, we should draft a change to the API, and standardise it with folks, but I'm hoping we can get by with just files and directories.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/local-output-files/1 branch from 4280542 to 77a0957 May 8, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 8, 2018

Mm. Ok.

@stuhood

stuhood approved these changes May 8, 2018

@illicitonion illicitonion merged commit 058159b into pantsbuild:master May 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/local-output-files/1 branch May 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment