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

[Douglas Yung via Phabricator via cfe-commits]

dyung added a comment.

Sorry to comment so late, but I just had time to look into this a little bit. 
This change broke running lit tests for me on Windows, and I wanted to see if 
there was a way I could restore it.

For reference, my sources are stored locally at `C:\Dev\git\dev`, but I until 
this change, I was able to access them through a file system junction point at 
`C:\src\git\dev` and run lit tests using that. But after this change, I can no 
longer run the lit tests when I am in `C:\src\git\dev`. My system is setup this 
way because we have weird A/V scanning rules that only exclude certain named 
directories, and my original path wasn't one of them, so I used the file system 
junction point to get my builds excluded while continuing to use the path 
structure that was familiar to me and that I also use on other systems 
(mac/linux).

Since this change seems to be targeted towards preventing resolving paths that 
are greater than `MAX_PATH` on Windows, could we possibly leave the 
substitution in, but check if the resulting path is longer than MAX_PATH rather 
than just excluding it completely?

Specifically, where I am seeing the difference is in the new function 
`abs_path_preserve_drive()` in util.py where we are now using 
`os.path.abspath()` instead of `os.path.realpath()`. I'm not completely sure 
what the effects of such a change might be to the rest of this change, but 
keeping the old conversion and then checking against `MAX_PATH` it seems would 
still accomplish your goal while allowing cases like mine to continue working 
as they previously did.

Thoughts?


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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann 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.

MrTrillian wrote:
> 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.
I think `trySimplifyPath()` is not particularly relevant as it just performs a 
simple canonicalization (removal of `.` and `..` path components without regard 
for symlink traversal) and case insensitive comparison with the remaining 
components with a "real" path that is presumed to already be devoid of such 
components. It is therefore sensitive to structure, but only for the (non-`.` 
and non-`..`) components present in the (non-real) `Components` vector; the 
"real" path may have more leading components (the `Components` vector may 
represent a relative path). The presence of a substitute drive in the "real" 
path won't contribute to a structural difference unless the `Components` vector 
is absolute but with a drive other than the substitute drive or if it is 
relative but starting at a higher directory level than the substitute drive; 
neither of which should be the case for this test when `%t` is consistently 
used.

The only relevant user path 

[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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> @tahonermann are all your comments addressed at this point?

Apologies for the late response; I was on vacation last week.

No. I'm still skeptical that there is ever a desire to expand substitute drives.




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.

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).


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

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: llvm/utils/lit/lit/discovery.py:60
+cfgpath = util.abs_path_preserve_drive(cfgpath)
+target = config_map.get(cfgpath)
 if target:

RKSimon wrote:
> RKSimon wrote:
> > Found the problem - you have moved the os.path.normpath call inside 
> > abs_path_preserve_drive, causing the path canonicalization fails - removing 
> > it and going back to os.path.normcase(cfgpath) seems to fix the issue - 
> > I'll push my fix shortly.
> 5ccfa1568130 fixed my builds but broke this - 
> https://lab.llvm.org/buildbot/#/builders/216/builds/24966
> 
> Any suggestions would be appreciated, but I can't think of anything but 
> reverting both commits until we get to the bottom of it.
Panic over - I think I've fixed this with d6f1880c629d


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

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: llvm/utils/lit/lit/discovery.py:60
+cfgpath = util.abs_path_preserve_drive(cfgpath)
+target = config_map.get(cfgpath)
 if target:

RKSimon wrote:
> Found the problem - you have moved the os.path.normpath call inside 
> abs_path_preserve_drive, causing the path canonicalization fails - removing 
> it and going back to os.path.normcase(cfgpath) seems to fix the issue - I'll 
> push my fix shortly.
5ccfa1568130 fixed my builds but broke this - 
https://lab.llvm.org/buildbot/#/builders/216/builds/24966

Any suggestions would be appreciated, but I can't think of anything but 
reverting both commits until we get to the bottom of it.


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

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: llvm/utils/lit/lit/discovery.py:60
+cfgpath = util.abs_path_preserve_drive(cfgpath)
+target = config_map.get(cfgpath)
 if target:

Found the problem - you have moved the os.path.normpath call inside 
abs_path_preserve_drive, causing the path canonicalization fails - removing it 
and going back to os.path.normcase(cfgpath) seems to fix the issue - I'll push 
my fix shortly.


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

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

@MrTrillian This is failing for me with:

  C:\LLVM\ninja>ninja check-llvm-codegen-x86
  [0/1/0/1] Running lit suite 
C:/LLVM/llvm-project/llvm/test/CodeGen/X86llvm-lit.py: 
C:\LLVM\llvm-project\llvm\utils\lit\lit\TestingConfig.py:151: fatal: unable to 
parse config file 'C:\\LLVM\\llvm-project\\llvm\\test\\lit.cfg.py', traceback: 
Traceback (most recent call last):
File "C:\LLVM\llvm-project\llvm\utils\lit\lit\TestingConfig.py", line 139, 
in load_from_path
  exec(compile(data, path, "exec"), cfg_globals, None)
File "C:\LLVM\llvm-project\llvm\test\lit.cfg.py", line 21, in 
  config.test_format = lit.formats.ShTest(not llvm_config.use_lit_shell)
  ^
  AttributeError: 'NoneType' object has no attribute 'use_lit_shell'


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

[Saleem Abdulrasool via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG05d613ea931b: [lit][clang] Avoid realpath on Windows due to 
MAX_PATH limitations (authored by compnerd).

Repository:
  rG LLVM Github Monorepo

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 

[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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir 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);

MrTrillian wrote:
> 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.
Thanks, looks good.


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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

LGTM other than my suggestion to add a comment. @tahonermann are all your 
comments addressed at this point?




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

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.


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: 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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir 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);

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 ..





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

MrTrillian wrote:
> 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.
I wonder if we should just remove `tryGetRealPathName`; it's not actually the 
real path in many cases.  Anyway, not for this patch, your change here seems 
fine.



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)

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?



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

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


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:
> 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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:655
+  SmallString<4096> AbsPathBuf = Name;
+  SmallString<4096> RealPathBuf;
+  if (!FS->makeAbsolute(AbsPathBuf)) {

8k is a lot of stack space. The only reason this was 4k in the first place is 
it was originally using `char[PATH_MAX]` and unix `realpath` directly.  I'd 
suggest just dropping to 128 per path.



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

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



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

Why is this change needed?


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.

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

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

Yes, that is fine.




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.

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.


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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann 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.

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.


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