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

Sam McCall via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 10 14:11:14 PDT 2020


sammccall added a comment.

In D83578#2145020 <https://reviews.llvm.org/D83578#2145020>, @MaskRay wrote:

> > How do you think we should sequence this?
>
> Apologies that as a non-native speaker I don't know what "sequence" means in the sentence. If you mean "dropping the feature from lit", are you signing off for the work? :) I'd be happy to review (I don't know enough about lit to locate the feature...)


Sorry to be unclear, I was on my phone and being lazy!

I mean do we want to e.g.:

- land patches removing particular substitutions and their uses, and eventually remove the feature if they can all be eliminated
- determine whether removing the feature is feasible, then land patches like this one, then remove the feature
- land patches like this one, then determine whether removing the feature is feasible, then remove the feature (I'm afraid this way will get stuck in a bad state)

But to back up a bit, I'm a bit worried about a chesterton's fence situation - why are these substitutions there. Unfortunately r315085 deleted the explanation for these substitutions, which read:

  # For each occurrence of an llvm tool name as its own word, replace it
  # with the full path to the build directory holding that tool.  This
  # ensures that we are testing the tools just built and not some random
  # tools that might happen to be in the user's PATH.  Thus this list
  # includes every tool placed in $(LLVM_OBJ_ROOT)/$(BuildMode)/bin
  # (llvm_tools_dir in lit parlance).

This was originally added to resolve https://bugs.llvm.org/show_bug.cgi?id=8199 in https://github.com/llvm/llvm-project/commit/dc276c315cec9e33df8e0e171b686f79143f651d and is still described in the testing guide.

I'm not sure the causes of that bug are gone, so we might just be reintroducing it. It's easy for me to say "it's better if the path is set correctly, and possibly hermetically" but I'm not confident I have a good enough handle on the various platforms and build modes to know how to get there...


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