[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian added inline comments. Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +// mismatch relies on resolving the real path and checking that casing differs. +// If we use %t and we

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian marked 4 inline comments as done. MrTrillian added a comment. Resolved remaining comments addressed with the FileManager.cpp change to branch on the native path format being Windows. This is ready to merge! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian marked an inline comment as done. MrTrillian added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:663 +} else { + llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true); + CanonicalName =

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 545250. MrTrillian marked an inline comment as done. MrTrillian added a comment. @benlangmuir I made the root-preserving canonicalization logic Windows-only now that I found how to test for that. It changes the code shape a little but I think it's an

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian marked an inline comment as done. MrTrillian added inline comments. Comment at: llvm/utils/lit/lit/LitConfig.py:192 f = f.f_back.f_back -file = os.path.abspath(inspect.getsourcefile(f)) +file =

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:663 +} else { + llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true); + CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage); benlangmuir wrote: >

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 545218. MrTrillian marked 4 inline comments as done. MrTrillian added a comment. Reverted some `os.path.abspath` that had been converted to `abs_path_preserve_drive` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian marked an inline comment as done. MrTrillian added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:663 +} else { + llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true); + CanonicalName =

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 544743. MrTrillian added a comment. Reduced path small strings length to 256 (incidentally close to MAX_PATH on Windows), per @benlangmuir 's comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. Ping. Any brave reviewer to help here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. Responded to comment. Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +// mismatch relies on resolving the real path and checking that casing differs. +// If

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 543677. MrTrillian added a comment. Fixed `clang-format` issues and improved test comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 Files: clang/include/clang/Basic/FileManager.h

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian marked an inline comment as done. MrTrillian added a comment. Marking comments as resolved per my reply. I'm not sure if that's best practice! @tahonermann Looking forward to reviews CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian marked an inline comment as done. MrTrillian added inline comments. Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +// mismatch relies on resolving the real path and checking

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. In D154130#4516226 , @tahonermann wrote: > It is unclear to me why/when we would ever want the substitute drive > expansion; the modified tests aren't very elucidating. My naive expectation > is that we effectively want to

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 542922. MrTrillian retitled this revision from "[lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations" to "[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations". MrTrillian edited the summary of this revision. CHANGES SINCE

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 542097. MrTrillian added a comment. Undo whitespace changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 Files: clang/include/clang/Basic/FileManager.h clang/lib/Basic/FileManager.cpp

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 542094. MrTrillian edited the summary of this revision. MrTrillian added a comment. Update clang path canonicalization to avoid resolving substitute drives (ie resolving to a path under a different root). This requires much less change to tests.

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. > That could impose a significant performance and drive space penalty in the > event that substitute drive paths are changed, but I would expect such > changes to be rare. I think the more likely case is that one would build their repo under the substitute drive

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. > I took a look at the code and it looks to me like it would be safe to change > ModuleMap::canonicalizeModuleMapPath() to use a drive preserving > canonicalization Symbolic links can point across drives so I think a drive preserving canonicalization cannot be much

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. I would appreciate help with moving this code review forwards. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 ___ cfe-commits mailing list

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. In D154130#4487292 , @jdenny wrote: > 3. Extend lit's own test suite to cover it. I submitted an update with your suggestions #1 and #2 but this #3 is much more difficult because of the nature of `%{t:real}`. I'm not sure I

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian updated this revision to Diff 539683. MrTrillian added a comment. - Renamed `safe_abs_path` to `abs_path_preserve_drive` @tahonermann - Renamed `%>t` and `%>/t` to `%{t:real}` and `%{/t:real}` @tahonermann - Renamed `PREFIX_EXPANDED` to `SUBMODULE_PREFIX` @aaron.ballman - Documented

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added reviewers: jansvoboda11, benlangmuir. MrTrillian added a comment. Add reviewers from https://reviews.llvm.org/D134923 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a subscriber: benlangmuir. MrTrillian added a comment. In D154130#4486898 , @tahonermann wrote: >> 95% of the %>t are around clang modulemap files, because that code resolves >> real paths in C++ by design, so I can't avoid it. > > Can

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. In D154130#4481673 , @aaron.ballman wrote: > Adding a few more folks who are interested in lit changes to try to get the > review unstuck. > > FWIW, I worry about the subtlety of the `>` change because it's not entirely >

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. @rnk , I would appreciate your review on this since you helped with the previous iteration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. All premerge build failures seem like flukes. - `x64 windows` failed 1/3 times - `x64 debian` failed 2/3 times with a timeout (passes locally) - `libcxx` seems to be failing for everyone: https://buildkite.com/llvm-project/libcxx-ci Repository: rG LLVM Github

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian added a comment. Herald added a subscriber: wangpc. How do I take action on the test failure? It runs without issues on my machine: tristan@nuc-on-wood:~/project-llvm$ python3 build/bin/llvm-lit -sv --filter test.toy build/tools/mlir /test/ Testing Time: 0.55s Excluded:

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

MrTrillian created this revision. MrTrillian added reviewers: rnk, compnerd, nlopes, Jake-Egan. Herald added a subscriber: delcypher. Herald added a reviewer: ributzka. Herald added a project: All. MrTrillian requested review of this revision. Herald added a reviewer: dang. Herald added projects: