[PATCH] D103014: [lit] Attempt for fix tests failing because of 'warning: non-portable path to file'

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added a comment.

In D103014#2806839 <https://reviews.llvm.org/D103014#2806839>, @Meinersbur wrote:

> In D103014#2805545 <https://reviews.llvm.org/D103014#2805545>, @krisb wrote:
>
>>> Now it is resolving twice?!?
>>
>> I think it would be better to get rid of lots of `\..\..\..\..\` when `path()` is used to construct paths for source, object, and other directories and files within a site config. So this is why I made it resolved twice (the first one for `__file__` to remove symlinks and the second one to simplify the resulting path). What do you think? Does this make sense?
>
> I originally asked whether changing the point where resolve is called is intentional, and this seems to be the intention (although unrelated to this patch). The second call will resolve all the paths of `__file__` as well, making the first resolve redundant.
>
> Greenlighting the patch, but please remove the first call to `resolve()`.

Then it seems I misinterpreted your comment.
Resolving `__file__` first and then concatenating is not the same as concatenating then resolving if we are talking about symlinks.
For example, having

  ~/workspace/llvm/llvm-project$ ln -s llvm/test/ test

we would get different results

  $ python
  Python 3.7.10 (default, Feb 20 2021, 21:15:28) 
  [GCC 9.3.0] on linux
  >>> from pathlib import Path
  >>> Path('test').resolve().parent
  PosixPath('/home/kbessonova/workspace/llvm/llvm-project/llvm')
  >>> Path('test').parent.resolve()
  PosixPath('/home/kbessonova/workspace/llvm/llvm-project')

I thought you were talking about this case (for example, if site configs are symlinks themselves or placed under symlinked directory within llvm tree) and it isn't something I was about to change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103014