[PATCH] D83578: [test] Replace a fragile lit feature (substitution in an argument place) with command -v

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 22 16:15:57 PDT 2021


MaskRay added a comment.

In D83578#2707573 <https://reviews.llvm.org/D83578#2707573>, @jhenderson wrote:

>> lit can do substitution in an argument place (like llvm-ar in ln -s llvm-ar) which is counterintuitive.
>
> Why is it counterintuitive? To me, it is most intuitive if references to an LLVM executable are always to the version in the build directory. By substituting the full path, it is always possible to see which version of a tool is actually being referenced, which is important for test debugging, if nothing else.
>
> FWIW, I find the patch makes the testd less readable. Admittedly, I'm not a massive Linux user, but I had to look up the meaning of `command -v` online, and it's quite likely I'll forget it again later. It's also not clear to future test writers why one might need to use this pattern.

The utilities and the shell are all of Unix style. It is well established that argv[0] is magic and the shell/OS can find it from `PATH`. `argv[i] (i!=0)` is not and an argument such as`ls`, `ln`, `llvm`, and `objdump` in a non argv[0] place does not do anything special.
For example, `echo ls` prints `ls`, not the absolute path to `ls`. I think Windows cmd.exe behaves this way as well.

There is a little mental gap for readers who haven't used `command -v` before, but this probably still looks better than `ln -s llvm-ranlib ....` which gives a false impression that this command is incorrect.

We could also add a replacement and use `ln -s %llvm-objdump ...` but all uses except the few tool-name.test tests use the raw names. The replacement rules seem a bit overkill (needs some code in `lit.*.py` files.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83578



More information about the llvm-commits mailing list