This is an archive of the discontinued LLVM Phabricator instance.

[libc++][CI] Improves clang-(tidy|query) selection.
ClosedPublic

Authored by Mordante on Dec 7 2022, 8:48 AM.

Details

Reviewers
ldionne
njames93
Group Reviewers
Restricted Project
Commits
rG30033bdaf3f4: [libc++][CI] Improves clang-(tidy|query) selection.
Summary

Hardcode the version of the tools used in the test feature script
instead of the tests. By changing the hard-coded location it's
easier to make the location flexible in the future.

Drive-by change

  • The minimum required version for clang-query is now 15, which matches our future idea as outlined in the Dockerfile.
  • The minimum required version for clang-tidy is now 16, which enables the new clang-tidy ADL plugin. This plugin is disabled for C++03 due to false positives when using noexcept, which is not an operator in C++03.

Diff Detail

Event Timeline

Mordante created this revision.Dec 7 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: arichardson. · View Herald Transcript
Mordante updated this revision to Diff 480946.Dec 7 2022, 9:09 AM

Retrigger CI to start on libc++

Mordante updated this revision to Diff 480948.Dec 7 2022, 9:16 AM

Add a dummy change to trigger libcx++ CI.

Mordante updated this revision to Diff 481285.Dec 8 2022, 8:15 AM

Remove dummy change.

Mordante published this revision for review.Dec 8 2022, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 9:33 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Dec 13 2022, 9:02 AM
libcxx/test/configs/cmake-bridge.cfg.in
33 ↗(On Diff #481285)

I would like to keep this to the smallest set of substitutions that we need to make the test suite work, so I am not sure about this approach. I'll need to take another look at this when I have more time.

ldionne accepted this revision.Dec 21 2022, 12:55 PM
ldionne added inline comments.
libcxx/utils/ci/run-buildbot
45–49 ↗(On Diff #481285)

This can be removed.

libcxx/utils/libcxx/test/features.py
143–145

I think I would dumb down the whole patch to this. It does mean that only folks with exactly clang-tidy-15 will get those tests enabled, but it's already pretty much the status quo, and doing this avoids piping through the clang-tidy version across multiple layers like buildkite-pipeline.yml and CMake.

We should also do the same for clang-query in the same patch.

If we want to be more clever about finding clang-tidy (e.g. falling back on clang-tidy --version >= 15), we can do something similar to what we do with getStdFlag in params.py and basically do a linear search with various options.

Also, restricting the changes to this means that we avoid adding another dependency from the test suite onto CMake, which is good since we want to strive for the test suite being as freestanding as possible (within reason).

This revision is now accepted and ready to land.Dec 21 2022, 12:55 PM
Mordante marked 2 inline comments as done.Dec 22 2022, 11:01 AM

Thanks for the suggestions.

libcxx/test/configs/cmake-bridge.cfg.in
33 ↗(On Diff #481285)

As discussed yesterday with @ldionne we go with a different approach so this change is undone.

libcxx/utils/libcxx/test/features.py
143–145

This didn't work as suggested, I used the same syntax as the bash test above.

Mordante updated this revision to Diff 484892.Dec 22 2022, 11:02 AM
Mordante marked an inline comment as done.

Rebased and use a different approach as discussed with @ldionne during the code review.

Mordante retitled this revision from [libc++][CI] Improves clang-tidy selection. to [libc++][CI] Improves clang-(tidy|query) selection..Dec 22 2022, 11:05 AM
Mordante edited the summary of this revision. (Show Details)
Mordante updated this revision to Diff 485133.Dec 23 2022, 8:40 AM

Rebased to test CI.

Mordante updated this revision to Diff 485141.Dec 23 2022, 9:26 AM

CI fixes.

Mordante updated this revision to Diff 485143.Dec 23 2022, 10:15 AM

Another attempt to fix the CI.

Mordante updated this revision to Diff 485409.Dec 27 2022, 11:43 AM

Test the CI, the last time seemed a bit flacky.

Mordante updated this revision to Diff 485412.Dec 27 2022, 12:35 PM

Restart CI.

Mordante updated this revision to Diff 485505.Dec 28 2022, 8:05 AM

CI fixes.

Mordante updated this revision to Diff 485510.Dec 28 2022, 8:27 AM

Retrigger CI.

Mordante updated this revision to Diff 485513.Dec 28 2022, 8:48 AM

Another attempt to start the CI.

Mordante updated this revision to Diff 485515.Dec 28 2022, 9:12 AM

CI test again...

Mordante updated this revision to Diff 485518.Dec 28 2022, 9:40 AM

CI seems not really willing to start.

Mordante edited the summary of this revision. (Show Details)Dec 28 2022, 11:00 AM
This revision was automatically updated to reflect the committed changes.