Page MenuHome
Differential D9450

macOS deps: Support building clang tidy
ClosedPublic
Actions

Authored by Ankit Meel (ankitm) on Nov 4 2020, 12:43 PM.

Details

Reviewers
Ray Molenkamp (LazyDodo)
Sebastián Barschkis (sebbas)
Group Reviewers
Platform: macOS
Commits
rB719dfd40889a: macOS deps: Support building clang tidy
Summary

This patch builds clang-extra-tools on macOS for the
clang-tidy binary. The script "run-clang-tidy.py" is
also harvested because using the CMAKE_C[XX]_CLANG_TIDY
option can miss out some files (like makesrna), and using the
script is faster as it does not compile the files.

Thanks to @Ray Molenkamp (LazyDodo) for the base patch.

It is based on patch D8502. I had come up with https://pasteall.org/VwhU/slim which could be
cleaner if we used the unified llvm source dir, and then use LLVM_ENABLE_PROJECTS.

The version of clang tidy that macOS developers get is 9. It is tied to LLVM library update
and thus will need us to change .clang-format to avoid newer changes.

For newer versions of clang tidy, build from source or use a package manager or
official website of llvm to get the binaries.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D8502 (branched from master)
Build Status
Buildable 11104
Build 11104: arc lint + arc unit

Event Timeline

Ankit Meel (ankitm) created this revision.Nov 4 2020, 12:43 PM
Harbormaster completed remote builds in B11104: Diff 30709.Nov 4 2020, 12:43 PM
Ankit Meel (ankitm) requested review of this revision.Nov 4 2020, 12:43 PM
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Nov 4 2020, 12:50 PM
Ray Molenkamp (LazyDodo) requested changes to this revision.Nov 4 2020, 2:40 PM
Ray Molenkamp (LazyDodo) added a subscriber: Ray Molenkamp (LazyDodo).
Ray Molenkamp (LazyDodo) added inline comments.
build_files/build_environment/cmake/clang.cmake
33

Why this change? or more specifically why this change in this change set? if we want to change from make to ninja that be fine (given you can get the other platform devs on board there) but i do not see how it is related to building clang-extra or not, nor does the patch description give any insights on why this change may be required

56

use echo . like elsewhere in the builder.

This revision now requires changes to proceed.Nov 4 2020, 2:40 PM
Ankit Meel (ankitm) updated this revision to Diff 30717.Nov 4 2020, 2:46 PM
Ankit Meel (ankitm) marked 2 inline comments as done.
  • review update, remove irrelevant changes.
Harbormaster completed remote builds in B11107: Diff 30717.Nov 4 2020, 2:48 PM
Ankit Meel (ankitm) updated this revision to Diff 30724.Nov 4 2020, 4:00 PM
  • rename BUILD_CLANG_EXTRA to BUILD_CLANG_TOOLS and similar for other variables.
  • Fix typos
Harbormaster completed remote builds in B11113: Diff 30724.Nov 4 2020, 4:00 PM
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Nov 4 2020, 4:00 PM
Ankit Meel (ankitm) added a reviewer: Sergey Sharybin (sergey).Nov 4 2020, 5:42 PM
Ankit Meel (ankitm) added a project: Platforms, Builds, Tests & Devices.
Sergey Sharybin (sergey) edited reviewers, added: Sebastián Barschkis (sebbas); removed: Sergey Sharybin (sergey).Nov 5 2020, 9:28 AM

I am all up for making clang-tidy available on macOS if it is not available via Xcode or brew.

However, not really maintaining this platform, Sebastian does ;)

Ankit Meel (ankitm) added a comment.Nov 5 2020, 9:33 AM

The patch as of now builds extra tools on linux also. Should I remove it from linux and keep it macOS only?
Brew didn't accept the formula https://github.com/Homebrew/homebrew-core/pull/61714

Ankit Meel (ankitm) updated this revision to Diff 30821.Nov 6 2020, 11:26 AM
  • remove linux.
