clang-tidy: allow target to be disabled #109

silverjam · 2022-04-16T02:02:02Z

When attempting to package libswiftnav as part of swiftnav-rs, the automation for clang-tidy that generates a common config for all of our repos causes a validation check in cargo publish to fail, this attempts to verify that builds don't modify themselves, see: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815

Example:

Compiling swiftnav-sys v0.7.2-alpha.0 (/Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0)
    Finished dev [unoptimized + debuginfo] target(s) in 40.27s
error: failed to verify package tarball

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: /Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0/src/libswiftnav/.clang-tidy

  To proceed despite this, pass the `--no-verify` flag.

To work around this we are re-introducing the ability to disable the clang-tidy targets in the "modern" clang-tidy cmake module. This is ported over from the OldClangTidy.cmake module.

Testing here: https://github.com/swift-nav/libswiftnav/commits/silverjam/disable-clang-tidy (and here for Rust: https://github.com/swift-nav/swiftnav-rs/commits/silverjam/disable-clang-tidy).

With ❯ cmake -B build -S . -DENABLE_CLANG_TIDY=OFF:

[..]
-- clang-tidy is disabled globally
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ubuntu/dev/libswiftnav/build

With ❯ cmake -B build -S . -Dlibswiftnav_ENABLE_CLANG_TIDY=OFF:

[..]
-- Performing Test HAVE_CXX_FLAG_WNO_FORMAT_EXTRA_ARGS
-- Performing Test HAVE_CXX_FLAG_WNO_FORMAT_EXTRA_ARGS - Success
-- libswiftnav clang-tidy support is DISABLED
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ubuntu/dev/libswiftnav/build

woodfell · 2022-04-16T07:02:28Z

This is one of the things I tried when writing this module but had to change it because of the idiotic way clang-tidy picks up config. By having it in the build dir clang-tidy will only ever work properly when run from the build dir, whereas when config is in the repo root clang-tidy will work properly when run in any subdirectory. This can cause problems with some IDEs and versions of clangd. (The same is true for .clang-format)

To get around the git thing we just add .clang-tidy to .gitignore in other repos since it fixes the issue without breaking other workflows. A proper solution would be to use a linter where a config file can be passed on the command line

woodfell · 2022-04-16T07:03:45Z

Also if you really want a custom .clang-tidy you can use the LegayClangTidy module which doesn't so any autogeneration of config

silverjam · 2022-04-16T07:16:57Z

To get around the git thing we just add .clang-tidy to .gitignore in other repos since it fixes the issue without breaking other workflows. A proper solution would be to use a linter where a config file can be passed on the command line

There seemed to be a command line option to pass in a config in the parallel clang-tidy script.

silverjam · 2022-04-16T07:18:42Z

Also if you really want a custom .clang-tidy you can use the LegayClangTidy module which doesn't so any autogeneration of config

Why do we need to generate the .clang-tidy? Just to have it the same across all repos?

I think we could turn off the auto generate and still use this module (but check in a copy of the generated .clang-tidy). If there's a benefit to doing it that way?

woodfell · 2022-04-16T08:58:15Z

That's what used to happen but there were constant problems where someone would make a change to .clang-tidy in one repo but not bother updating any of the other ones. It caused issues where something like (say) LSNP would pass just fine but there would be some syntax in a header file that would only cause a lint error by the time it made it up to starling. Synchronisation which doesn't require manual steps was the major goal of revamping this module a few months ago.

(of course if we can flatten everything down this becomes much less of a problem.....)

woodfell · 2022-04-16T08:59:01Z

To get around the git thing we just add .clang-tidy to .gitignore in other repos since it fixes the issue without breaking other workflows. A proper solution would be to use a linter where a config file can be passed on the command line

There seemed to be a command line option to pass in a config in the parallel clang-tidy script.

There isn't such an option for plain old clang-tidy. Not sure how clangd behaves but I have a feeling it just does the same recursive search upwards as clang-tidy

silverjam · 2022-04-16T15:09:07Z

@woodfell Thanks for clarifying the state of play here, ideally we'd like to get libswiftnav where it doesn't modify itself during compilation, do you have a recommendation here?

jungleraptor

