[libc++] Install clang-tidy in docker containers

[philnik]

Summary

Install clang-tidy

[Mordante]

The Docker part looks good! I think it would be best to first commit the Docker part. After that's propagated to the build nodes we can look at the part to use it.
WDYT?

libcxx/utils/ci/run-buildbot

97 ↗

(On Diff #399865)

I think the name db is not really descriptive, how about create-compile_commands-symlink?

200 ↗

(On Diff #399865)

I wonder whether we need this in all configurations.
Secondly let's make sure we export the compile commands in the configuration where we use this.

[ldionne]

I'm wondering whether we should perhaps do this from the libc++ build itself, which might help resolve https://github.com/llvm/llvm-project/issues/45348 at the same time. Any thoughts?

libcxx/utils/ci/run-buildbot

200 ↗

(On Diff #399865)

I think we need to, because the clang-tidy check is run in all configurations.

[philnik]

libcxx/utils/ci/run-buildbot

200 ↗

(On Diff #399865)

@ldionne
Is there any way to run the test only if clang-tidy is found and there is a compile_commands.json in the root directory? Otherwise this would require clang-tidy for the libc++ test suite to succeed.

@Mordante
I didn't put it in all configs. I don't know exactly what configs we want to check. Do we only want to check C++20 code or do we want to check all code? I think the different platforms should all be checked, since there are a few places where we have platform-specific code.

[ldionne]

libcxx/utils/ci/run-buildbot

200 ↗

(On Diff #399865)

For checking whether clang-tidy exists, you can add this to libcxx/utils/libcxx/test/features.py:

Feature(name='has-clang-tidy',
        when=lambda cfg: runScriptExitCode(cfg, ['clang-tidy --version']) != 0),

(assuming clang-tidy --version makes sense)

For compile_commands.json, like I said in my other comment, I would much rather we produce something that clang-tidy can use unconditionally -- is that realistic?

[philnik]

I just checked the clang-tidy docs again and it seems we can just always generate a compile_commands.json and point clang-tidy to it with -p <path-to-db>.

[philnik]

• Address comments

[ldionne]

libcxx/CMakeLists.txt

25

Also, I would suggest moving it to runtimes/ instead so it is used when we're building libc++abi and libunwind too.

[ldionne]

@sammccall Do you know whether clang-tidy can use compile_flags.txt? If it did, that could perhaps solve both this issue and also https://github.com/llvm/llvm-project/issues/45348.

I looked at compile_commands.json locally and it made it clear that we can't ship it as part of our installation, but compile_flags.txt would be an option. Thoughts?

[sammccall]

In D117268#3244971, @ldionne wrote:

@sammccall Do you know whether clang-tidy can use compile_flags.txt? If it did, that could perhaps solve both this issue and also https://github.com/llvm/llvm-project/issues/45348.

Yes and no:

• if you run clang-tidy on a file, it will correctly discover compile_flags.txt (or you can force it with -p) and use its flags
• but if you want to run clang-tidy on a whole project, you need to somehow choose what files are in scope. run-clang-tidy.py is the usual tool for this and only supports compile_commands.json.

So it depends exactly how you plan to run clang-tidy.

I looked at compile_commands.json locally and it made it clear that we can't ship it as part of our installation, but compile_flags.txt would be an option. Thoughts?

FWIW, agree that compile_commands.json is never portable across machines but a carefully-constructed compile_flags.txt can be.

[ldionne]

In D117268#3246850, @sammccall wrote:

In D117268#3244971, @ldionne wrote:

@sammccall Do you know whether clang-tidy can use compile_flags.txt? If it did, that could perhaps solve both this issue and also https://github.com/llvm/llvm-project/issues/45348.

Yes and no:

• if you run clang-tidy on a file, it will correctly discover compile_flags.txt (or you can force it with -p) and use its flags
• but if you want to run clang-tidy on a whole project, you need to somehow choose what files are in scope. run-clang-tidy.py is the usual tool for this and only supports compile_commands.json.

Awesome, thanks for the information. In our case, we are not even trying to lint the .cpp files that compile_commands.json contains -- we're trying to lint our headers, and those are not mentioned in compile_commands.json.

@philnik I am now fairly convinced that the right path forward is to provide compile_flags.txt and to point clang-tidy to it in D117174 using -p.

I'd suggest dropping the compile_commands.json part of this change and adding compile_flags.txt in D117174 (in other words, this patch could only touch the Dockerfile). To begin, I would place compile_flags.txt in a place like libcxx/test/libcxx/, and as a separate commit, we can move it to the libc++ installation proper so as to fix https://github.com/llvm/llvm-project/issues/45348. The reason for doing that in a separate commit is that I strongly suspect it's going to be harder to land than it seems due to people including compile_flags.txt, so it would be best to keep it separate in order to unblock you.

Sorry for the churn, but at last I think this is a much cleaner approach.

P.S.: Based on the GitHub bug report, I think we'll want our compile_flags.txt to contain this:

-stdlib=libc++
-stdlib++-isystemv1
-xc++-header

[sammccall]

SGTM!

-stdlib++-isystemv1

This puts the relative path v1 on the include path (it's a fancy -I v1).
In case of compile_flags.txt, the working directory is the dir containing compile_flags.txt.
So if this is to be placed somewhere other than the parent of the v1 header directory, it will need to be some other (relative or absolute) path to that directory.

[philnik]

• Remove CMake changes

libcxx/CMakeLists.txt

list(INSERT CMAKE_MODULE_PATH 0
   "${CMAKE_CURRENT_SOURCE_DIR}/cmake"
   "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules"
   "${LLVM_COMMON_CMAKE_UTILS}"
   "${LLVM_COMMON_CMAKE_UTILS}/Modules"
   )
 
 set(CMAKE_FOLDER "libc++")
 
+set(CMAKE_EXPORT_COMPILE_COMMANDS On)
+
 set(LIBCXX_SOURCE_DIR  ${CMAKE_CURRENT_SOURCE_DIR})
 set(LIBCXX_BINARY_DIR  ${CMAKE_CURRENT_BINARY_DIR})
 set(LIBCXX_BINARY_INCLUDE_DIR "${LIBCXX_BINARY_DIR}/include/c++build")
 
 if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR LIBCXX_STANDALONE_BUILD)
   project(libcxx CXX C)
 
   set(PACKAGE_NAME libcxx)

libcxx/utils/ci/Dockerfile

 RUN ln -fs /usr/bin/clang++-$(($LLVM_LATEST_VERSION + 1)) /usr/bin/c++ && [ -e $(readlink /usr/bin/c++) ]
 RUN ln -fs /usr/bin/clang-$(($LLVM_LATEST_VERSION + 1)) /usr/bin/cc && [ -e $(readlink /usr/bin/cc) ]
 
 # Install clang-format
 RUN apt-get install -y clang-format-$LLVM_LATEST_VERSION
 RUN ln -s /usr/bin/clang-format-$LLVM_LATEST_VERSION /usr/bin/clang-format && [ -e $(readlink /usr/bin/clang-format) ]
 RUN ln -s /usr/bin/git-clang-format-$LLVM_LATEST_VERSION /usr/bin/git-clang-format && [ -e $(readlink /usr/bin/git-clang-format) ]
 
+# Install clang-tidy
+RUN apt-get install -y clang-tidy-$LLVM_LATEST_VERSION
+RUN ln -s /usr/bin/clang-tidy-$LLVM_LATEST_VERSION /usr/bin/clang-tidy && [ -e $(readlink /usr/bin/clang-tidy) ]
+
 # Install the most recent GCC
 ENV GCC_LATEST_VERSION=11
 RUN add-apt-repository ppa:ubuntu-toolchain-r/test
 RUN apt-get update && apt install -y gcc-$GCC_LATEST_VERSION g++-$GCC_LATEST_VERSION
 
 # Install a recent CMake
 RUN wget https://github.com/Kitware/CMake/releases/download/v3.21.1/cmake-3.21.1-linux-x86_64.sh -O /tmp/install-cmake.sh
 RUN bash /tmp/install-cmake.sh --prefix=/usr --exclude-subdir --skip-license