codechecker icon indicating copy to clipboard operation
codechecker copied to clipboard

Raise an exception instead of silently failing on a broken diagtool

Open Szelethus opened this issue 3 years ago • 3 comments

Especially if you build your own clang, you are very likely to forget building the diagtool target.

I personally build my clang with the following command:

ninja -C build/ clang clang-tidy

This patch serves as a reminder not to forget the diagtool target either. Its not a big deal if its not built, but some tests fail, and the silent failure leads to a miserable debugging experience.

I did not include any tests because having a broken diagtool around would be a hassle. image

Szelethus avatar Jun 28 '22 15:06 Szelethus

One thing to add: diagtool will be broken if you

  • build diagtool at least once
  • build with shared libraries
  • build some dependencies of diagtool on a different version of llvm-project, but not diagtool itself

Which happens all the time as you are only testing clang/clang-tidy.

Szelethus avatar Jun 29 '22 09:06 Szelethus

I'm a little confused about this solution. In line 69 get_warnings() function has an early return if diagtool is not found. How is it possible that this binary is not available in your environment and the function goes through this check?

get_diagtool_bin() is a function which checks whether diagtool is available. This function is used in line 163. So in case diagtool is not available, --warnings flag is not registered among the command-line arguments. So the control flow shouldn't even reach the added exception throwing, because argparse module ceases execution at the very beginning. If we intend to give a message to the user then we should remove the condition in line 163 so the debug message is reached. Side note: get_diagtool_bin() also prints a debug message, but it is also not printed for the same reason.

bruntib avatar Jul 27 '22 11:07 bruntib

Oh, I've just read your previous comment which may give an answer to my question. So it is possible that diagtool is available, but broken. In this case I think this message could go to error log level, because it crashes CodeChecker execution.

bruntib avatar Jul 27 '22 11:07 bruntib