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

tahonermann added a comment. 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 restore the Python 3.7 behavior. Comment at: clang/test/ExtractAPI/rela

[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 clang/te

[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. CHANGES

[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 and

[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

benlangmuir 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 This sounds reasonable to me (the person who added canonicalizeModuleMapPath), though I am not at al

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

tahonermann 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 you link to evidence that this

[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 cfe-commits@lists.llvm.

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

jdenny added a comment. In D154130#4502036 , @MrTrillian wrote: > 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

[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 c

[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

jdenny added a comment. 3. Extend lit's own test suite to cover it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 ___ cfe-commits mailing list cfe-commits@lis

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

jdenny added a comment. If the `%>` feature is going to remain (depending on, for example, the answer to @tahonermann's question about modulemap), please: 1. Add it to the lit documentation at https://llvm.org/docs/TestingGuide.html#substitutions and

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

tahonermann added a comment. > 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 you link to evidence that this behavior is by design? It isn't obvious to me why modulemap files would demand different behavior; es

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

ChuanqiXu added a comment. I don't write module map a lot neither. But I am curious where is the definition for the term `%>/t`? It is indeed a little bit odd at the first look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://rev

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

aaron.ballman added subscribers: ChuanqiXu, tahonermann, Bigcheese. aaron.ballman added a comment. In D154130#4481763 , @MrTrillian wrote: > In D154130#4481673 , @aaron.ballman > wrote: > >> Adding a few more fo

[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 > cl

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

aaron.ballman added reviewers: jdenny, jhenderson. aaron.ballman added a comment. 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 clear to me when I'd need to use `%>t` in a

[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 Mono

[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: 1

[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: c