Maybe this is over-complicating things but it's possible we could add an optional parameter to the clang tidy module that allows the user to specify the directory it gets written to (defaults to CMAKE_SOURCE_DIR).

We add an option to libswiftnav (i.e. -DCLANG_TIDY_DIR) that defaults to CMAKE_SOURCE_DIR, so that we don't break local dev workflows.

Then in libswiftnav-rs we could add a CMakeLists.txt that overrides this to CMAKE_BINARY_DIR, (or as a cmake arg in the rust build scripts).

silverjam · 2022-04-19T19:10:26Z

Maybe this is over-complicating things but it's possible we could add an optional parameter to the clang tidy module that allows the user to specify the directory it gets written to (defaults to CMAKE_SOURCE_DIR).

We add an option to libswiftnav (i.e. -DCLANG_TIDY_DIR) that defaults to CMAKE_SOURCE_DIR, so that we don't break local dev workflows.

Then in libswiftnav-rs we could add a CMakeLists.txt that overrides this to CMAKE_BINARY_DIR, (or as a cmake arg in the rust build scripts).

I was thinking we could take the approach we've taken in other places with generated content that has to live "in tree", which is to require that changes/updates must be intentionally staged and committed.

That is:

cmake module generates a clang-tidy config, then runs git diff to ensure nothing has changed
build fails if generated config is different than what's checked in (e.g. git diff returns a non-zero exit code)

So, if clang-tidy was updated, this would require all bumps to the cmake module to include an update to the generated clang-tidy in each of those repos, but if these updates are infrequent it, this doesn't seem like a super horrible idea.

We could also optionally enable this behavior (only for repos that care about this behavior)-- so the "generate and git diff" dance would only happen if the .clang-tidy is not git ignored (see stackoverflow for a possible implementation). This way we could turn on this behavior for the libswiftnav-private repo, and not for everything else.

@isaactorz @woodfell thoughts?

woodfell · 2022-04-19T21:21:29Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

silverjam · 2022-04-19T21:55:15Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

It triggers this check when attempting to release/package swiftnav-rs: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815

Example:

Compiling swiftnav-sys v0.7.2-alpha.0 (/Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0)
    Finished dev [unoptimized + debuginfo] target(s) in 40.27s
error: failed to verify package tarball

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: /Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0/src/libswiftnav/.clang-tidy

  To proceed despite this, pass the `--no-verify` flag.

jungleraptor · 2022-04-19T22:08:15Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

It triggers this check when attempting to release/package swiftnav-rs: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815

Example:
Compiling swiftnav-sys v0.7.2-alpha.0 (/Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0)
    Finished dev [unoptimized + debuginfo] target(s) in 40.27s
error: failed to verify package tarball

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: /Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0/src/libswiftnav/.clang-tidy

  To proceed despite this, pass the `--no-verify` flag.

But we can get around this by making the directory the clang-tidy file gets written to variable (my suggestion above).

That would be a lot more lightweight change to implement to this module than your suggestion above. We also wouldn't need to patch every repo using this module every time we make a change to it.

Are there any other big advantages for having the .clang-tidy file checked in that would make the downsides worth it?

woodfell · 2022-04-19T22:12:30Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

It triggers this check when attempting to release/package swiftnav-rs: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815

Example:
Compiling swiftnav-sys v0.7.2-alpha.0 (/Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0)
    Finished dev [unoptimized + debuginfo] target(s) in 40.27s
error: failed to verify package tarball

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: /Users/joseph.angelo/repos/swiftnav-rs/target/package/swiftnav-sys-0.7.2-alpha.0/src/libswiftnav/.clang-tidy

  To proceed despite this, pass the `--no-verify` flag.

I see.

Another way could be to add a command line flag to complete disable the ClangTidy module. We then use than flag from swiftnav-rs so that nothing gets generated in any location at all. Presumably we're not going to miss the clang tidy targets in this build?

silverjam · 2022-04-19T22:13:50Z

But we can get around this by making the directory the clang-tidy file gets written to variable (my suggestion above).

That would be a lot more lightweight change to implement to this module than your suggestion above. We also wouldn't need to patch every repo using this module every time we make a change to it.

Yeah, the simplicity of your suggestion is a big upside. However, I don't think we should build this change without the "explicit enable" part, that is, you'd be required to check-in a copy of .clang-tidy before it would do the git diff check.

