From 8e38e7949dfa275e633530a6bfc7ac8a17e264e0 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Tue, 1 Aug 2023 07:23:42 -0700 Subject: [PATCH 1/3] [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations Running lit tests on Windows can fail because its use of `os.path.realpath` expands substitute drives, which are used to keep paths short and avoid hitting MAX_PATH limitations. Changes lit logic to: Use `os.path.abspath` on Windows, where `MAX_PATH` is a concern that we can work around using substitute drives, which `os.path.realpath` would resolve. Use `os.path.realpath` on Unix, where the current directory always has symlinks resolved, so it is impossible to preserve symlinks in the presence of relative paths, and so we must make sure that all code paths use real paths. Also updates clang's `FileManager::getCanonicalName` and `ExtractAPI` code to avoid resolving substitute drives (i.e. resolving to a path under a different root). How tested: built with `-DLLVM_ENABLE_PROJECTS=clang` and built `check-all` on both Windows Differential Revision: https://reviews.llvm.org/D154130 Reviewed By: @benlangmuir Patch by Tristan Labelle ! --- clang/include/clang/Basic/FileManager.h | 7 ++ clang/lib/Basic/FileManager.cpp | 56 ++++++---- clang/lib/ExtractAPI/ExtractAPIConsumer.cpp | 4 +- .../test/Lexer/case-insensitive-include-win.c | 8 +- llvm/cmake/modules/AddLLVM.cmake | 9 +- llvm/docs/CommandGuide/lit.rst | 10 ++ llvm/docs/TestingGuide.rst | 14 ++- llvm/utils/lit/lit/TestRunner.py | 103 +++++++++--------- llvm/utils/lit/lit/builtin_commands/diff.py | 2 +- llvm/utils/lit/lit/discovery.py | 12 +- llvm/utils/lit/lit/util.py | 17 +++ .../Inputs/config-map-discovery/driver.py | 4 +- .../Inputs/config-map-discovery/lit.alt.cfg | 2 +- .../Inputs/use-llvm-tool-required/lit.cfg | 5 +- .../lit/tests/Inputs/use-llvm-tool/lit.cfg | 5 +- llvm/utils/llvm-lit/llvm-lit.in | 2 +- 16 files changed, 166 insertions(+), 94 deletions(-) diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h index 8eca0fd64f0b5..b192dc9fb37b9 100644 --- a/clang/include/clang/Basic/FileManager.h +++ b/clang/include/clang/Basic/FileManager.h @@ -339,6 +339,13 @@ class FileManager : public RefCountedBase { /// required, which is (almost) never. StringRef getCanonicalName(const FileEntry *File); +private: + /// Retrieve the canonical name for a given file or directory. + /// + /// The first param is a key in the CanonicalNames array. + StringRef getCanonicalName(const void *Entry, StringRef Name); + +public: void PrintStats() const; }; diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp index f9b2afffa5913..1c32aab2fb562 100644 --- a/clang/lib/Basic/FileManager.cpp +++ b/clang/lib/Basic/FileManager.cpp @@ -651,34 +651,48 @@ void FileManager::GetUniqueIDMapping( } StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) { - llvm::DenseMap::iterator Known - = CanonicalNames.find(Dir); - if (Known != CanonicalNames.end()) - return Known->second; - - StringRef CanonicalName(Dir->getName()); - - SmallString<4096> CanonicalNameBuf; - if (!FS->getRealPath(Dir->getName(), CanonicalNameBuf)) - CanonicalName = CanonicalNameBuf.str().copy(CanonicalNameStorage); - - CanonicalNames.insert({Dir, CanonicalName}); - return CanonicalName; + return getCanonicalName(Dir, Dir->getName()); } StringRef FileManager::getCanonicalName(const FileEntry *File) { - llvm::DenseMap::iterator Known - = CanonicalNames.find(File); + return getCanonicalName(File, File->getName()); +} + +StringRef FileManager::getCanonicalName(const void *Entry, StringRef Name) { + llvm::DenseMap::iterator Known = + CanonicalNames.find(Entry); if (Known != CanonicalNames.end()) return Known->second; - StringRef CanonicalName(File->getName()); - - SmallString<4096> CanonicalNameBuf; - if (!FS->getRealPath(File->getName(), CanonicalNameBuf)) - CanonicalName = CanonicalNameBuf.str().copy(CanonicalNameStorage); + // Name comes from FileEntry/DirectoryEntry::getName(), so it is safe to + // store it in the DenseMap below. + StringRef CanonicalName(Name); + + SmallString<256> AbsPathBuf; + SmallString<256> RealPathBuf; + if (!FS->getRealPath(Name, RealPathBuf)) { + if (is_style_windows(llvm::sys::path::Style::native)) { + // For Windows paths, only use the real path if it doesn't resolve + // a substitute drive, as those are used to avoid MAX_PATH issues. + AbsPathBuf = Name; + if (!FS->makeAbsolute(AbsPathBuf)) { + if (llvm::sys::path::root_name(RealPathBuf) == + llvm::sys::path::root_name(AbsPathBuf)) { + CanonicalName = RealPathBuf.str().copy(CanonicalNameStorage); + } else { + // Fallback to using the absolute path. + // Simplifying /../ is semantically valid on Windows even in the + // presence of symbolic links. + llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true); + CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage); + } + } + } else { + CanonicalName = RealPathBuf.str().copy(CanonicalNameStorage); + } + } - CanonicalNames.insert({File, CanonicalName}); + CanonicalNames.insert({Entry, CanonicalName}); return CanonicalName; } diff --git a/clang/lib/ExtractAPI/ExtractAPIConsumer.cpp b/clang/lib/ExtractAPI/ExtractAPIConsumer.cpp index 5a490080f38c6..35748ecedd4b0 100644 --- a/clang/lib/ExtractAPI/ExtractAPIConsumer.cpp +++ b/clang/lib/ExtractAPI/ExtractAPIConsumer.cpp @@ -184,9 +184,7 @@ struct LocationFileChecker { if (ExternalFileEntries.count(File)) return false; - StringRef FileName = File->tryGetRealPathName().empty() - ? File->getName() - : File->tryGetRealPathName(); + StringRef FileName = SM.getFileManager().getCanonicalName(File); // Try to reduce the include name the same way we tried to include it. bool IsQuoted = false; diff --git a/clang/test/Lexer/case-insensitive-include-win.c b/clang/test/Lexer/case-insensitive-include-win.c index 6a17d0b552448..ed722feb3d994 100644 --- a/clang/test/Lexer/case-insensitive-include-win.c +++ b/clang/test/Lexer/case-insensitive-include-win.c @@ -2,9 +2,15 @@ // This file should only include code that really needs a Windows host OS to // run. +// Note: We must use the real path here, because the logic to detect case +// mismatches must resolve the real path to figure out the original casing. +// If we use %t and we are on a substitute drive S: mapping to C:\subst, +// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h" +// and avoid emitting the diagnostic because the structure is different. + // REQUIRES: system-windows // RUN: mkdir -p %t.dir // RUN: touch %t.dir/foo.h -// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s +// RUN: not %clang_cl /FI \\?\%{t:real}.dir\FOO.h /WX -fsyntax-only %s 2>&1 | FileCheck %s // CHECK: non-portable path to file '"\\?\ diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake index a8a52a45e4973..b3cee26cab051 100644 --- a/llvm/cmake/modules/AddLLVM.cmake +++ b/llvm/cmake/modules/AddLLVM.cmake @@ -1688,10 +1688,15 @@ endfunction() # use it and can't be in a lit module. Use with make_paths_relative(). string(CONCAT LLVM_LIT_PATH_FUNCTION "# Allow generated file to be relocatable.\n" - "from pathlib import Path\n" + "import os\n" + "import platform\n" "def path(p):\n" " if not p: return ''\n" - " return str((Path(__file__).parent / p).resolve())\n" + " # Follows lit.util.abs_path_preserve_drive, which cannot be imported here.\n" + " if platform.system() == 'Windows':\n" + " return os.path.abspath(os.path.join(os.path.dirname(__file__), p))\n" + " else:\n" + " return os.path.realpath(os.path.join(os.path.dirname(__file__), p))\n" ) # This function provides an automatic way to 'configure'-like generate a file diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst index e9bd46e84769a..580ba9e33b7ca 100644 --- a/llvm/docs/CommandGuide/lit.rst +++ b/llvm/docs/CommandGuide/lit.rst @@ -541,6 +541,16 @@ TestRunner.py: %/p %p but ``\`` is replaced by ``/`` %/t %t but ``\`` is replaced by ``/`` %/T %T but ``\`` is replaced by ``/`` + %{s:real} %s after expanding all symbolic links and substitute drives + %{S:real} %S after expanding all symbolic links and substitute drives + %{p:real} %p after expanding all symbolic links and substitute drives + %{t:real} %t after expanding all symbolic links and substitute drives + %{T:real} %T after expanding all symbolic links and substitute drives + %{/s:real} %/s after expanding all symbolic links and substitute drives + %{/S:real} %/S after expanding all symbolic links and substitute drives + %{/p:real} %/p after expanding all symbolic links and substitute drives + %{/t:real} %/t after expanding all symbolic links and substitute drives + %{/T:real} %/T after expanding all symbolic links and substitute drives %{/s:regex_replacement} %/s but escaped for use in the replacement of a ``s@@@`` command in sed %{/S:regex_replacement} %/S but escaped for use in the replacement of a ``s@@@`` command in sed %{/p:regex_replacement} %/p but escaped for use in the replacement of a ``s@@@`` command in sed diff --git a/llvm/docs/TestingGuide.rst b/llvm/docs/TestingGuide.rst index cd18cb4a4a5a1..b62dc280074f4 100644 --- a/llvm/docs/TestingGuide.rst +++ b/llvm/docs/TestingGuide.rst @@ -581,7 +581,7 @@ RUN lines: ``${fs-sep}`` Expands to the file system separator, i.e. ``/`` or ``\`` on Windows. -``%/s, %/S, %/t, %/T:`` +``%/s, %/S, %/t, %/T`` Act like the corresponding substitution above but replace any ``\`` character with a ``/``. This is useful to normalize path separators. @@ -590,7 +590,17 @@ RUN lines: Example: ``%/s: C:/Desktop Files/foo_test.s.tmp`` -``%:s, %:S, %:t, %:T:`` +``%{s:real}, %{S:real}, %{t:real}, %{T:real}`` +``%{/s:real}, %{/S:real}, %{/t:real}, %{/T:real}`` + + Act like the corresponding substitution, including with ``/``, but use + the real path by expanding all symbolic links and substitute drives. + + Example: ``%s: S:\foo_test.s.tmp`` + + Example: ``%{/s:real}: C:/SDrive/foo_test.s.tmp`` + +``%:s, %:S, %:t, %:T`` Act like the corresponding substitution above but remove colons at the beginning of Windows paths. This is useful to allow concatenation diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py index 664235beaae6b..eb595e5194825 100644 --- a/llvm/utils/lit/lit/TestRunner.py +++ b/llvm/utils/lit/lit/TestRunner.py @@ -70,7 +70,7 @@ def change_dir(self, newdir): if os.path.isabs(newdir): self.cwd = newdir else: - self.cwd = os.path.realpath(os.path.join(self.cwd, newdir)) + self.cwd = lit.util.abs_path_preserve_drive(os.path.join(self.cwd, newdir)) class TimeoutHelper(object): """ @@ -401,7 +401,7 @@ def executeBuiltinMkdir(cmd, cmd_shenv): dir = to_unicode(dir) if kIsWindows else to_bytes(dir) cwd = to_unicode(cwd) if kIsWindows else to_bytes(cwd) if not os.path.isabs(dir): - dir = os.path.realpath(os.path.join(cwd, dir)) + dir = lit.util.abs_path_preserve_drive(os.path.join(cwd, dir)) if parent: lit.util.mkdir_p(dir) else: @@ -446,7 +446,7 @@ def on_rm_error(func, path, exc_info): path = to_unicode(path) if kIsWindows else to_bytes(path) cwd = to_unicode(cwd) if kIsWindows else to_bytes(cwd) if not os.path.isabs(path): - path = os.path.realpath(os.path.join(cwd, path)) + path = lit.util.abs_path_preserve_drive(os.path.join(cwd, path)) if force and not os.path.exists(path): continue try: @@ -1153,58 +1153,59 @@ def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False): substitutions.extend(test.config.substitutions) tmpName = tmpBase + '.tmp' baseName = os.path.basename(tmpBase) - substitutions.extend([('%s', sourcepath), - ('%S', sourcedir), - ('%p', sourcedir), - ('%{pathsep}', os.pathsep), - ('%t', tmpName), - ('%basename_t', baseName), - ('%T', tmpDir)]) - - substitutions.extend([ - ('%{fs-src-root}', pathlib.Path(sourcedir).anchor), - ('%{fs-tmp-root}', pathlib.Path(tmpBase).anchor), - ('%{fs-sep}', os.path.sep), - ]) - - # "%/[STpst]" should be normalized. - substitutions.extend([ - ('%/s', sourcepath.replace('\\', '/')), - ('%/S', sourcedir.replace('\\', '/')), - ('%/p', sourcedir.replace('\\', '/')), - ('%/t', tmpBase.replace('\\', '/') + '.tmp'), - ('%/T', tmpDir.replace('\\', '/')), - ('%/et',tmpName.replace('\\', '\\\\\\\\\\\\\\\\')), - ]) - - # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're - # also in a regex replacement context of a s@@@ regex. + + substitutions.append(("%{pathsep}", os.pathsep)) + substitutions.append(("%basename_t", baseName)) + + substitutions.extend( + [ + ("%{fs-src-root}", pathlib.Path(sourcedir).anchor), + ("%{fs-tmp-root}", pathlib.Path(tmpBase).anchor), + ("%{fs-sep}", os.path.sep), + ] + ) + + substitutions.append(("%/et", tmpName.replace("\\", "\\\\\\\\\\\\\\\\"))) + def regex_escape(s): s = s.replace('@', r'\@') s = s.replace('&', r'\&') return s - substitutions.extend([ - ('%{/s:regex_replacement}', - regex_escape(sourcepath.replace('\\', '/'))), - ('%{/S:regex_replacement}', - regex_escape(sourcedir.replace('\\', '/'))), - ('%{/p:regex_replacement}', - regex_escape(sourcedir.replace('\\', '/'))), - ('%{/t:regex_replacement}', - regex_escape(tmpBase.replace('\\', '/')) + '.tmp'), - ('%{/T:regex_replacement}', - regex_escape(tmpDir.replace('\\', '/'))), - ]) - - # "%:[STpst]" are normalized paths without colons and without a leading - # slash. - substitutions.extend([ - ('%:s', colonNormalizePath(sourcepath)), - ('%:S', colonNormalizePath(sourcedir)), - ('%:p', colonNormalizePath(sourcedir)), - ('%:t', colonNormalizePath(tmpBase + '.tmp')), - ('%:T', colonNormalizePath(tmpDir)), - ]) + + path_substitutions = [ + ("s", sourcepath), ("S", sourcedir), ("p", sourcedir), + ("t", tmpName), ("T", tmpDir) + ] + for path_substitution in path_substitutions: + letter = path_substitution[0] + path = path_substitution[1] + + # Original path variant + substitutions.append(("%" + letter, path)) + + # Normalized path separator variant + substitutions.append(("%/" + letter, path.replace("\\", "/"))) + + # realpath variants + # Windows paths with substitute drives are not expanded by default + # as they are used to avoid MAX_PATH issues, but sometimes we do + # need the fully expanded path. + real_path = os.path.realpath(path) + substitutions.append(("%{" + letter + ":real}", real_path)) + substitutions.append(("%{/" + letter + ":real}", + real_path.replace("\\", "/"))) + + # "%{/[STpst]:regex_replacement}" should be normalized like + # "%/[STpst]" but we're also in a regex replacement context + # of a s@@@ regex. + substitutions.append( + ("%{/" + letter + ":regex_replacement}", + regex_escape(path.replace("\\", "/")))) + + # "%:[STpst]" are normalized paths without colons and without + # a leading slash. + substitutions.append(("%:" + letter, colonNormalizePath(path))) + return substitutions def _memoize(f): diff --git a/llvm/utils/lit/lit/builtin_commands/diff.py b/llvm/utils/lit/lit/builtin_commands/diff.py index a12e5e100b1bd..7233d81e72dc3 100644 --- a/llvm/utils/lit/lit/builtin_commands/diff.py +++ b/llvm/utils/lit/lit/builtin_commands/diff.py @@ -229,7 +229,7 @@ def main(argv): try: for file in args: if file != "-" and not os.path.isabs(file): - file = os.path.realpath(os.path.join(os.getcwd(), file)) + file = util.abs_path_preserve_drive(file) if flags.recursive_diff: if file == "-": diff --git a/llvm/utils/lit/lit/discovery.py b/llvm/utils/lit/lit/discovery.py index bb77aa310c713..06d058a0afdd4 100644 --- a/llvm/utils/lit/lit/discovery.py +++ b/llvm/utils/lit/lit/discovery.py @@ -7,7 +7,7 @@ import sys from lit.TestingConfig import TestingConfig -from lit import LitConfig, Test +from lit import LitConfig, Test, util def chooseConfigFileFromDir(dir, config_names): for name in config_names: @@ -52,8 +52,8 @@ def search1(path): # configuration to load instead. config_map = litConfig.params.get('config_map') if config_map: - cfgpath = os.path.realpath(cfgpath) - target = config_map.get(os.path.normcase(cfgpath)) + cfgpath = util.abs_path_preserve_drive(cfgpath) + target = config_map.get(cfgpath) if target: cfgpath = target @@ -63,13 +63,13 @@ def search1(path): cfg = TestingConfig.fromdefaults(litConfig) cfg.load_from_path(cfgpath, litConfig) - source_root = os.path.realpath(cfg.test_source_root or path) - exec_root = os.path.realpath(cfg.test_exec_root or path) + source_root = util.abs_path_preserve_drive(cfg.test_source_root or path) + exec_root = util.abs_path_preserve_drive(cfg.test_exec_root or path) return Test.TestSuite(cfg.name, source_root, exec_root, cfg), () def search(path): # Check for an already instantiated test suite. - real_path = os.path.realpath(path) + real_path = util.abs_path_preserve_drive(path) res = cache.get(real_path) if res is None: cache[real_path] = res = search1(path) diff --git a/llvm/utils/lit/lit/util.py b/llvm/utils/lit/lit/util.py index 75cface848390..435be397716f5 100644 --- a/llvm/utils/lit/lit/util.py +++ b/llvm/utils/lit/lit/util.py @@ -127,6 +127,23 @@ def usable_core_count(): return n +def abs_path_preserve_drive(path): + """Return the absolute path without resolving drive mappings on Windows. + + """ + if platform.system() == "Windows": + # Windows has limitations on path length (MAX_PATH) that + # can be worked around using substitute drives, which map + # a drive letter to a longer path on another drive. + # Since Python 3.8, os.path.realpath resolves sustitute drives, + # so we should not use it. In Python 3.7, os.path.realpath + # was implemented as os.path.abspath. + return os.path.normpath(os.path.abspath(path)) + else: + # On UNIX, the current directory always has symbolic links resolved, + # so any program accepting relative paths cannot preserve symbolic + # links in paths and we should always use os.path.realpath. + return os.path.normpath(os.path.realpath(path)) def mkdir(path): try: diff --git a/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py b/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py index db9141b9b1bf9..6f22a7f869620 100644 --- a/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py +++ b/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py @@ -2,9 +2,7 @@ import os import sys -main_config = sys.argv[1] -main_config = os.path.realpath(main_config) -main_config = os.path.normcase(main_config) +main_config = lit.util.abs_path_preserve_drive(sys.argv[1]) config_map = {main_config : sys.argv[2]} builtin_parameters = {'config_map' : config_map} diff --git a/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg b/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg index c7b303f50a05c..27860e192fdc1 100644 --- a/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg +++ b/llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg @@ -5,5 +5,5 @@ config.suffixes = ['.txt'] config.test_format = lit.formats.ShTest() import os -config.test_exec_root = os.path.realpath(os.path.dirname(__file__)) +config.test_exec_root = os.path.dirname(lit.util.abs_path_preserve_drive(__file__)) config.test_source_root = os.path.join(config.test_exec_root, "tests") diff --git a/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg b/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg index b1c510e05ab74..f5101dbf2b052 100644 --- a/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg +++ b/llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg @@ -1,11 +1,14 @@ import lit.formats +import lit.util + config.name = 'use-llvm-tool-required' config.suffixes = ['.txt'] config.test_format = lit.formats.ShTest() config.test_source_root = None config.test_exec_root = None import os.path -config.llvm_tools_dir = os.path.realpath(os.path.dirname(__file__)) + +config.llvm_tools_dir = os.path.dirname(lit.util.abs_path_preserve_drive(__file__)) import lit.llvm lit.llvm.initialize(lit_config, config) lit.llvm.llvm_config.use_llvm_tool('found', required=True) diff --git a/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg b/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg index 2bd401aacde35..f248ec596c78c 100644 --- a/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg +++ b/llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg @@ -1,11 +1,14 @@ import lit.formats +import lit.util + config.name = 'use-llvm-tool' config.suffixes = ['.txt'] config.test_format = lit.formats.ShTest() config.test_source_root = None config.test_exec_root = None import os.path -this_dir = os.path.realpath(os.path.dirname(__file__)) + +this_dir = os.path.dirname(lit.util.abs_path_preserve_drive(__file__)) config.llvm_tools_dir = os.path.join(this_dir, 'build') import lit.llvm lit.llvm.initialize(lit_config, config) diff --git a/llvm/utils/llvm-lit/llvm-lit.in b/llvm/utils/llvm-lit/llvm-lit.in index 33ec8017cf05f..0b6683b6b6230 100755 --- a/llvm/utils/llvm-lit/llvm-lit.in +++ b/llvm/utils/llvm-lit/llvm-lit.in @@ -8,7 +8,7 @@ config_map = {} def map_config(source_dir, site_config): global config_map - source_dir = os.path.realpath(source_dir) + source_dir = os.path.abspath(source_dir) source_dir = os.path.normcase(source_dir) site_config = os.path.normpath(site_config) config_map[source_dir] = site_config From 41634302fa28b1af81e60b4974bf10d97b058ae7 Mon Sep 17 00:00:00 2001 From: Simon Pilgrim Date: Wed, 2 Aug 2023 15:37:36 +0100 Subject: [PATCH 2/3] [lit] Add missing os.path.normcase() to config-map-discovery test 5ccfa1568130 removed normalization from abs_path_preserve_drive but I missed adding this case back to the caller --- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py | 1 + 1 file changed, 1 insertion(+) diff --git a/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py b/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py index 6f22a7f869620..19a74faeca73e 100644 --- a/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py +++ b/llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py @@ -3,6 +3,7 @@ import sys main_config = lit.util.abs_path_preserve_drive(sys.argv[1]) +main_config = os.path.normcase(main_config) config_map = {main_config : sys.argv[2]} builtin_parameters = {'config_map' : config_map} From fda6d9e4fb710f0a4954cce507bba5577188f1ea Mon Sep 17 00:00:00 2001 From: Simon Pilgrim Date: Wed, 2 Aug 2023 13:43:05 +0100 Subject: [PATCH 3/3] [lit] abs_path_preserve_drive - don't normalize path, leave this to the caller As noted on D154130, this was preventing path matching between normalized/unnormalized paths on some windows builds. --- llvm/utils/lit/lit/discovery.py | 2 +- llvm/utils/lit/lit/util.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/utils/lit/lit/discovery.py b/llvm/utils/lit/lit/discovery.py index 06d058a0afdd4..254b8ae5050ad 100644 --- a/llvm/utils/lit/lit/discovery.py +++ b/llvm/utils/lit/lit/discovery.py @@ -53,7 +53,7 @@ def search1(path): config_map = litConfig.params.get('config_map') if config_map: cfgpath = util.abs_path_preserve_drive(cfgpath) - target = config_map.get(cfgpath) + target = config_map.get(os.path.normcase(cfgpath)) if target: cfgpath = target diff --git a/llvm/utils/lit/lit/util.py b/llvm/utils/lit/lit/util.py index 435be397716f5..4d3802db31fab 100644 --- a/llvm/utils/lit/lit/util.py +++ b/llvm/utils/lit/lit/util.py @@ -138,12 +138,12 @@ def abs_path_preserve_drive(path): # Since Python 3.8, os.path.realpath resolves sustitute drives, # so we should not use it. In Python 3.7, os.path.realpath # was implemented as os.path.abspath. - return os.path.normpath(os.path.abspath(path)) + return os.path.abspath(path) else: # On UNIX, the current directory always has symbolic links resolved, # so any program accepting relative paths cannot preserve symbolic # links in paths and we should always use os.path.realpath. - return os.path.normpath(os.path.realpath(path)) + return os.path.realpath(path) def mkdir(path): try: