Introduce repo guardrail and fix all prone bugs (tool detection) #897
+36,948
−27,714
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| Checks: > | ||
| -*, | ||
| bugprone-*, | ||
| -bugprone-easily-swappable-parameters, | ||
| -bugprone-assignment-in-if-condition, | ||
| clang-analyzer-*, | ||
| -clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling, | ||
| -clang-analyzer-security.insecureAPI.strcpy, | ||
| -clang-analyzer-valist.Uninitialized, | ||
| performance-*, | ||
| portability-*, | ||
| misc-* | ||
| WarningsAsErrors: >- | ||
| clang-analyzer-*, | ||
| bugprone-* | ||
| HeaderFilterRegex: '.*' | ||
| AnalyzeTemporaryDtors: false | ||
| FormatStyle: file | ||
| User: covesa | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| #!/bin/bash | ||
|
Contributor
There was a problem hiding this comment. suggestion: You might want to use something like uv + precommit instead of hand building this, it can help you with the pre commit and pre push too.
Contributor
There was a problem hiding this comment. i.e. here is what we are using in opensovd https://github.com/eclipse-opensovd/cicd-workflows/blob/main/.pre-commit-hooks.yaml it will make your life easier and simplify the github stages too. (Linking opensovd only because it's a good example on how to configure pre-commit, you don't have to this exact config)
Collaborator
Author
There was a problem hiding this comment. This much more good reference to me, things can be learnt form other experts.
Contributor
There was a problem hiding this comment. Anytime! you can also reach out to me directly, if you have questions about that or need support :) |
||
| # | ||
| # SPDX-License-Identifier: MPL-2.0 | ||
| # | ||
| # commit-msg hook for dlt-daemon | ||
| # | ||
| # Validates the commit message of the current commit being created. | ||
| # This mirrors the CI commit-check rules so issues are caught before commit. | ||
| # | ||
| # To enable: | ||
| # git config core.hooksPath .githooks | ||
| # | ||
| # To bypass (emergency only): | ||
| # git commit --no-verify | ||
| # | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| NC='\033[0m' | ||
|
|
||
| COMMIT_MSG_FILE="$1" | ||
|
|
||
| print_header() | ||
| { | ||
| echo | ||
| echo "==================================================" | ||
| echo "$1" | ||
| echo "==================================================" | ||
| } | ||
|
|
||
| main() | ||
| { | ||
| print_header "COMMIT MESSAGE CHECK" | ||
|
|
||
| local SUBJECT BODY FULL | ||
| SUBJECT=$(head -1 "${COMMIT_MSG_FILE}") | ||
| BODY=$(sed -n '2,$p' "${COMMIT_MSG_FILE}") | ||
| FULL=$(cat "${COMMIT_MSG_FILE}") | ||
|
|
||
| local FAILED=0 | ||
|
|
||
| # Strip comment lines (lines starting with #) for validation | ||
| local SUBJECT_NOCOMMENT BODY_NOCOMMENT FULL_NOCOMMENT | ||
| SUBJECT_NOCOMMENT=$(grep -v '^#' "${COMMIT_MSG_FILE}" | head -1) | ||
| BODY_NOCOMMENT=$(grep -v '^#' "${COMMIT_MSG_FILE}" | sed -n '2,$p') | ||
| FULL_NOCOMMENT=$(grep -v '^#' "${COMMIT_MSG_FILE}") | ||
|
|
||
| # Subject must not be empty | ||
| if [ -z "${SUBJECT_NOCOMMENT}" ]; then | ||
| echo "Commit subject is empty." | ||
| FAILED=1 | ||
| fi | ||
|
|
||
| # Subject must be <= 72 chars | ||
| if [ ${#SUBJECT_NOCOMMENT} -gt 72 ]; then | ||
| echo "Commit subject exceeds 72 characters:" | ||
| echo " ${SUBJECT_NOCOMMENT}" | ||
| FAILED=1 | ||
| fi | ||
|
|
||
| # No trailing period on subject | ||
| if echo "${SUBJECT_NOCOMMENT}" | grep -qE '\.$'; then | ||
| echo "Commit subject must not end with a period:" | ||
| echo " ${SUBJECT_NOCOMMENT}" | ||
| FAILED=1 | ||
| fi | ||
|
|
||
| # No fixup!/squash! commits | ||
| if echo "${SUBJECT_NOCOMMENT}" | grep -qE '^(fixup|squash)!'; then | ||
| echo "fixup!/squash! commits are not allowed" | ||
| FAILED=1 | ||
| fi | ||
|
|
||
| # Subject must start with an imperative verb | ||
| if ! echo "${SUBJECT_NOCOMMENT}" | grep -qE \ | ||
| '^(Add|Fix|Remove|Update|Refactor|Implement|Improve|Introduce|Rename|Replace|Cleanup|Document|Convert|Move|Enable|Disable)\b'; then | ||
| echo "Commit subject must start with an imperative verb." | ||
| echo " Allowed: Add|Fix|Remove|Update|Refactor|Implement|Improve|" | ||
| echo " Introduce|Rename|Replace|Cleanup|Document|Convert|" | ||
| echo " Move|Enable|Disable" | ||
| echo " Got: ${SUBJECT_NOCOMMENT}" | ||
| FAILED=1 | ||
| fi | ||
|
|
||
| # Must have Signed-off-by (DCO) | ||
| if ! echo "${FULL_NOCOMMENT}" | grep -q '^Signed-off-by:'; then | ||
| echo "Missing Signed-off-by." | ||
| echo " Use: git commit -s" | ||
| FAILED=1 | ||
| fi | ||
|
|
||
| # Blank line between subject and body | ||
| local SECOND_LINE | ||
| SECOND_LINE=$(grep -v '^#' "${COMMIT_MSG_FILE}" | sed -n '2p') | ||
| if [ -n "${SECOND_LINE}" ]; then | ||
| if ! echo "${SECOND_LINE}" | grep -qE '^[[:space:]]*$'; then | ||
| echo "Blank line required between subject and body." | ||
| FAILED=1 | ||
| fi | ||
| fi | ||
|
|
||
| # Body lines must be <= 72 chars | ||
| while IFS= read -r LINE; do | ||
| [ -z "${LINE}" ] && continue | ||
| if [ ${#LINE} -gt 72 ]; then | ||
| echo "Commit body line exceeds 72 characters:" | ||
| echo " ${LINE}" | ||
| FAILED=1 | ||
| fi | ||
| done <<< "${BODY_NOCOMMENT}" | ||
|
|
||
| if [ ${FAILED} -ne 0 ]; then | ||
| echo "" | ||
| echo -e "${RED}FAIL: Commit message validation failed${NC}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo -e "${GREEN}PASS${NC}" | ||
| } | ||
|
|
||
| main | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| #!/bin/bash | ||
| # | ||
| # SPDX-License-Identifier: MPL-2.0 | ||
| # | ||
| # Pre-commit hook for dlt-daemon | ||
| # | ||
| # Runs clang-format and commit-message checks on staged files. | ||
| # This mirrors the CI pipeline so issues are caught before push. | ||
| # | ||
| # To enable: | ||
| # git config core.hooksPath .githooks | ||
| # | ||
| # To bypass (emergency only): | ||
| # git commit --no-verify | ||
| # | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| YELLOW='\033[0;33m' | ||
| NC='\033[0m' | ||
|
|
||
| # Directories excluded from checks (same as check.sh) | ||
| EXCLUDE_PATTERNS=( | ||
| '^src/android/' | ||
| '^src/dlt-qnx-system/' | ||
| '^src/core_dump_handler/' | ||
| '^src/dbus/' | ||
| '^src/kpi/' | ||
| '^src/tests/' | ||
| '^qnx/' | ||
| '^googletest/' | ||
| '^doc/' | ||
| '^build/' | ||
| ) | ||
|
|
||
| EXCLUDE_REGEX="$(IFS='|'; echo "${EXCLUDE_PATTERNS[*]}")" | ||
|
|
||
| print_header() | ||
| { | ||
| echo | ||
| echo "==================================================" | ||
| echo "$1" | ||
| echo "==================================================" | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Collect staged C/C++ files | ||
| # --------------------------------------------------------------------------- | ||
| get_staged_files() | ||
| { | ||
| git diff --cached --name-only --diff-filter=ACM \ | ||
| | grep -E '^(src|include|tests)/.*\.(c|cc|cpp|cxx|h|hh|hpp|hxx)$' \ | ||
| | grep -Ev "${EXCLUDE_REGEX}" \ | ||
| || true | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # clang-format check | ||
| # --------------------------------------------------------------------------- | ||
| run_format_check() | ||
| { | ||
| # Prefer clang-format-16 to match CI (Docker container with LLVM 16) | ||
| local CLANG_FORMAT | ||
| CLANG_FORMAT=$(command -v clang-format-16 || command -v clang-format-18 || command -v clang-format) | ||
|
|
||
| if [ -z "${CLANG_FORMAT}" ]; then | ||
| echo -e "${YELLOW}WARNING: clang-format not found, skipping format check${NC}" | ||
| return 0 | ||
| fi | ||
|
|
||
| print_header "CLANG-FORMAT (staged files)" | ||
|
|
||
| local FILES | ||
| FILES=$(get_staged_files) | ||
|
|
||
| if [ -z "${FILES}" ]; then | ||
| echo "No C/C++ files staged." | ||
| return 0 | ||
| fi | ||
|
|
||
| local FAILED=0 | ||
|
|
||
| while IFS= read -r FILE; do | ||
| [ -z "${FILE}" ] && continue | ||
|
|
||
| echo "Checking ${FILE}" | ||
|
|
||
| if ! "${CLANG_FORMAT}" \ | ||
| --style=file \ | ||
| --dry-run \ | ||
| --Werror \ | ||
| "${FILE}"; then | ||
| FAILED=1 | ||
| fi | ||
| done <<< "${FILES}" | ||
|
|
||
| if [ "${FAILED}" -ne 0 ]; then | ||
| echo "" | ||
| echo -e "${RED}FAIL: One or more files are not formatted${NC}" | ||
| echo "Fix with:" | ||
| echo " ./check.sh fix-format" | ||
| echo "Or for specific files:" | ||
| echo " clang-format -i <file>" | ||
| return 1 | ||
| fi | ||
|
|
||
| echo -e "${GREEN}PASS${NC}" | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # SPDX license header check | ||
| # --------------------------------------------------------------------------- | ||
| run_spdx_check() | ||
| { | ||
| print_header "SPDX LICENSE HEADER (staged files)" | ||
|
|
||
| local FILES | ||
| FILES=$(git diff --cached --name-only --diff-filter=ACM \ | ||
| | grep -E '^(src|include|tests)/.*\.(c|cc|cpp|cxx|h|hh|hpp|hxx)$' \ | ||
| | grep -Ev "${EXCLUDE_REGEX}" \ | ||
| || true) | ||
|
|
||
| if [ -z "${FILES}" ]; then | ||
| echo "No C/C++ files staged." | ||
| return 0 | ||
| fi | ||
|
|
||
| local FAILED=0 | ||
|
|
||
| while IFS= read -r FILE; do | ||
| [ -z "${FILE}" ] && continue | ||
|
|
||
| if ! grep -q 'SPDX-License-Identifier:' "${FILE}"; then | ||
| echo "Missing SPDX-License-Identifier in: ${FILE}" | ||
| FAILED=1 | ||
| fi | ||
| done <<< "${FILES}" | ||
|
|
||
| if [ "${FAILED}" -ne 0 ]; then | ||
| echo "" | ||
| echo -e "${RED}FAIL: Missing SPDX headers${NC}" | ||
| echo "Add the following to the top of each file:" | ||
| echo " /* SPDX-License-Identifier: MPL-2.0 */" | ||
| return 1 | ||
| fi | ||
|
|
||
| echo -e "${GREEN}PASS${NC}" | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Main | ||
| # --------------------------------------------------------------------------- | ||
| main() | ||
| { | ||
| run_format_check | ||
| run_spdx_check | ||
|
|
||
| print_header "PRE-COMMIT" | ||
| echo -e "${GREEN}All checks passed${NC}" | ||
| } | ||
|
|
||
| main |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
suggestion: Give a reason for disabling warnings about insecure APIs. dlt is doing a lot of buffer handling, so doing that safely would be a benefit right?
Same for strcpy it could be replaced with strncpy in (almost?) all cases and prevent a bunch of memory issues. Might be worth at least a follow up?
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.
Thanks for your comments
This is due to the huge workload a reviewer need for this such "WTF" Pull Request from me, so to reduce the painful review for now we temp ok with this disable, and shall apply Annet K secure api like strcpy_s ... or any suitable approach later in other PRs
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.
Probably worth creating a couple of issues for that, so someone can easily volunteer to fix these? It would be a big benefit to the project and is likely something that can be pretty well with AI (at least the first shot, it probably needs some human brains to fix some details then)