[PATCH] D103014: [lit] Attempt for fix tests failing because of 'warning: non-portable path to file'

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added a comment.

In D103014#2799086 <https://reviews.llvm.org/D103014#2799086>, @thakis wrote:

> In D103014#2799049 <https://reviews.llvm.org/D103014#2799049>, @krisb wrote:
>
>> In D103014#2792673 <https://reviews.llvm.org/D103014#2792673>, @Meinersbur wrote:
>>
>>> Besides the switch from os.path to pathlib, is the change to first build the path, then to resolve it (instead of first using abspath to resolve it, then concat the path) intentional?
>>
>> Oh, surely no. I'll add back resolving first. Thank you for pointing to this!
>>
>>> Could this also be fixed by making discovery.py consistently use the the map value instead its normcase-d key when building paths?
>>
>> We can do smth like
>>
>>   --- a/llvm/utils/lit/lit/discovery.py
>>   +++ b/llvm/utils/lit/lit/discovery.py
>>   @@ -53,8 +53,7 @@ def getTestSuite(item, litConfig, cache):
>>            config_map = litConfig.params.get('config_map')
>>            if config_map:
>>                cfgpath = os.path.realpath(cfgpath)
>>   -            cfgpath = os.path.normcase(cfgpath)
>>   -            target = config_map.get(cfgpath)
>>   +            target = config_map.get(os.path.normcase(cfgpath))
>>                if target:
>>                    cfgpath = target
>>
>> for discovery.py so that site config will be loaded in the 'original' case (and this seems to be right things to do), but it will not fully fix the problem.
>
> I had tried that, but it didn't work: http://reviews.llvm.org/rGb9fd375d75d4bbf34453696127854d0192e3ccf6

Yeah, I've seen this patch, but it doesn't seem like the case now.

> When the PWD has a lower-case drive letter, like after running cd c:\ with a lower-case "c:" (cmd's default is upper-case, but it takes case-ness from what's passed to cd apparently).

This should be a 'drive letter' specific problem. Since you've already made Wnonportable-include-path tolerant to drive letter case, it seems no longer a problem.

> When manually passing an absolute path to llvm-lit with a lower-case drive letter: python bin\llvm-lit.py -sv c:\llvm-project\clang\test\PCH

I guess the issue here is in `assert t is None or t == cfgpath` statement as `t == cfgpath` assumption seems not fully correct. `cfgpath` in this case is a user-specified absolute path, so it may or may not be the same as one that was constructed by `os.path.abspath()` and added to the map.
If to comment the assert out everything else would work as expected.

On the other side, `os.path.abspath()` for relative paths bases on `cwd` and its case, and may return different results for the same file/directory, depending on the case of `cwd`. `os.path.realpath()` doesn't seem to work consistently as well, especially for msys/mingw setups. But `Path.resolve()` always returns the same on-disk path for the specified file or directory (at least in my experiments, but it would be really great if somebody could try this patch and tell if they see some issues with it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103014