[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