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

dyung added a comment. Sorry to comment so late, but I just had time to look into this a little bit. This change broke running lit tests for me on Windows, and I wanted to see if there was a way I could restore it. For reference, my sources are stored locally at `C:\Dev\git\dev`, but I until

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

tahonermann 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 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

tahonermann added a comment. > @tahonermann are all your comments addressed at this point? Apologies for the late response; I was on vacation last week. No. I'm still skeptical that there is ever a desire to expand substitute drives. Comment at:

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

RKSimon added inline comments. Comment at: llvm/utils/lit/lit/discovery.py:60 +cfgpath = util.abs_path_preserve_drive(cfgpath) +target = config_map.get(cfgpath) if target: RKSimon wrote: > RKSimon wrote: > > Found the problem

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

RKSimon added inline comments. Comment at: llvm/utils/lit/lit/discovery.py:60 +cfgpath = util.abs_path_preserve_drive(cfgpath) +target = config_map.get(cfgpath) if target: RKSimon wrote: > Found the problem - you have moved

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

RKSimon added inline comments. Comment at: llvm/utils/lit/lit/discovery.py:60 +cfgpath = util.abs_path_preserve_drive(cfgpath) +target = config_map.get(cfgpath) if target: Found the problem - you have moved the

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

RKSimon added a comment. @MrTrillian This is failing for me with: C:\LLVM\ninja>ninja check-llvm-codegen-x86 [0/1/0/1] Running lit suite C:/LLVM/llvm-project/llvm/test/CodeGen/X86llvm-lit.py: C:\LLVM\llvm-project\llvm\utils\lit\lit\TestingConfig.py:151: fatal: unable to parse config file

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG05d613ea931b: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations (authored by compnerd). Repository: rG LLVM Github Monorepo

[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

benlangmuir 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); MrTrillian wrote: >

[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

benlangmuir accepted this revision. benlangmuir added a comment. This revision is now accepted and ready to land. LGTM other than my suggestion to add a comment. @tahonermann are all your comments addressed at this point? Comment at: clang/lib/Basic/FileManager.cpp:663 +}

[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

benlangmuir 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); MrTrillian wrote: >

[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

benlangmuir added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:655 + SmallString<4096> AbsPathBuf = Name; + SmallString<4096> RealPathBuf; + if (!FS->makeAbsolute(AbsPathBuf)) { 8k is a lot of stack space. The only reason this was 4k in the

[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

tahonermann added a comment. > Marking comments as resolved per my reply. I'm not sure if that's best > practice! Yes, that is fine. 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 +//

[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

tahonermann 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 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