This adds 2 new lit helpers %{fsroot} and %{fssep}, these allow
writing tests that correctly handle slashes on Windows. In the case of
tests like clang/test/CodeGen/debug-prefix-map.c, these are unable to
correctly test behavior on both platforms, unless they fork and add OS
requirements, because the relevant logic hits host specific codepaths
like checking if paths are absolute.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/test/lit.cfg.py | ||
|---|---|---|
| 60 | c:\? | |
| clang/test/lit.cfg.py | ||
|---|---|---|
| 60 | Do you mean lowercase or a single slash? I see ~2x the number of uppercase vs lowercase in this codebase now, and the second is just because of escaping | |
| clang/test/lit.cfg.py | ||
|---|---|---|
| 60 | I meant the single slash, yes, wasn't sure if it needed to be escaped or not | |
Comment Actions
Allow empty leading on windows
This behavior is currently broken, the directory should never be / or C:\ but the logic is broken and only handles /
Comment Actions
Wouldn't this one also be solved pretty much the same, but differently, by changing if (llvm::sys::path::is_absolute(RemappedFile)) { into is_absolute_gnu? Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)
(Overall I think it might be good to have the native separators and root available for tests though, even if this particular test might make sense both as-is and with native separators.)
Comment Actions
I think it could but also users could still remap to native window's paths, so likely we'd want this test as well I think either way?
Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)
Good point here, I could definitely see wanting to support the entire matrix of host windows paths vs not, and target windows paths vs not, but I think that would require significantly more changes for other places that call llvm::sys::path::* APIs and also use the default native argument (I'm not sure how difficult this would be, but it would require a bit of auditing)
Comment Actions
I guess this might be a good addition as a new test, yeah, but I think it would be good to keep this test as is too, and change the code to use is_absolute_gnu (and fix up the test reference here to expect an empty directory in the output).
Since we're remapping debug paths, it's plausible that the target path can be a different style (when cross compiling, where debug prefix remapping is kinda important), and then it's probably good to use a more lax definition of whether a path is absolute. (Alternatively, we maybe should try to detect the kind of path used and use the appropriate style as argument to is_absolute, but I don't think that necessarily helps here.)
Good point here, I could definitely see wanting to support the entire matrix of host windows paths vs not, and target windows paths vs not, but I think that would require significantly more changes for other places that call llvm::sys::path::* APIs and also use the default native argument (I'm not sure how difficult this would be, but it would require a bit of auditing)
I would expect that to mostly work so far, except for corner cases like these. Auditing probably doesn't hurt if one wants to spend the effort, otherwise I'd just expect it to work and try it out and see if one runs into any issues somewhere, if someone has such a usecase.
Comment Actions
Yep this makes sense. I would rather not scope that into this if that's ok. Since this test was already invalid and broken (actual fix in https://reviews.llvm.org/D111579) I would rather land these and then take that on to unblock my original use case (https://reviews.llvm.org/D111587) if I can find some more time
Comment Actions
Ok, fair enough - I guess that sounds reasonable, so that'd be a +1 from me to this patch. Ideally it'd be good to find someone more authoritative around clang and these areas than me, but I guess reviewer attention is scarce and hard to come by. @rnk - does this seem sensible to you?
Let's give it a little more time for someone else to comment on it, otherwise I guess I could approve it...
Then for the future cleanup, one might be able to stop using the native substitutions, by keeping one test entirely using possibly-foreign posix style forward slashes, and one using the windows style paths (with that test possibly limited to only executed when on windows, if necessary - but being able to map things to a windows path while cross compiling is good too).
Comment Actions
These seem like generic substitutions that would be useful to anyone writing lit tests. We already have %{pathsep}:
https://llvm.org/docs/CommandGuide/lit.html#substitutions
These should live there as well, and be documented similarly, I think.
| clang/test/lit.cfg.py | ||
|---|---|---|
| 60 | This isn't really a separator, this is the root itself. I think that it makes sense to name it accordingly. Please do not hard code this to C:\. There is no guarantee that the root is C:\. | |
| clang/test/lit.cfg.py | ||
|---|---|---|
| 60 | Yea I didn't like the original naming here either, any suggestions? I avoid path in these because of the existing pathsep, maybe fsroot and fssep? What are you suggesting in place of hardcoding C:\? I don't think we could detect what folks are using in the tests, so it does seem like it would be up for individual tests to do this however it makes sense. It appears that there are a few hundred uses of this in tests today, does that break in general if you're using another drive? | |
| llvm/docs/TestingGuide.rst | ||
|---|---|---|
| 577 ↗ | (On Diff #411573) | Stray backticks in "Expands"? |
| llvm/utils/lit/lit/TestRunner.py | ||
| 1124 ↗ | (On Diff #411573) | It is pretty common to run the LLVM test suite on secondary drives, especially on buildbot VMs, to separate build disk usage from the root partition. Can we have two separators, ${srcroot} and ${tmproot} that expand to the drive letter of the source and temp path base? That seems like it would be general and implementable. |
| llvm/utils/lit/lit/TestRunner.py | ||
|---|---|---|
| 1124 ↗ | (On Diff #411573) | +1. I don't even do builds on C: drive, on one of my machines. |
| llvm/utils/lit/lit/TestRunner.py | ||
|---|---|---|
| 1124 ↗ | (On Diff #411573) | Updated, I kept the fs prefixing thinking that srcroot and tmproot might be too confusable as an alternative to %t and friends, let me know what you think! |
Comment Actions
For reference here's an example of a test that uses this https://reviews.llvm.org/D111579
Comment Actions
Seems reasonable, though I'm not a fan of the variable names - they seem a bit difficult to read due to no separation (e.g., %fs-src-root or %fs_src_root vs %fssrcroot)
c:\?