Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Add a clang-tidy build #3001

Merged
merged 32 commits into from
Jul 17, 2024
Merged

Conversation

msiddhu
Copy link
Contributor

@msiddhu msiddhu commented Jul 11, 2024

Fixes #2051

Changes

Problems

Currently installing gRPC is eating up a lot of time. Open for suggestions to optimize this.

Note

The build contains features which are in currently in preview. Should modify the workflow file when the feature is released as stable.

Remaining Work:

  • Add more checks and resolve the warnings via multiple cleanups.
  • Compare the warnings count of main branch and the current PR.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.66%. Comparing base (497eaf4) to head (ba3a6d8).
Report is 103 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3001      +/-   ##
==========================================
+ Coverage   87.12%   87.66%   +0.55%     
==========================================
  Files         200      190      -10     
  Lines        6109     5858     -251     
==========================================
- Hits         5322     5135     -187     
+ Misses        787      723      -64     

see 121 files with indirect coverage changes

@msiddhu msiddhu marked this pull request as ready for review July 11, 2024 18:53
@msiddhu msiddhu requested a review from a team July 11, 2024 18:53
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding clang-tidy to the CI.

See some comments.

.github/workflows/clang-tidy.yaml Outdated Show resolved Hide resolved
.github/workflows/clang-tidy.yaml Outdated Show resolved Hide resolved
tools/clang-tidy.sh Outdated Show resolved Hide resolved
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taking shape.

Somehow the CI log do not show clang-tidy warnings, this needs some troubleshooting.

Make sure it works properly locally first, to validate CMAKE_CXX_CLANG_TIDY works.

.github/workflows/clang-tidy.yaml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@msiddhu
Copy link
Contributor Author

msiddhu commented Jul 15, 2024

A still have a problem. I'm not able to suppress the third_party. Please help me with this if you know anyway. Or could revert back to original approach without using the -DCMAKE_CXX_CLANG_TIDY .

I have tried different combinations of regex but the clang-tidy is still not suppressing the warnings.

Even, this is taking longer to run in CI

@msiddhu msiddhu requested a review from marcalff July 15, 2024 14:22
@marcalff
Copy link
Member

A still have a problem. I'm not able to suppress the third_party. Please help me with this if you know anyway. Or could revert back to original approach without using the -DCMAKE_CXX_CLANG_TIDY .

I have tried different combinations of regex but the clang-tidy is still not suppressing the warnings.

Even, this is taking longer to run in CI

See this code in the top level CMakeList.txt:

if(WITH_PROMETHEUS)
  find_package(prometheus-cpp CONFIG QUIET)
  if(NOT prometheus-cpp_FOUND)
    message(STATUS "Trying to use local prometheus-cpp from submodule")
    if(EXISTS ${PROJECT_SOURCE_DIR}/third_party/prometheus-cpp/.git)
      set(SAVED_ENABLE_TESTING ${ENABLE_TESTING})
      set(ENABLE_TESTING OFF)
      add_subdirectory(third_party/prometheus-cpp)
      set(ENABLE_TESTING ${SAVED_ENABLE_TESTING})
    else()

The same way the prometheus-cpp sub directory is build with ENABLE_TESTING = OFF, there should be a way to build that sub directory without clang-tidy, as in:

      set(CMAKE_CXX_CLANG_TIDY "")

Save and restore the setting, same as SAVED_ENABLE_TESTING.

@marcalff marcalff self-assigned this Jul 15, 2024
@msiddhu
Copy link
Contributor Author

msiddhu commented Jul 16, 2024

Thanks alot for the help. @marcalff

  • Added the set(CMAKE_CXX_CLANG_TIDY "") in required third_parties.
  • Enabled some more checks. Currently the warnings are 542. and takes 18 min to run the check.
  • In the next cleanups we could adjust the configuration file to get our correct requirements.

Thanks.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the CI

@marcalff marcalff changed the title [CI] clang-tidy in GitHub Workflow [CI] Add a clang-tidy build Jul 17, 2024
@marcalff marcalff merged commit aac2b77 into open-telemetry:main Jul 17, 2024
52 checks passed
@lalitb
Copy link
Member

lalitb commented Jul 17, 2024

Currently installing gRPC is eating up a lot of time. Open for suggestions to optimize this.

The bazel-windows build, which take ~ 30mins, provide enough buffer for CI tasks that are slower but still relatively fast. It's also worth considering using Alpine Linux, as it includes the gRPC package, and the opentelemetry-demo is already utilizing it.

@msiddhu
Copy link
Contributor Author

msiddhu commented Jul 17, 2024

Thanks @lalitb. I'll check this and include the OTLP_GRPC in the next clean up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Add a clang-tidy build
3 participants