Are there any other big advantages for having the .clang-tidy file checked in that would make the downsides worth it?

The benefit is just the "out of the box" factor, if anyone else is consuming the repo, they don't need to know about our cmake stuff to prevent it from modifying the source tree.

silverjam · 2022-04-19T22:16:24Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

It triggers this check when attempting to release/package swiftnav-rs: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815
[..]

I see.

Another way could be to add a command line flag to complete disable the ClangTidy module. We then use than flag from swiftnav-rs so that nothing gets generated in any location at all. Presumably we're not going to miss the clang tidy targets in this build?

Yeah, that would work too (don't need clang-tidy targets for a wrapped build).

This reverts commit 8dfb955.

silverjam · 2022-04-19T23:43:11Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

It triggers this check when attempting to release/package swiftnav-rs: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815
[..]

I see.
Another way could be to add a command line flag to complete disable the ClangTidy module. We then use than flag from swiftnav-rs so that nothing gets generated in any location at all. Presumably we're not going to miss the clang tidy targets in this build?

Yeah, that would work too (don't need clang-tidy targets for a wrapped build).

I ported over the ability to disable the clang-tidy targets from the legacy module (see https://github.com/swift-nav/cmake/blob/master/OldClangTidy.cmake#L206).

silverjam · 2022-04-19T23:57:01Z

Yeah that sounds like it would work. But what's the problem with using .gitignore here? As far as I know it's been working alright so far?

It triggers this check when attempting to release/package swiftnav-rs: https://github.com/rust-lang/cargo/blob/50d52ebf0ef91f392f47b4d9b9e954bb25c60aca/src/cargo/ops/cargo_package.rs#L815
[..]

I see.
Another way could be to add a command line flag to complete disable the ClangTidy module. We then use than flag from swiftnav-rs so that nothing gets generated in any location at all. Presumably we're not going to miss the clang tidy targets in this build?

Yeah, that would work too (don't need clang-tidy targets for a wrapped build).

I ported over the ability to disable the clang-tidy targets from the legacy module (see https://github.com/swift-nav/cmake/blob/master/OldClangTidy.cmake#L206).

So, this is probably the best option, seems this is already something we were using in this module: https://github.com/swift-nav/swiftnav-rs/blob/master/swiftnav-sys/build.rs#L20 -- this was probably broken when the new clang-tidy module was introduced.

jungleraptor · 2022-04-20T16:28:11Z

ClangTidy.cmake

+  # Global clang-tidy enable option, influences the default project specific enable option
+  option(ENABLE_CLANG_TIDY "Enable auto-linting of code using clang-tidy globally" ON)
+  if(NOT ENABLE_CLANG_TIDY)
+    early_exit(STATUS "auto-linting is disabled globally")


Early exit is a macro that's not defined in this module (defined in OldClangTidy.cmake).

Might want to test these changes in a branch of libswiftnav.

I did test this and it seemed to work?

Ah ok I see why it didn't break.

I guess macros get global scope once they're defined. This works because it's picking up the version of this macro that's defined in ClangFormat.cmake, because that module gets included before hand (bit like relying on a transitive include in C/C++).

It's a nit but these things break sometimes when shuffling around build files and then it's a pain to track them down after the fact.

The best thing would be to define these common helper macros and functions in a common file, but for now it would be sufficient to just define the macro in the places it gets used.

ClangTidy.cmake

Co-authored-by: Isaac Torres <[email protected]>

sonarqubecloud · 2022-04-20T17:33:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

jungleraptor

LGTM

woodfell

LGTM!

write clang-tidy to build dir

8dfb955

silverjam mentioned this pull request Apr 16, 2022

ci: don't generate clang-tidy swift-nav/libswiftnav#24

Merged

silverjam requested review from jungleraptor, jbangelo and woodfell April 16, 2022 04:38

jungleraptor reviewed Apr 18, 2022

View reviewed changes

Jason Mobarak added 2 commits April 19, 2022 16:27

port clang-tidy enable/disable from legacy module

fe45c2c

Revert "write clang-tidy to build dir"

35975f8

This reverts commit 8dfb955.

silverjam changed the title ~~write clang-tidy to build dir~~ clang-tidy: allow target to be disabled Apr 19, 2022

update docs

1a3414d

jungleraptor reviewed Apr 20, 2022

View reviewed changes

ClangTidy.cmake Outdated Show resolved Hide resolved

jungleraptor reviewed Apr 20, 2022

View reviewed changes

ClangTidy.cmake Outdated Show resolved Hide resolved

Update ClangTidy.cmake

87b9c9b

Co-authored-by: Isaac Torres <[email protected]>

define early_exit macro

7faa0fb

jungleraptor approved these changes Apr 20, 2022

View reviewed changes

woodfell approved these changes Apr 20, 2022

View reviewed changes

silverjam merged commit 31604e7 into master Apr 20, 2022

silverjam deleted the silverjam/clang-tidy-config-in-build branch April 20, 2022 21:59

swiftnav-svc-jenkins mentioned this pull request Dec 20, 2022

Auto submodule update: cmake - Only set sanitizer suppression file if sanitzing is enabled swift-nav/albatross#389

Closed

swiftnav-svc-jenkins mentioned this pull request Feb 13, 2023

Auto submodule update: cmake - Exclude unit test header files from coverage reporting (#145) swift-nav/albatross#406

Merged

swiftnav-svc-jenkins mentioned this pull request Jan 4, 2024

Auto submodule update: cmake - ASIO C++ and ASIO Grpc libraries (#170) swift-nav/albatross#472

Closed

swiftnav-svc-jenkins mentioned this pull request Jan 24, 2024

Auto submodule update: cmake - Handle new suitesparse version (#171) swift-nav/albatross#474

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang-tidy: allow target to be disabled #109

clang-tidy: allow target to be disabled #109

silverjam commented Apr 16, 2022 •

edited

Loading

woodfell commented Apr 16, 2022

woodfell commented Apr 16, 2022

silverjam commented Apr 16, 2022

silverjam commented Apr 16, 2022

woodfell commented Apr 16, 2022 •

edited

Loading

woodfell commented Apr 16, 2022

silverjam commented Apr 16, 2022

jungleraptor left a comment •

edited

Loading

silverjam commented Apr 19, 2022

woodfell commented Apr 19, 2022

silverjam commented Apr 19, 2022

jungleraptor commented Apr 19, 2022

woodfell commented Apr 19, 2022

silverjam commented Apr 19, 2022

silverjam commented Apr 19, 2022 •

edited

Loading

silverjam commented Apr 19, 2022

silverjam commented Apr 19, 2022

jungleraptor Apr 20, 2022

silverjam Apr 20, 2022

jungleraptor Apr 20, 2022

sonarqubecloud bot commented Apr 20, 2022

jungleraptor left a comment

woodfell left a comment

clang-tidy: allow target to be disabled #109

clang-tidy: allow target to be disabled #109

Conversation

silverjam commented Apr 16, 2022 • edited Loading

woodfell commented Apr 16, 2022

woodfell commented Apr 16, 2022

silverjam commented Apr 16, 2022

silverjam commented Apr 16, 2022

woodfell commented Apr 16, 2022 • edited Loading

woodfell commented Apr 16, 2022

silverjam commented Apr 16, 2022

jungleraptor left a comment • edited Loading

Choose a reason for hiding this comment

silverjam commented Apr 19, 2022

woodfell commented Apr 19, 2022

silverjam commented Apr 19, 2022

jungleraptor commented Apr 19, 2022

woodfell commented Apr 19, 2022

silverjam commented Apr 19, 2022

silverjam commented Apr 19, 2022 • edited Loading

silverjam commented Apr 19, 2022

silverjam commented Apr 19, 2022

jungleraptor Apr 20, 2022

Choose a reason for hiding this comment

silverjam Apr 20, 2022

Choose a reason for hiding this comment

jungleraptor Apr 20, 2022

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 20, 2022

jungleraptor left a comment

Choose a reason for hiding this comment

woodfell left a comment

Choose a reason for hiding this comment

silverjam commented Apr 16, 2022 •

edited

Loading

woodfell commented Apr 16, 2022 •

edited

Loading

jungleraptor left a comment •

edited

Loading

silverjam commented Apr 19, 2022 •

edited

Loading