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

Sat Jul 8 06:13:08 PDT 2023

aaron.ballman added subscribers: ChuanqiXu, tahonermann, Bigcheese.
aaron.ballman added a comment.

In D154130#4481763 <https://reviews.llvm.org/D154130#4481763>, @MrTrillian wrote:

> In D154130#4481673 <https://reviews.llvm.org/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 clear to me when I'd need to use `%>t` in a test. I worry code reviewers will miss this sort of thing and we'll only find out there's an issue when the test fails for someone with a problematic path. Is there a rule of thumb we should be following for its use?
>
> Thanks for the extra reviewers!
>
> 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. In fact I should rename `PREFIX_EXPANDED` to `MODULEMAP_PREFIX` so it would be much clearer.

Okay, if this is mostly specific to module maps, that may resolve most of my concern (we don't add a lot of new tests there, so it's less of a burden for reviewers). CC @ChuanqiXu @Bigcheese @tahonermann to see if there are modules concerns with this.

> There are three cases where I didn't expect to need the expanded paths: `relative_include.m`, `case-insensitive-include-win.c` and `module-header-mismatches.m`. There may be a way to change the clang implementation so it does not need to have expanded paths, but that felt like a different investigation.
>
> I'm happy to consider alternative syntaxes to `%>t` too.

I think the syntax is reasonable enough (at least, I don't have arguably better suggestions), it was more just the "how will I know when to use it?" concerns.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154130/new/

https://reviews.llvm.org/D154130