-
Notifications
You must be signed in to change notification settings - Fork 37
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: Combine linter/style/tidy/portability checks #520
Conversation
Codecov Report
@@ Coverage Diff @@
## master #520 +/- ##
=======================================
Coverage 99.69% 99.69%
=======================================
Files 54 54
Lines 17322 17322
=======================================
Hits 17270 17270
Misses 52 52 |
@@ -17,7 +14,7 @@ executors: | |||
- image: ethereum/cpp-build-env:14-gcc-10 | |||
linux-clang-latest: | |||
docker: | |||
- image: ethereum/cpp-build-env:14-clang-10 | |||
- image: ethereum/cpp-build-env:15-clang-10 |
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.
What is the update in 15? Why not update the others for consistency too?
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 only updated this one to include clang-format
.
@@ -304,13 +304,13 @@ jobs: | |||
sanitizers-macos: | |||
executor: macos | |||
environment: | |||
BUILD_TYPE: RelWithDebInfo | |||
CMAKE_OPTIONS: -DENABLE_ASSERTIONS=ON -DSANITIZE=address,undefined,nullability,implicit-unsigned-integer-truncation,implicit-signed-integer-truncation | |||
UBSAN_OPTIONS: halt_on_error=1 |
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.
This is not a build, but runtime parameter, hence not move under build
settings?
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.
It must be the environment variable anyway (libubsan checks it), so this way of setting it is fine.
@@ -440,7 +441,6 @@ workflows: | |||
requires: | |||
- sanitizers-macos | |||
- coverage | |||
- cxx20 |
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.
You did not remove the cxx20
task above. Should it be kept?
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.
Should be removed.
This moves clang-tidy checking and C++20 build to "lint" job.
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.
Looks good to me, just remove the stray cxx20 task.
This moves clang-tidy checking and C++20 build to "lint" job.
The
-Weverything
build will be added to "lint" too in #508.