[clang][deps] Substitute clang-scan-deps executable in lit tests

[jansvoboda11]

Summary

The lit tests for clang-scan-deps invoke the tool without going through the substitution system. While the test runner correctly picks up the clang-scan-deps binary from the build directory, it doesn't print its absolute path. When copying the invocations when reproducing test failures, this can result in command not found: clang-scan-deps errors or worse yet: pick up the system clang-scan-deps. This patch adds new local %clang-scan-deps substitution.

[lxfind]

Thank you!

[dblaikie]

Hmm, don't we have some other substitution feature that avoids the need to add the %? (like, I think LLVM's tests don't usually use the % for many tool names - but they still make for good copy/pastable command lines, if I'm not mistaken?)

[dexonsmith]

In D107155#2916664, @dblaikie wrote:

Hmm, don't we have some other substitution feature that avoids the need to add the %? (like, I think LLVM's tests don't usually use the % for many tool names - but they still make for good copy/pastable command lines, if I'm not mistaken?)

I think that'd be adding clang-scan-deps somehow to the tools variable in clang/test/lit.cfg.py:
https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py#L59
(which will then call add_tool_substitutions at https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py#L111)

[dblaikie]

In D107155#2917400, @dexonsmith wrote:

In D107155#2916664, @dblaikie wrote:

Hmm, don't we have some other substitution feature that avoids the need to add the %? (like, I think LLVM's tests don't usually use the % for many tool names - but they still make for good copy/pastable command lines, if I'm not mistaken?)

I think that'd be adding clang-scan-deps somehow to the tools variable in clang/test/lit.cfg.py:
https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py#L59
(which will then call add_tool_substitutions at https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py#L111)

Hmm, fair enough. Yeah - guess all the actual clang tests use %clang, %clang_cc1 - so I guess there's precedent in both directions. I wonder what the "norm" is (which tools use one approach, which use the other). But anyway, no objection in this case given the precedent. Though it does sound like this is a "tool" like the other cases, so maybe should be handled that way - up to you folks, though.

[dblaikie]

Just because I was curious. Some grep/seding. Nothing definitive:

10188 %clang
 1289 %clang_analyze_cc1
32253 %clang_cc1
   22 clang-check
  503 %clang_cl
   59 clang-offload-bundler
    2 clang-offload-wrapper
   69 clang-rename
    2 clang-repl
   52 clang-scan-deps
  212 %clangxx
  257 %s
   14 %scan-build

So bit of a mix.

[jansvoboda11]

Move substitution to clang/test/lit.cfg.py, revert % prefixes.

[jansvoboda11]

In D107155#2916664, @dblaikie wrote:

Hmm, don't we have some other substitution feature that avoids the need to add the %? (like, I think LLVM's tests don't usually use the % for many tool names - but they still make for good copy/pastable command lines, if I'm not mistaken?)

You're right, I wasn't aware of this (and usually see %clang everywhere).

In D107155#2917400, @dexonsmith wrote:

I think that'd be adding clang-scan-deps somehow to the tools variable in clang/test/lit.cfg.py:
https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py#L59
(which will then call add_tool_substitutions at https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py#L111)

Nice, that seems like the best place for this.

I updated the patch to reflect both findings, thanks!

[dblaikie]

Sounds good to me

clang/test/lit.cfg.py

 # For each occurrence of a clang tool name, replace it with the full path to
 # the build directory holding that tool.  We explicitly specify the directories
 # to search to ensure that we get the tools just built and not some random
 # tools that might happen to be in the user's PATH.
 tool_dirs = [config.clang_tools_dir, config.llvm_tools_dir]
 
 tools = [
     'apinotes-test', 'c-index-test', 'clang-diff', 'clang-format', 'clang-repl',
-    'clang-tblgen', 'opt', 'llvm-ifs', 'yaml2obj',
+    'clang-tblgen', 'clang-scan-deps', 'opt', 'llvm-ifs', 'yaml2obj',
     ToolSubst('%clang_extdef_map', command=FindTool(
         'clang-extdef-mapping'), unresolved='ignore'),
 ]
 
 if config.clang_examples:
     config.available_features.add('examples')
     tools.append('clang-interpreter')