Harbormaster completed remote builds in B11161: Diff 30821.Nov 6 2020, 11:26 AM
Ankit Meel (ankitm) added a reviewer: Platform: macOS.Nov 11 2020, 8:21 PM
Ankit Meel (ankitm) added a project: Platform: macOS.
Sebastián Barschkis (sebbas) added a comment.Nov 12 2020, 12:55 PM

Can someone give the reason why Linux was removed? In my view it would be nice to have platforms aligned as much as possible.

Ankit Meel (ankitm) added a subscriber: Sergey Sharybin (sergey).Nov 12 2020, 1:09 PM

Can someone give the reason why Linux was removed? In my view it would be nice to have platforms aligned as much as possible.

Apparently @Sergey Sharybin (sergey) wasn't a subscriber until now. So I asked in chat and did't get an affirmative answer.

Sergey Sharybin (sergey) added a subscriber: Sybren A. Stüvel (sybren).Nov 12 2020, 1:22 PM

I am not sure what is the benefit of pre-compiling Clang-Tidy on Linux. Sounds like opening a can of worms with all sort ABI compatibility issues. Clang-Tidy is easily available on Linux, and to me it's similar to why we don't compile, say, GCC for Linux.

We do have clang-format compiled, but this is because different versions might behave differently, causing unwanted noise all over the code. I am not aware of such issue with clang-tidy. Unsupported options are AFAIR ignored, so it is still possible to use older clang-tidy versions.

There are benefits of not compiling clang-tidy:

  • Ability to easily use latest version as it comes out
  • Easier and safer (in terms of obscure ABI issues) setup

I am not sure what are the benefits of compiling clang-tidy.

I would suggest moving forward with clang-tidy on macOS, as we all agree this is something we want/need to do. Linux we can/should discuss separate I think.

On a completely unrelated topic, proper spelling of Linux platform maintainer is @Sybren A. Stüvel (sybren) ;)

Sebastián Barschkis (sebbas) accepted this revision.Dec 4 2020, 3:08 PM

It looks like this is the best way to get clang-tidy support on macOS with no hassle. Let's go for it!
Similarly to clang-format, developers will still be able to manually set their own version of clang-tidy.

By going this way, I would argue in favor of precompiling it on Linux too. Relying on clang-tidy to behave the same across different versions on different OSs seems a bit opportunistic to me.
Unless this premise is documented somewhere.

Ankit Meel (ankitm) retitled this revision from Deps_Builder: Add clang-extra-tools to clang to macOS deps: Support building clang tidy.Dec 4 2020, 4:17 PM
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

@Sebastián Barschkis (sebbas) Should I land it, or would you like to do it along with the svn updates ?
After both are done, I'll fix the following in a follow up commit

if((UNIX AND NOT APPLE) OR (CMAKE_GENERATOR MATCHES "^Visual Studio.+"))
  option(WITH_CLANG_TIDY "Use Clang Tidy to analyze the source code (only enable for development on Linux using Clang, or Windows using the Visual Studio IDE)" OFF)
  mark_as_advanced(WITH_CLANG_TIDY)
endif()
Sebastián Barschkis (sebbas) added a comment.Dec 4 2020, 4:22 PM

@Ankit Meel (ankitm) I'd say it's safe to land now!

Ray Molenkamp (LazyDodo) accepted this revision.Dec 4 2020, 4:32 PM
This revision is now accepted and ready to land.Dec 4 2020, 4:32 PM
Closed by commit rB719dfd40889a: macOS deps: Support building clang tidy (authored by Ankit Meel (ankitm)). · Explain WhyDec 4 2020, 4:45 PM
This revision was automatically updated to reflect the committed changes.
Ankit Meel (ankitm) added a commit: rB719dfd40889a: macOS deps: Support building clang tidy.

Revision Contents
Changeset List

PathSize
build_files/
build_environment/
cmake/
41 lines
2 lines
2 lines
3 lines

Diff 30709

View Options

build_files/build_environment/cmake/clang.cmake

Loading...
View Options

build_files/build_environment/cmake/harvest.cmake

Loading...
View Options

build_files/build_environment/cmake/llvm.cmake

Loading...
View Options

build_files/build_environment/cmake/versions.cmake

Loading...