test: migrate linter cppcheck from bash to python and follow-up improvements #6876
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis PR makes two source-level changes and updates lint tooling: it makes two NetInfoEntry constructors explicit to prevent implicit conversions; it fixes word clearing, loop variable scoping, and bit-counting logic in CMnemonic::Check; it narrows lint discovery to Python files, removes a bash-based cppcheck runner, and adds a new Python-based cppcheck runner with filtering, caching, and deterministic behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 Ruff (0.13.3)test/lint/lint-cppcheck-dash.py77-77: Starting a process with a partial executable path (S607) 85-85: (S603) 85-85: Consider Replace with (RUF005) 100-122: Consider iterable unpacking instead of concatenation Replace with iterable unpacking (RUF005) 124-124: (S603) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/netinfo.h (1)
1-1: Resolve the merge conflict before merging.The pipeline indicates a merge conflict in this file that must be resolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/evo/netinfo.h(1 hunks)src/wallet/bip39.cpp(2 hunks)test/lint/all-lint.py(1 hunks)test/lint/lint-cppcheck-dash.py(1 hunks)test/lint/lint-cppcheck-dash.sh(0 hunks)
💤 Files with no reviewable changes (1)
- test/lint/lint-cppcheck-dash.sh
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/netinfo.hsrc/wallet/bip39.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/netinfo.h
🧠 Learnings (1)
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/netinfo.h
🪛 GitHub Actions: Check Potential Conflicts
src/evo/netinfo.h
[error] 1-1: Merge conflict detected in file 'src/evo/netinfo.h'.
🪛 Ruff (0.13.3)
test/lint/lint-cppcheck-dash.py
77-77: Starting a process with a partial executable path
(S607)
85-85: subprocess call: check for execution of untrusted input
(S603)
85-85: Consider ['git', 'ls-files', '--', *patterns] instead of concatenation
Replace with ['git', 'ls-files', '--', *patterns]
(RUF005)
100-122: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
124-124: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (3)
src/evo/netinfo.h (1)
123-123: LGTM: Prevents implicit conversions.Adding
explicitto theCServiceconstructor is a best practice that prevents unintended implicit conversions fromCServicetoNetInfoEntry.src/wallet/bip39.cpp (2)
103-103: LGTM: Efficient string clearing.Using
resize(0)instead of assignment avoids deallocating and reallocating memory, which is more efficient forSecureString.
117-122: LGTM: Correct bit counting logic.The changes improve code quality:
- Scoping
kito the loop is better practice- Incrementing
nBitsCountinside the loop is necessary to correctly count each of the 11 bits per word- The logic correctly processes
nWordCount * 11bits total (validated at line 128)
| dependencies_output = subprocess.run( | ||
| cppcheck_cmd, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| text=True, | ||
| ) | ||
|
|
||
| unique_sorted_lines = sorted(set(dependencies_output.stdout.splitlines())) | ||
| for line in unique_sorted_lines: | ||
| if re.search(enabled_regexp, line) and not re.search(ignored_regexp, line) and re.search(files_regexp, line): | ||
| warnings.append(line) | ||
|
|
||
| if warnings: | ||
| print('\n'.join(warnings)) | ||
| print() | ||
| print("Advice not applicable in this specific case? Add an exception by updating") | ||
| print(f"IGNORED_WARNINGS in {__file__}") | ||
| # Uncomment to enforce the linter / comment to run locally | ||
| exit_code = 1 | ||
|
|
There was a problem hiding this comment.
Propagate cppcheck execution failures
Right now we ignore cppcheck’s exit status. If it dies due to a configuration or invocation error (missing include paths, bad arguments, etc.), the run still reports success as long as no warning matches our filters. That can silently mask broken lint runs. Capture the non-zero return code and bail out immediately so the CI fails when cppcheck itself fails.
dependencies_output = subprocess.run(
cppcheck_cmd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True,
)
+ if dependencies_output.returncode != 0:
+ print(dependencies_output.stdout, end='')
+ sys.exit(dependencies_output.returncode)
+
unique_sorted_lines = sorted(set(dependencies_output.stdout.splitlines()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dependencies_output = subprocess.run( | |
| cppcheck_cmd, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.STDOUT, | |
| text=True, | |
| ) | |
| unique_sorted_lines = sorted(set(dependencies_output.stdout.splitlines())) | |
| for line in unique_sorted_lines: | |
| if re.search(enabled_regexp, line) and not re.search(ignored_regexp, line) and re.search(files_regexp, line): | |
| warnings.append(line) | |
| if warnings: | |
| print('\n'.join(warnings)) | |
| print() | |
| print("Advice not applicable in this specific case? Add an exception by updating") | |
| print(f"IGNORED_WARNINGS in {__file__}") | |
| # Uncomment to enforce the linter / comment to run locally | |
| exit_code = 1 | |
| dependencies_output = subprocess.run( | |
| cppcheck_cmd, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.STDOUT, | |
| text=True, | |
| ) | |
| if dependencies_output.returncode != 0: | |
| print(dependencies_output.stdout, end='') | |
| sys.exit(dependencies_output.returncode) | |
| unique_sorted_lines = sorted(set(dependencies_output.stdout.splitlines())) | |
| for line in unique_sorted_lines: | |
| if re.search(enabled_regexp, line) and not re.search(ignored_regexp, line) and re.search(files_regexp, line): | |
| warnings.append(line) | |
| if warnings: | |
| print('\n'.join(warnings)) | |
| print() | |
| print("Advice not applicable in this specific case? Add an exception by updating") | |
| print(f"IGNORED_WARNINGS in {__file__}") | |
| # Uncomment to enforce the linter / comment to run locally | |
| exit_code = 1 |
🧰 Tools
🪛 Ruff (0.13.3)
124-124: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
In test/lint/lint-cppcheck-dash.py around lines 124 to 143, the script currently
ignores cppcheck's exit status which can hide execution/configuration failures;
update the code to check dependencies_output.returncode immediately after
subprocess.run and if it's non-zero print dependencies_output.stdout (or a clear
error message including that output) and exit with a non-zero status (e.g.,
sys.exit(dependencies_output.returncode) or exit(1)); do this before processing
warnings so CI fails when cppcheck itself fails.
|
This pull request has conflicts, please rebase. |
…) for secure string
…uns all tests twice) BACKPORT NOTE: only left-over changes has not been done previously is replacing a mask "lint-*" to "lint-*.py" and it's done f26a496 test: clean up all-lint.py (Martin Leitner-Ankerl) 64d72c4 test: rename lint-all.py to all-lint.py (Martin Leitner-Ankerl) Pull request description: When running `./test/lint/lint-all.py`, the script runs all tests but also calls itself because the comparison with `__file__` doesn't work. Comparing resolved paths gives reliable comparison, and lint-all.py doesn't call itself any more ACKs for top commit: laanwj: Code review ACK f26a496 Tree-SHA512: b44abdd685f7b48a6a9f48e96d97138b635c31c1c7ab543cb5636b5f49690ccd56fa6fec01ae7fcc16af01a613372ee77632f70c32059919b373aa8051953791
| '-DCOPYRIGHT_YEAR', | ||
| '-DDEBUG', | ||
| '-DUSE_EPOLL', | ||
| '-DCHAR_BIT=8', |
There was a problem hiding this comment.
Consideration for followup, should we also define ENABLE_STACKTRACES and platform-specific macros like ENABLE_SSSE3, ENABLE_SSE41, ENABLE_X86_AESNI, ENABLE_ARM_AES, ENABLE_ARM_NEON to expand coverage?
There was a problem hiding this comment.
Consider to create follow-up PR, this PR is about replacing bash to python and some related necessary changes
There was a problem hiding this comment.
Btw, ENABLE_SSE41, ENABLE_X86_AESNI, ENABLE_ARM_AES will probably make a difference for src/crypto/x11 subdirectory which is kind of 3rd party code and not a subject for cpplint.
Though, ENABLE_STACKTRACES - could be useful
There was a problem hiding this comment.
Accelerated routines aren't third-party code as we are maintaining it but we can defer this decision to after some more accelerated backends are implemented
3 participants
Issue being fixed or feature implemented
It's the only linter that still uses bash script instead python.
What was done?
cppcheckHow Has This Been Tested?
Run standalone linter:
test/lint/lint-cppcheck-dash.pyRun all linters:
test/lint/all-lint.pyOn CI it seems running https://github.com/dashpay/dash/actions/runs/18279286751/job/52038431836?pr=6876:
Breaking Changes
N/A
Checklist: