[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added inline comments.



Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// 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.

tahonermann wrote:
> MrTrillian wrote:
> > tahonermann wrote:
> > > MrTrillian wrote:
> > > > tahonermann wrote:
> > > > > Hmm, is it really desirable or necessary that the case mismatch logic 
> > > > > resolve substituted drives? I wouldn't think so. This seems like 
> > > > > another case where our common canonicalization would be desired.
> > > > I think it is necessary as I don't see how else we can retrieve the 
> > > > original casing of the file path. Merely making the path absolute would 
> > > > not do that. Searching for solutions on the internet finds ideas that 
> > > > are worse like converting to a short path then back to a long path or 
> > > > rebuilding the path one component at a time with a series directory 
> > > > listing requests.
> > > The warning this test checks for is diagnosed in 
> > > `Preprocessor::HandleHeaderIncludeOrImport()`; search for 
> > > `pp_nonportable_path` and/or `pp_nonportable_system_path`. I believe this 
> > > warning will be issued if any component of the path does not match the 
> > > case of the include directive. Since the file name differs in case, 
> > > unless there is a bug in handling of the file name, this test isn't 
> > > sensitive to case mismatches in `%t`.
> > > 
> > > From a user point of view, resolving a substitute drive doesn't seem 
> > > desirableto me with respect to determining whether a non-portable path is 
> > > being used; I don't think the behavior should be dependent on whether a 
> > > substitute drive is being used or what its target is.
> > @tahonermann I think the code is working by design and it would be an 
> > unrelated change to modify its logic. See `trySimplifyPath` in 
> > `PPDirectives.cpp`:
> > 
> > ```
> >   // Below is a best-effort to handle ".." in paths. It is admittedly
> >   // not 100% correct in the presence of symlinks.
> > 
> > // If these path components differ by more than just case, then we
> > // may be looking at symlinked paths. Bail on this diagnostic to 
> > avoid
> > // noisy false positives.
> > ```
> > 
> > The test was previously implicitly requiring getting the realpath, and it 
> > worked because `lit.py`'s logic of doing that. Now that requirement is 
> > explicit in the test.
> I'm still not following here. Are you saying that `trySimplifyPath()` will 
> replace substitute drives? If so, that doesn't sound desirable and I would 
> expect it to be problematic for your use case. I think we should follow this 
> up ... somewhere (now that this review is closed).
@tahonermann . `trySimplifyPath()` does not replace substitute drives. It's a 
best-effort attempt to see if the included path mismatches the real file path 
only by case. It explicitly bails out without diagnostics if it finds that the 
included path has a different shape from the real file path, which will happen 
if the included path is on a substitute drive. It has to compare with the real 
file path because this is the only reasonable way to get the original path 
casing.

It was already the case that this diagnostic bailed out in the presence of 
symbolic links, so there are no behavioral differences. I needed to update the 
test because previously `lit.py` would enforce that `%t` was a real path, and 
now it doesn't, which means that we would hit the "bail out" code path and not 
produce the diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian marked 4 inline comments as done.
MrTrillian added a comment.

Resolved remaining comments addressed with the FileManager.cpp change to branch 
on the native path format being Windows. This is ready to merge!


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian marked an inline comment as done.
MrTrillian added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

benlangmuir wrote:
> MrTrillian wrote:
> > benlangmuir wrote:
> > > MrTrillian wrote:
> > > > benlangmuir wrote:
> > > > > Removing .. can change where the path points in the presence of 
> > > > > symlinks; is this needed?
> > > > > Removing .. can change where the path points in the presence of 
> > > > > symlinks; is this needed?
> > > > 
> > > > @benlangmuir That's true and not ideal, but `makeAbsolute` will not 
> > > > resolve `/./` or `/../` in paths, so it's not a canonicalization and 
> > > > some tests were failing because of that. One alternative would be to 
> > > > use `makeAbsolute` + `remove_dots` on Windows (where removing dot dots 
> > > > is semantically correct) and `getRealPath` on Unix, like I do in lit. 
> > > > Suggestions?
> > > Wouldn't removing .. have the same issue with symlinks on Windows? I know 
> > > symlinks are less common there, but it's not clear to me why it would be 
> > > correct.  I guess you could also check if the paths resolve to the same 
> > > file after removing ..
> > > 
> > > 
> > @benlangmuir Windows simplifies `\..\` in paths before starting to walk the 
> > filesystem. 
> > https://superuser.com/questions/1502360/different-behavior-of-double-dots-and-symlinks-in-windows-and-unix
> > 
> > If I detected that the path didn't resolve to the same file after removing 
> > `..`, what would I do?
> > 
> > I think the current logic will only end up using the `else` branch on 
> > Windows. At least I can't think of a scenario where the root would change 
> > from using realpath on unix.
> Thanks, I wasn't aware of that subtlety! I suggest we add a comment about 
> that here and also mention (or assert) that this is only reachable on Windows.
> Thanks, I wasn't aware of that subtlety! I suggest we add a comment about 
> that here and also mention (or assert) that this is only reachable on Windows.

@benlangmuir Accounted for in lastest revision. The root-preserving 
canonicalization logic is now Windows-only.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 545250.
MrTrillian marked an inline comment as done.
MrTrillian added a comment.

@benlangmuir I made the root-preserving canonicalization logic Windows-only now 
that I found how to test for that. It changes the code shape a little but I 
think it's an improvement.


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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -127,6 +127,23 @@
 
 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 

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian marked an inline comment as done.
MrTrillian added inline comments.



Comment at: llvm/utils/lit/lit/LitConfig.py:192
 f = f.f_back.f_back
-file = os.path.abspath(inspect.getsourcefile(f))
+file = lit.util.abs_path_preserve_drive(inspect.getsourcefile(f))
 line = inspect.getlineno(f)

benlangmuir wrote:
> Why are you changing abspath (here and elsewhere)? I understand why you are 
> changing realpath -> lit.util.abs_path_preserve_drive, but what's the issue 
> with bare abspath?
> Why are you changing abspath (here and elsewhere)? I understand why you are 
> changing realpath -> lit.util.abs_path_preserve_drive, but what's the issue 
> with bare abspath?

You're right, this is overzealous. I think it made more sense with an older 
iteration of this code. I removed it.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

benlangmuir wrote:
> MrTrillian wrote:
> > benlangmuir wrote:
> > > Removing .. can change where the path points in the presence of symlinks; 
> > > is this needed?
> > > Removing .. can change where the path points in the presence of symlinks; 
> > > is this needed?
> > 
> > @benlangmuir That's true and not ideal, but `makeAbsolute` will not resolve 
> > `/./` or `/../` in paths, so it's not a canonicalization and some tests 
> > were failing because of that. One alternative would be to use 
> > `makeAbsolute` + `remove_dots` on Windows (where removing dot dots is 
> > semantically correct) and `getRealPath` on Unix, like I do in lit. 
> > Suggestions?
> Wouldn't removing .. have the same issue with symlinks on Windows? I know 
> symlinks are less common there, but it's not clear to me why it would be 
> correct.  I guess you could also check if the paths resolve to the same file 
> after removing ..
> 
> 
@benlangmuir Windows simplifies `\..\` in paths before starting to walk the 
filesystem. 
https://superuser.com/questions/1502360/different-behavior-of-double-dots-and-symlinks-in-windows-and-unix

If I detected that the path didn't resolve to the same file after removing 
`..`, what would I do?

I think the current logic will only end up using the `else` branch on Windows. 
At least I can't think of a scenario where the root would change from using 
realpath on unix.



Comment at: llvm/utils/lit/lit/TestRunner.py:1282
+# a leading slash.
+substitutions.append(("%:" + letter, colonNormalizePath(path)))
 

benlangmuir wrote:
> This change drops the `+ ".tmp"`  that was previously added to 
> `%t:regex_replacement` and `%:t`.
> This change drops the `+ ".tmp"`  that was previously added to 
> `%t:regex_replacement` and `%:t`.

@benlangmuir Actually not! The `path_substitutions` list uses `("t", tmpName)` 
which includes the `.tmp`, instead of `tmpBase` which doesn't. Looking at the 
original code, there didn't seem to be a reason to use `tmpBase` and re-append 
`.tmp` instead of using `tmpName`.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 545218.
MrTrillian marked 4 inline comments as done.
MrTrillian added a comment.

Reverted some `os.path.abspath` that had been converted to 
`abs_path_preserve_drive`


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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/lit/util.py
===
--- llvm/utils/lit/lit/util.py
+++ llvm/utils/lit/lit/util.py
@@ -127,6 +127,23 @@
 
 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 

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian marked an inline comment as done.
MrTrillian added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:663
+} else {
+  llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+  CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);

benlangmuir wrote:
> Removing .. can change where the path points in the presence of symlinks; is 
> this needed?
> Removing .. can change where the path points in the presence of symlinks; is 
> this needed?

@benlangmuir That's true and not ideal, but `makeAbsolute` will not resolve 
`/./` or `/../` in paths, so it's not a canonicalization and some tests were 
failing because of that. One alternative would be to use `makeAbsolute` + 
`remove_dots` on Windows (where removing dot dots is semantically correct) and 
`getRealPath` on Unix, like I do in lit. Suggestions?



Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:190
 
-StringRef FileName = File->tryGetRealPathName().empty()
- ? File->getName()
- : File->tryGetRealPathName();
+StringRef FileName = SM.getFileManager().getCanonicalName(File);
 

benlangmuir wrote:
> Why is this change needed?
> Why is this change needed?

@benlangmuir We don't want raw `getRealPath`s on Windows because of substitute 
drives and MAX_PATH issues. That is the idea behind this diff. If I leave the 
`tryGetRealPathName` here, I need to change the `relative_include.m` test as in 
this previous diff: https://reviews.llvm.org/D154130?id=539683 , which is 
undesirable.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 544743.
MrTrillian added a comment.

Reduced path small strings length to 256 (incidentally close to MAX_PATH on 
Windows), per @benlangmuir 's comment


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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.abs_path_preserve_drive(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/setup.py
===
--- llvm/utils/lit/setup.py
+++ llvm/utils/lit/setup.py
@@ -7,7 +7,7 @@
 # is nice to allow:
 #   python path/to/setup.py install
 # to work (for scripts, etc.)
-os.chdir(os.path.dirname(os.path.abspath(__file__)))

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

Ping. Any brave reviewer to help here?


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

Responded to comment.




Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// 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.

tahonermann wrote:
> MrTrillian wrote:
> > tahonermann wrote:
> > > Hmm, is it really desirable or necessary that the case mismatch logic 
> > > resolve substituted drives? I wouldn't think so. This seems like another 
> > > case where our common canonicalization would be desired.
> > I think it is necessary as I don't see how else we can retrieve the 
> > original casing of the file path. Merely making the path absolute would not 
> > do that. Searching for solutions on the internet finds ideas that are worse 
> > like converting to a short path then back to a long path or rebuilding the 
> > path one component at a time with a series directory listing requests.
> The warning this test checks for is diagnosed in 
> `Preprocessor::HandleHeaderIncludeOrImport()`; search for 
> `pp_nonportable_path` and/or `pp_nonportable_system_path`. I believe this 
> warning will be issued if any component of the path does not match the case 
> of the include directive. Since the file name differs in case, unless there 
> is a bug in handling of the file name, this test isn't sensitive to case 
> mismatches in `%t`.
> 
> From a user point of view, resolving a substitute drive doesn't seem 
> desirableto me with respect to determining whether a non-portable path is 
> being used; I don't think the behavior should be dependent on whether a 
> substitute drive is being used or what its target is.
@tahonermann I think the code is working by design and it would be an unrelated 
change to modify its logic. See `trySimplifyPath` in `PPDirectives.cpp`:

```
  // Below is a best-effort to handle ".." in paths. It is admittedly
  // not 100% correct in the presence of symlinks.

// If these path components differ by more than just case, then we
// may be looking at symlinked paths. Bail on this diagnostic to avoid
// noisy false positives.
```

The test was previously implicitly requiring getting the realpath, and it 
worked because `lit.py`'s logic of doing that. Now that requirement is explicit 
in the test.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 543677.
MrTrillian added a comment.

Fixed `clang-format` issues and improved test comment.


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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.abs_path_preserve_drive(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/setup.py
===
--- llvm/utils/lit/setup.py
+++ llvm/utils/lit/setup.py
@@ -7,7 +7,7 @@
 # is nice to allow:
 #   python path/to/setup.py install
 # to work (for scripts, etc.)
-os.chdir(os.path.dirname(os.path.abspath(__file__)))
+os.chdir(os.path.dirname(lit.util.abs_path_preserve_drive(__file__)))
 

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian marked an inline comment as done.
MrTrillian added a comment.

Marking comments as resolved per my reply. I'm not sure if that's best 
practice! @tahonermann

Looking forward to reviews


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian marked an inline comment as done.
MrTrillian added inline comments.



Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// 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.

tahonermann wrote:
> Hmm, is it really desirable or necessary that the case mismatch logic resolve 
> substituted drives? I wouldn't think so. This seems like another case where 
> our common canonicalization would be desired.
I think it is necessary as I don't see how else we can retrieve the original 
casing of the file path. Merely making the path absolute would not do that. 
Searching for solutions on the internet finds ideas that are worse like 
converting to a short path then back to a long path or rebuilding the path one 
component at a time with a series directory listing requests.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

In D154130#4516226 , @tahonermann 
wrote:

> It is unclear to me why/when we would ever want the substitute drive 
> expansion; the modified tests aren't very elucidating. My naive expectation 
> is that we effectively want to restore the Python 3.7 behavior.

It took some more investigation to figure out why I still needed realpaths 
there. In the end:

- `case-insensitive-include.win.c`: still needs it, added a comment.
- `relative_module.m`: required a fix in `ExtractAPIConsumer.cpp` to avoid 
calling `getRealPath`.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 542922.
MrTrillian retitled this revision from "[lit] Avoid os.path.realpath on Windows 
due to MAX_PATH limitations" to "[lit][clang] Avoid realpath on Windows due to 
MAX_PATH limitations".
MrTrillian edited the summary of this revision.

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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.abs_path_preserve_drive(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/setup.py
===
--- llvm/utils/lit/setup.py
+++ llvm/utils/lit/setup.py
@@ -7,7 +7,7 @@
 # is nice to allow:
 #   python path/to/setup.py install
 # to work (for 

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 542097.
MrTrillian added a comment.

Undo whitespace changes.


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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/test/ExtractAPI/relative_include.m
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.abs_path_preserve_drive(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/setup.py
===
--- llvm/utils/lit/setup.py
+++ llvm/utils/lit/setup.py
@@ -7,7 +7,7 @@
 # is nice to allow:
 #   python path/to/setup.py install
 # to work (for scripts, etc.)
-os.chdir(os.path.dirname(os.path.abspath(__file__)))
+os.chdir(os.path.dirname(lit.util.abs_path_preserve_drive(__file__)))
 sys.path.insert(0, ".")
 
 import lit
Index: 

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 542094.
MrTrillian edited the summary of this revision.
MrTrillian added a comment.

Update clang path canonicalization to avoid resolving substitute drives (ie 
resolving to a path under a different root). This requires much less change to 
tests.


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

https://reviews.llvm.org/D154130

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/test/ExtractAPI/relative_include.m
  clang/test/Lexer/case-insensitive-include-win.c
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.abs_path_preserve_drive(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
+++ llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
@@ -5,5 +5,5 @@
 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")
Index: llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
===
--- llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
+++ 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}
Index: llvm/utils/lit/setup.py
===
--- llvm/utils/lit/setup.py
+++ llvm/utils/lit/setup.py
@@ -7,7 +7,7 @@
 # is nice to allow:
 #   python path/to/setup.py install
 # to work (for 

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

> That could impose a significant performance and drive space penalty in the 
> event that substitute drive paths are changed, but I would expect such 
> changes to be rare.

I think the more likely case is that one would build their repo under the 
substitute drive and later build it under the expanded path on the original 
drive. Though I feel like a lot of caching might then break.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

> I took a look at the code and it looks to me like it would be safe to change 
> ModuleMap::canonicalizeModuleMapPath() to use a drive preserving 
> canonicalization

Symbolic links can point across drives so I think a drive preserving 
canonicalization cannot be much more than a conversion to an absolute path -- 
similar to the now renamed `abs_path_preserve_drive` I'm adding to lit. I think 
the logic would have to be split across Windows (only abs) and non-Windows 
(realpath) again. I also think a cache miss in this case is ok, so I will 
follow up with a clang change implementing this. Thanks!


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

I would appreciate help with moving this code review forwards. Thanks!


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

In D154130#4487292 , @jdenny wrote:

> 3. Extend lit's own test suite to cover it.

I submitted an update with your suggestions #1 and #2 but this #3 is much more 
difficult because of the nature of `%{t:real}`. I'm not sure I can have a 
source/target path that includes symlinks in a way that allows me to test this, 
and even more so with substitute drives since we can't know which drive letters 
are unallocated.


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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian updated this revision to Diff 539683.
MrTrillian added a comment.

- Renamed `safe_abs_path` to `abs_path_preserve_drive` @tahonermann
- Renamed `%>t` and `%>/t` to `%{t:real}` and `%{/t:real}` @tahonermann
- Renamed `PREFIX_EXPANDED` to `SUBMODULE_PREFIX` @aaron.ballman
- Documented `%{t:real}` and `%{/t:real}` syntaxes @jdenny


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

https://reviews.llvm.org/D154130

Files:
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-excluded-header.m
  clang/test/ClangScanDeps/modules-extern-submodule.c
  clang/test/ClangScanDeps/modules-extern-unrelated.m
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-implementation-private.m
  clang/test/ClangScanDeps/modules-implementation-vfs.m
  clang/test/ClangScanDeps/modules-implicit-dot-private.m
  clang/test/ClangScanDeps/modules-incomplete-umbrella.c
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch-imports.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/modules-redefinition.m
  clang/test/ClangScanDeps/modules-symlink-dir-from-module.c
  clang/test/ClangScanDeps/modules-symlink-dir.c
  clang/test/ClangScanDeps/modules-transitive.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/test/ExtractAPI/relative_include.m
  clang/test/Lexer/case-insensitive-include-win.c
  clang/test/VFS/module-header-mismatches.m
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.abs_path_preserve_drive(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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
 
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool-required"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 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)
Index: 

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added reviewers: jansvoboda11, benlangmuir.
MrTrillian added a comment.

Add reviewers from https://reviews.llvm.org/D134923


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a subscriber: benlangmuir.
MrTrillian added a comment.

In D154130#4486898 , @tahonermann 
wrote:

>> 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.
>
> Can you link to evidence that this behavior is by design? It isn't obvious to 
> me why modulemap files would demand different behavior; especially since that 
> would exacerbate `MAX_PATH` problems.

I agree with you. The original rationale seems to be here:
https://github.com/llvm/llvm-project/blob/926f3759ec62a8f170e76a60316cc0bdd9dd2ec9/clang/lib/Lex/HeaderSearch.cpp#L257

  cpp
  // To avoid false-negatives, we form as canonical a path as we can, and 
map
  // to lower-case in case we're on a case-insensitive file system.

And this change has more context: https://reviews.llvm.org/D134923 by 
@benlangmuir

> I'm not fond of the `safe_abs_path` name; "safe" doesn't communicate anything 
> and the implementation is no more safe than `os.path.abspath` or 
> `os.path.realpath`. Suggested alternatives:
>
> - `short_abs_path`
> - `shortest_abs_path`
> - `abs_path_no_subst_drive`
> - `abs_path_preserve_drive`

Thanks for the name suggestions, those are better indeed! I will update with 
the last one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

In 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` and it should be much clearer.

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 to not need expanded paths, but that felt like a different 
investigation.

I'm happy to consider alternative syntaxes to `%>t` too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

@rnk , I would appreciate your review on this since you helped with the 
previous iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.

All premerge build failures seem like flukes.

- `x64 windows` failed 1/3 times
- `x64 debian` failed 2/3 times with a timeout (passes locally)
- `libcxx` seems to be failing for everyone: 
https://buildkite.com/llvm-project/libcxx-ci


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added a comment.
Herald added a subscriber: wangpc.

How do I take action on the test failure? It runs without issues on my machine:

  tristan@nuc-on-wood:~/project-llvm$ python3 build/bin/llvm-lit -sv --filter 
test.toy build/tools/mlir
  /test/
  
  Testing Time: 0.55s
Excluded: 1636
Passed  :1

And I see that the failure is a timeout so I wonder if it's reliable?

  Exit Code: -9
  Timeout: Reached timeout of 60 seconds


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian created this revision.
MrTrillian added reviewers: rnk, compnerd, nlopes, Jake-Egan.
Herald added a subscriber: delcypher.
Herald added a reviewer: ributzka.
Herald added a project: All.
MrTrillian requested review of this revision.
Herald added a reviewer: dang.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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.

Unfortunately, clang has `ModuleMap::canonicalizeModuleMapPath` which resolves 
the real path, so several tests had to be made aware that modulemap paths are 
special and need resolved paths. This required adding `%>/t` to `lit` to expand 
paths and normalize directory separators.

This is an updated version of https://reviews.llvm.org/D152709

How tested: built with `-DLLVM_ENABLE_PROJECTS=clang` and built `check-all` on 
both Windows and Linux (WSL-Ubuntu)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154130

Files:
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-excluded-header.m
  clang/test/ClangScanDeps/modules-extern-submodule.c
  clang/test/ClangScanDeps/modules-extern-unrelated.m
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-implementation-private.m
  clang/test/ClangScanDeps/modules-implementation-vfs.m
  clang/test/ClangScanDeps/modules-implicit-dot-private.m
  clang/test/ClangScanDeps/modules-incomplete-umbrella.c
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch-imports.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/modules-redefinition.m
  clang/test/ClangScanDeps/modules-symlink-dir-from-module.c
  clang/test/ClangScanDeps/modules-symlink-dir.c
  clang/test/ClangScanDeps/modules-transitive.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/test/ExtractAPI/relative_include.m
  clang/test/Lexer/case-insensitive-include-win.c
  clang/test/VFS/module-header-mismatches.m
  llvm/cmake/modules/AddLLVM.cmake
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/builtin_commands/diff.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/discovery.py
  llvm/utils/lit/lit/util.py
  llvm/utils/lit/setup.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/driver.py
  llvm/utils/lit/tests/Inputs/config-map-discovery/lit.alt.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool-required/lit.cfg
  llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
  llvm/utils/lit/tests/lit.cfg
  llvm/utils/llvm-lit/llvm-lit.in

Index: llvm/utils/llvm-lit/llvm-lit.in
===
--- llvm/utils/llvm-lit/llvm-lit.in
+++ llvm/utils/llvm-lit/llvm-lit.in
@@ -8,7 +8,7 @@
 
 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
Index: llvm/utils/lit/tests/lit.cfg
===
--- llvm/utils/lit/tests/lit.cfg
+++ llvm/utils/lit/tests/lit.cfg
@@ -6,6 +6,7 @@
 import subprocess
 
 import lit.formats
+import lit.util
 from lit.llvm import llvm_config
 
 # Configuration file for the 'lit' test runner.
@@ -35,7 +36,7 @@
 lit_path = os.path.join(llvm_src_root, "utils", "lit")
 else:
 lit_path = os.path.join(config.test_source_root, "..")
-lit_path = os.path.abspath(lit_path)
+lit_path = lit.util.safe_abs_path(lit_path)
 
 # Required because some tests import the lit module
 if llvm_config:
Index: llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
===
--- llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
+++ llvm/utils/lit/tests/Inputs/use-llvm-tool/lit.cfg
@@ -1,4 +1,5 @@
 import lit.formats
+import lit.util
 
 config.name = "use-llvm-tool"
 config.suffixes = [".txt"]
@@ -7,7 +8,7 @@
 config.test_exec_root = None
 import os.path
 
-this_dir = os.path.realpath(os.path.dirname(__file__))
+this_dir =