[PATCH] D109128: [VFS] Use original path when falling back to external FS

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc972175649f4: [VFS] Use original path when falling back to external FS (authored by keith). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 387030. keith added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 Files: clang/test/VFS/relative-path-errors.c

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D109128#3097060 , @keith wrote: > @dexonsmith turns out the test I was adding is broken on main today too. If > it's alright with you I

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. @JDevlieghere can you take another pass? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 ___ cfe-commits mailing list

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. @dexonsmith can you take another look? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 ___ cfe-commits mailing list

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. @dexonsmith turns out the test I was adding is broken on main today too. If it's alright with you I will punt that to another diff to not increase the scope of this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 383406. keith added a comment. Remove broken test for now Turns out this fails on main as well, so I'll punt this to another change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added inline comments. Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1738 + +TEST_F(VFSFromYAMLTest, RelativePathHitWithoutCWD) { + IntrusiveRefCntPtr BaseFS( So this test case is actually failing. The difference between it and the others

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180 + if (ExternalFS) +ExternalFS->setCurrentWorkingDirectory(Path); + keith wrote: > dexonsmith wrote: > > keith wrote: > > > JDevlieghere wrote: > > > > I'm

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 380821. keith added a comment. Fix test paths on windows Otherwise this resulted in a json string with non-escaped backslashes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180 + if (ExternalFS) +ExternalFS->setCurrentWorkingDirectory(Path); + dexonsmith wrote: > keith wrote: > > JDevlieghere wrote: > > > I'm pretty sure there was a reason

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 380802. keith marked 2 inline comments as done. keith added a comment. Remove `cd` of ExternalFS Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 Files:

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180 + if (ExternalFS) +ExternalFS->setCurrentWorkingDirectory(Path); + keith wrote: > JDevlieghere wrote: > > I'm pretty sure there was a reason we stopped doing

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 380125. keith added a comment. Improve lit test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 Files: clang/test/VFS/relative-path-errors.c

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 379882. keith added a comment. Fix format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 Files: clang/test/VFS/relative-path-errors.c

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 379881. keith marked an inline comment as done. keith added a comment. Extract fallback status logic to another function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180 + if (ExternalFS) +ExternalFS->setCurrentWorkingDirectory(Path); + JDevlieghere wrote: > I'm pretty sure there was a reason we stopped doing this. There should be >

[PATCH] D109128: [VFS] Use original path when falling back to external FS

JDevlieghere added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1179-1180 + if (ExternalFS) +ExternalFS->setCurrentWorkingDirectory(Path); + I'm pretty sure there was a reason we stopped doing this. There should be some discussion

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. Ok I've updated the diff here based on @dexonsmith's original suggestion, and some offline discussion with @JDevlieghere The new logic remaps the files and statuses, in the fallback to the external FS case, to use the originally requested path. I opted not to use the

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 379559. keith marked 4 inline comments as done. keith added a comment. Update to remap file paths after fetching them from the external FS Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added a comment. In D109128#3000374 , @vsapsai wrote: > In D109128#2999846 , @dexonsmith > wrote: > >> In D109128#2997588 , @JDevlieghere >> wrote: >> >>>

[PATCH] D109128: [VFS] Use original path when falling back to external FS

vsapsai added a comment. In D109128#2999846 , @dexonsmith wrote: > In D109128#2997588 , @JDevlieghere > wrote: > >> Keith and I discussed this offline. My suggestion was to do the following: >> >> 1. Check the

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added a comment. In D109128#3000157 , @JDevlieghere wrote: > Yes, Keith and I came to the same conclusion yesterday. I was worried about > tracking both paths at all times, but I like your suggestion of only changing > the path when

[PATCH] D109128: [VFS] Use original path when falling back to external FS

JDevlieghere added a comment. Yes, Keith and I came to the same conclusion yesterday. I was worried about tracking both paths at all times, but I like your suggestion of only changing the path when requested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added a comment. (BTW, does this problem affect OverlayFileSystem as well?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 ___ cfe-commits mailing

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added a comment. In D109128#2997588 , @JDevlieghere wrote: > Keith and I discussed this offline. My suggestion was to do the following: > > 1. Check the overlay for the canonicalized path > 2. Check the fall-through for the canonicalized

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. In D109128#2997588 , @JDevlieghere wrote: > If I understand correctly, this patch does that, but swaps 2 and 3. What's > the motivation for trying the non-canonical path first? If we treat the > current working directory as a

[PATCH] D109128: [VFS] Use original path when falling back to external FS

JDevlieghere added a comment. Keith and I discussed this offline. My suggestion was to do the following: 1. Check the overlay for the canonicalized path 2. Check the fall-through for the canonicalized path 3. Check the fall-through for the original path If I understand correctly, this patch

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 372041. keith added a comment. Handle all cases by passing relative path to ExternalFS first Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 Files:

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. I'm realizing more now that these changes break the original intent of rG0be9ca7c0f9a733f846bb6bc4e8e36d46b518162 as well, given that it seems like the core canonicalization of the path before passing

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. I think I might have to apply this same logic to the other functions changed in the original commit as well, this seems to solve this isolated case, but I think there are some more cases that aren't currently fixed by this Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith added a comment. In D109128#2982780 , @JDevlieghere wrote: > I'm sure I'm missing something, but after rereading the patch several times I > still don't see the functional change. It just looks like it's renaming every > instance of `Path` to

[PATCH] D109128: [VFS] Use original path when falling back to external FS

JDevlieghere added a comment. I'm sure I'm missing something, but after rereading the patch several times I still don't see the functional change. It just looks like it's renaming every instance of `Path` to `CanonicalPath` and `Path_` to `Path`? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 370480. keith marked 2 inline comments as done. keith added a comment. Update variable name and add unit test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 Files:

[PATCH] D109128: [VFS] Use original path when falling back to external FS

vsapsai added a comment. Was looking at an issue caused by over-eager RedirectingFileSystem path canonicalization and this patch fixes it. The repro we have is more complicated (involves 3 modules), so I don't think it is worth including with this change. Comment at:

[PATCH] D109128: [VFS] Use original path when falling back to external FS

dexonsmith added reviewers: vsapsai, dexonsmith. dexonsmith requested changes to this revision. dexonsmith added a subscriber: vsapsai. dexonsmith added a comment. This revision now requires changes to proceed. I think @vsapsai was looking at this as well. This mostly LGTM, but I think this can

[PATCH] D109128: [VFS] Use original path when falling back to external FS

varungandhi-apple added a comment. cc @JDevlieghere Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109128/new/ https://reviews.llvm.org/D109128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith updated this revision to Diff 370155. keith added a comment. Some of the other checks were invalid given the current tests, these seem to be enough to solve this issue without breaking existing tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D109128: [VFS] Use original path when falling back to external FS

keith created this revision. keith added a reviewer: JDevlieghere. Herald added subscribers: dexonsmith, hiraditya. keith requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This is a follow up to