This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jansvoboda11 on Jul 30 2021, 5:50 AM.

Details

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.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Jul 30 2021, 5:50 AM
jansvoboda11 created this revision.
jansvoboda11 edited the summary of this revision. (Show Details)
lxfind accepted this revision.Jul 30 2021, 8:10 AM

Thank you!

This revision is now accepted and ready to land.Jul 30 2021, 8:10 AM

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?)

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, 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.

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.

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

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).

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 accepted this revision.Aug 2 2021, 12:00 PM

Sounds good to me