This is an archive of the discontinued LLVM Phabricator instance.

[lit] Keep original cfg file case around.
ClosedPublic

Authored by thakis on Apr 14 2020, 4:53 PM.

Details

Summary

There's been some back and forth if the cfg paths in the
config_map should be normcase()d. The argument for is that
it allows using all-lower spelling in cmd on Windows, the
argument against that doing so is lossy. (See r313918.)

Before the relative-paths-in-generated-lit.site.cfg.py work,
there was no downside to calling normcase(), but with it
we need a hack to recover the original case.

This time, normcase() the hashtable key, but store the original
cased key in addition to the value. This fixes both cons, at the
cost of a few bytes more memory.

Diff Detail

Event Timeline

thakis created this revision.Apr 14 2020, 4:53 PM
thakis marked 2 inline comments as done.Apr 14 2020, 4:54 PM
thakis added inline comments.
llvm/utils/lit/lit/formats/googletest.py
15

This is somewhat unrelated; I believe this call is unneeded after https://reviews.llvm.org/D34855

llvm/utils/lit/lit/util.py
19

This is also unrelated; this function seems to be dead.

hans accepted this revision.Apr 15 2020, 6:30 AM

Seems fine to me. Maybe try landing the unrelated changes separately to minimize things.

This revision is now accepted and ready to land.Apr 15 2020, 6:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 11:32 AM
Meinersbur added inline comments.
llvm/utils/lit/lit/discovery.py
67

Although it turned out to be unrelated to this patch, I leave this comment here:

check-clang started to fail with these kind of error messages:

FAIL: Clang :: SemaObjC/selector-4.m (1 of 1)
******************** TEST 'Clang :: SemaObjC/selector-4.m' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\users\meinersbur\build\llvm-project\release\bin\clang.exe -cc1 -internal-isystem c:\users\meinersbur\build\llvm-project\release\lib\clang\11.0.0\include -nostdsysteminc -Wselector -x objective-c C:\users\meinersbur\src\llvm-project\clang\test\SemaObjC\selector-4.m -include C:\users\meinersbur\src\llvm-project\clang\test\SemaObjC\selector-4.m -verify
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\users\meinersbur\build\llvm-project\release\bin\clang.exe" "-cc1" "-internal-isystem" "c:\users\meinersbur\build\llvm-project\release\lib\clang\11.0.0\include" "-nostdsysteminc" "-Wselector" "-x" "objective-c" "C:\users\meinersbur\src\llvm-project\clang\test\SemaObjC\selector-4.m" "-include" "C:\users\meinersbur\src\llvm-project\clang\test\SemaObjC\selector-4.m" "-verify"
# command stderr:
error: 'warning' diagnostics seen but not expected:
  Line 1: non-portable path to file '"C\\Users\meinersbur\src\llvm-project\clang\test\SemaObjC\selector-4.m"'; specified path differs in case from file name on disk
1 error generated.

error: command failed with exit status: 1

The C\\ resulted from some printing problem, but what cause the issue was -include C:\users\.... with lower-case 'U' while the folder is upper case in the file system. It is the expansion of %s by lit.

Why would lit pass the wrong case? I suspected this patch.

The lower-case "user" originates from __file__ in Clang's lit.cfg.py assigned to config.test_source_root, which is used by Test.getSourcePath() to use as the substitution of %s. No idea why __file__ would not use the proper file's case, bit this call of os.path.realpath might have been intended to return the correct case.

Except, it doesn't. At least in the Python 3.7 that ships with the current Visual Studio. However, Python 3.8 changes the case from user to User. So my solution is to use Python 3.8 from now on, but I have no idea why it worked before or why nobody else seem to have the same problem. I leave this here in case might be useful to someone else.