Page MenuHomePhabricator

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

Authored by philnik on Thu, Jan 13, 6:32 PM.

Details

Reviewers
Mordante
ldionne
Group Reviewers
Restricted Project
Commits
rGc10cbb243caf: [libc++] Install clang-tidy in docker containers
Summary

Install clang-tidy

Diff Detail

Event Timeline

philnik created this revision.Thu, Jan 13, 6:32 PM
philnik requested review of this revision.Thu, Jan 13, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 13, 6:32 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

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 added inline comments.Fri, Jan 14, 9:38 AM
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 added inline comments.Fri, Jan 14, 9:52 AM
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?

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 updated this revision to Diff 400097.Fri, Jan 14, 11:48 AM
philnik marked 4 inline comments as done.
  • Address comments
philnik retitled this revision from [libc++] Install clang-tidy in docker containers and create command db symlink to [libc++] Install clang-tidy in docker containers and create compile_commands.json.Fri, Jan 14, 11:49 AM
ldionne added inline comments.Fri, Jan 14, 12:19 PM
libcxx/CMakeLists.txt
25 ↗(On Diff #400097)

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

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

@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

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 updated this revision to Diff 400609.Mon, Jan 17, 11:02 AM
  • Remove CMake changes
philnik retitled this revision from [libc++] Install clang-tidy in docker containers and create compile_commands.json to [libc++] Install clang-tidy in docker containers.Mon, Jan 17, 11:04 AM
philnik edited the summary of this revision. (Show Details)
ldionne accepted this revision.Mon, Jan 17, 11:26 AM
This revision is now accepted and ready to land.Mon, Jan 17, 11:26 AM
This revision was automatically updated to reflect the committed changes.