r269100 - [VFS] Reconstruct the VFS overlay tree for more accurate lookup

[Sean Silva via cfe-commits]

On Tue, May 10, 2016 at 11:14 PM, Bruno Cardoso Lopes <
bruno.cardoso at gmail.com> wrote:

> Hi Sean,
>
> > So sorry! It had gone quiet for a few hours and I wanted to avoid
> leaving it
> > red any longer.
>
> No problem, the priority is to get bot greens!
>
> > Does any platform define a guaranteed directory iteration order? I feel
> like
> > depending on it in the first place is unwise (or maybe I'm
> misunderstanding?
> > e.g. I'm thinking that we should sort the results of directory iteration
> to
> > guarantee a particular order if needed for a comparison in a test)
>
> Yes, I was thinking about that too, however I'm not yet sure this is
> actually happening due to iteration order.
>
> >>
> >> Or can you help me debug it once I land it again?
> >
> >
> > Sure, but like I mentioned the nature of this error is very troubling to
> me
> > (high-level API use running into low-level system issues). It suggests
> that
> > there is an incorrect layering or something. I hate these annoying
> windows
> > path issues as much as everyone else, so I'd rather we focus energy on
> > building an appropriate layering to consolidate all the special handling
> in
> > one place than to continually band-aid issues as they creep into all
> parts
> > of the codebase. VirtualFileSystem.cpp already has quite a bit of
> technical
> > debt that violates our policies in this regard (platform-specific
> #ifdef's
> > outside libSupport; I count 3 in VirtualFileSystem.cpp which is a huge
> > number considering how recent that code is).
>
> Totally agree. Part of that is on me - I'm actually relying on
> http://reviews.llvm.org/D19842 as a base to do a proper clean up by
> using a target independent version of realpath. FileManager is likely
> benefiting from that as well (and has itself some layering violations
> I took precedence from). However, I'm not even sure what are the root
> causes that make stuff fails on windows, given that, it's hard to know
> what proper layering should look like, because aside windows-avoid
> hacks, the rest looks fine to me; I count on you then to help me with
> that, feel free to file PRs about anything specific :)
>

I'm glad to help, but it makes me uneasy to have somebody working on a
filesystem abstraction that does not have ready access to test and debug
their changes across the major host platforms that LLVM supports (linux,
mac, windows). Is there any way you can get access? I don't think that
windows 10 is critical here; win7 or win8 are fine.

-- Sean Silva


>
> >>
> >> I might
> >> check-in the new tests separately from the rest to make it easier to
> >> revert.
> >
> >
> > I don't think there's precedent for doing this in LLVM (or at least it
> would
> > be considered as bad practice). We typically check in tests alongside the
> > code being tested specifically to avoid the temptation to leave
> > untested/broken code in tree.
>
> True. However it's kinda subtle in this case: (1) this patch is not
> really a feature, it just re-arranges the tree to allow faster search,
> so the current tests passing should be enough. OTOH, the side-effect
> is that it fixes directory iteration because different roots pointing
> to the same path now can be seen as the same by directory iteration. I
> had the intention to test that, to make sure we don't regress in the
> future. (2) I have no other way to "debug" on windows10-ps4 platform
> besides reducing-by-commit. Anyway, I'm gonna try a more minimal
> testcase first.
>
> --
> Bruno Cardoso Lopes
> http://www.brunocardoso.cc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160510/57ee0f5b/attachment.html>