-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: update clang-tidy and clang-format from 10 to 12 #33141
Conversation
@jiangyinzuo Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
@jiangyinzuo Please associate the related issue to the body of your Pull Request. (eg. “issue: #”) |
internal/core/run_clang_format.sh
Outdated
@@ -6,8 +6,15 @@ else | |||
fi | |||
CorePath=$1 | |||
|
|||
# Check if clang-format-10 exists | |||
if command -v clang-format-10 >/dev/null 2>&1; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether we can replace all the clang-format-10
with clang-format
in the codebase. So I write a simple check here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #33141 +/- ##
==========================================
- Coverage 81.94% 81.93% -0.01%
==========================================
Files 1015 1015
Lines 130213 130213
==========================================
- Hits 106701 106694 -7
- Misses 19529 19535 +6
- Partials 3983 3984 +1 |
Thanks for your contribution. Not specifying clang format version may lead to unpredictable results. Could you also modify the files as shown in #33173, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration test finds a DATA RACE, which appears to be unrelated to this PR.
|
@czs007 Can you help rerun this? |
@jiangyinzuo Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
-10
suffix for clang-tidy and clang-format in install_deps.sh6ff36df
to
63089a7
Compare
Default llvm toolchain version in Ubuntu 20.04 is 10, while Ubuntu 22.04 does not have `clang-tidy-10` or `clang-format-10` by default. Upgrade clang-format and clang-tidy from 10 to 12, so that both Ubuntu 20.04 and 22.04 are able to run. See milvus-io#33173 issue: milvus-io#33142 Co-authored-by: Patrick Weizhi Xu <[email protected]> Signed-off-by: Patrick Weizhi Xu <[email protected]> Signed-off-by: Yinzuo Jiang <[email protected]>
@PwzXxm @XuanYang-cn @yhmo CI has passed, can we proceed now? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: czs007, jiangyinzuo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Default llvm toolchain version in Ubuntu 20.04 is 10, while Ubuntu 22.04 does not have `clang-tidy-10` or `clang-format-10` by default. issue: milvus-io#33142 Signed-off-by: Patrick Weizhi Xu <[email protected]> Signed-off-by: Yinzuo Jiang <[email protected]>
Default llvm toolchain version in Ubuntu 20.04 is 10, while Ubuntu 22.04 does not have
clang-tidy-10
orclang-format-10
by default.issue: #33142