Skip to content

RDKB-66207 : Add clang-format workflow. - #1305

Open
mateuszCieslak-GL wants to merge 10 commits into
rdkcentral:developfrom
mateuszCieslak-GL:clang_format
Open

RDKB-66207 : Add clang-format workflow.#1305
mateuszCieslak-GL wants to merge 10 commits into
rdkcentral:developfrom
mateuszCieslak-GL:clang_format

Conversation

@mateuszCieslak-GL

@mateuszCieslak-GL mateuszCieslak-GL commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

NOTE: Superseeded by PR #1320
clang-format file is present in the repository, but not used. As a result, code not adhering to coding standards is slipping through the net.
Let's add it to the workflow, observational at first.

It will be run on incoming PRs. Produced output will be copilot-style - in the PR comments will appear with suggested fixes to bring PR in line with coding standards.

Result of it's run will not break the merge requirements.

@mateuszCieslak-GL
mateuszCieslak-GL requested a review from a team as a code owner July 29, 2026 14:42
Copilot AI review requested due to automatic review settings July 29, 2026 14:43
@mateuszCieslak-GL
mateuszCieslak-GL marked this pull request as draft July 29, 2026 14:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a two-stage GitHub Actions workflow to run clang-format on changed C/C header lines in pull requests and post the resulting formatting diffs back to the PR as GitHub “suggestion” review comments (non-blocking).

Changes:

  • Introduces a clang-format PR workflow that runs git-clang-format and uploads a diff artifact when formatting changes are detected.
  • Introduces a clang-format-suggestions workflow that triggers on completion of stage 1, downloads the artifact, converts diffs into PR review suggestions, and posts them via the GitHub API.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/clang-format.yml Stage 1 formatter run on PRs; produces and uploads a diff + PR metadata artifact.
.github/workflows/clang-format-suggestions.yml Stage 2 reviewer; converts the diff artifact into “suggestion” comments and posts a PR review.
Comments suppressed due to low confidence (2)

.github/workflows/clang-format.yml:24

  • Stage 1 posts suggestions against the PR head SHA (recorded in pr-meta.env), but actions/checkout on pull_request defaults to the synthetic merge commit. That mismatch can shift line numbers and cause stage 2 to fail posting suggestions (422) or to target the wrong lines when the base branch has moved. Check out the PR head ref explicitly, and fetch the base branch ref needed for merge-base/diffing.
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0                      # need base history to diff changed lines

.github/workflows/clang-format.yml:33

  • BASE_SHA is set to the base branch tip SHA. For long-lived PRs, that SHA is often not an ancestor of the PR head, so git-clang-format can compute diffs that include unrelated base-branch changes, producing suggestions that don’t correspond to the PR diff (and may trigger 422 in stage 2). Use the merge-base of the PR head and the base branch ref instead.
          BASE_SHA="${{ github.event.pull_request.base.sha }}"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/clang-format.yml Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

.github/workflows/clang-format.yml:12

  • The pull_request path filters use the uncommon patterns **.c / **.h, which are easy to misread and can behave differently than the standard recursive file globs. Using **/*.c and **/*.h is clearer and avoids any globstar edge cases for nested paths.
    paths:
      - '**.c'
      - '**.h'

.github/workflows/clang-format-suggestions.yml:6

  • Trailing whitespace at the end of this comment line can cause noisy diffs/linters in some setups; remove the extra space after the period.
# stage 1 as passive data and posts it as review suggestions. 

@mateuszCieslak-GL
mateuszCieslak-GL marked this pull request as ready for review August 4, 2026 16:30
Copilot AI review requested due to automatic review settings August 4, 2026 16:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

.github/workflows/clang-format.yml:69

  • The workflow triggers on **/*.cpp and the step name says it reformats .c/.cpp/.h, but git-clang-format is restricted to --extensions c,h, so C++ changes will never produce suggestions. Either include C++ extensions here or remove **/*.cpp from the trigger list.
          git-clang-format --style=file --extensions c,h "$BASE_SHA" -- . "${EXCLUDES[@]}"

.github/workflows/clang-format.yml:51

  • git merge-base can fail here even with fetch-depth: 0 because the base SHA may not be present in the local clone when checking out only the PR head SHA. Fetching the base SHA explicitly makes the merge-base computation reliable.
          BASE_SHA="$(git merge-base "${{ github.event.pull_request.base.sha }}" HEAD)" || {
            echo "::error::Could not compute merge base — is fetch-depth: 0 set?"
            exit 1
          }

.github/workflows/clang-format-suggestions.yml:33

  • This comment is missing a closing parenthesis, which makes the sentence hard to read.
    # fire 'completed'). Allow failure too (if stage 1 fails, suggestions are still useful!

Copilot AI review requested due to automatic review settings August 5, 2026 14:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (4)

.github/workflows/clang-format-suggestions.yml:144

  • The example command shown when suggestions exceed the cap uses --extensions c,h, which won’t match the workflow’s .cpp scope once enabled. Keep this guidance consistent with the actual formatter invocation.
                       "> ```\n"
                       "> pip install clang-format==18.1.8\n"
                       "> git-clang-format --style=file --extensions c,h <merge-base>\n"
                       "> ```")

.github/workflows/clang-format.yml:69

  • The workflow triggers on **/*.cpp, but git-clang-format is limited to --extensions c,h, so .cpp changes will never be reformatted/suggested. Include cpp (and any other intended extensions) in --extensions, and update the nearby comment so it stays accurate.
          # --extensions c,h is why nothing but C sources is ever formatted.
          #
          # git-clang-format exit codes: 0 = nothing to do, 1 = it reformatted
          # something (this is NOT an error), 2+ = genuine failure. The default
          # shell is `bash -e`, so a bare call would abort the step on the

.github/workflows/clang-format.yml:26

  • actions/checkout is referenced as @v6, but this repo’s other workflows use actions/checkout@v4 (e.g. .github/workflows/makefile.yml:28, .github/workflows/community_label_by_author.yml:17). Aligning versions reduces maintenance risk and avoids failures if v6 is not available.
      - uses: actions/checkout@v6

.github/workflows/clang-format-suggestions.yml:33

  • Minor typo in the comment: the parenthesis opened in Allow failure too ( isn’t closed, which makes the comment harder to read.

This issue also appears on line 141 of the same file.

    # Exclude cancelled runs at the source (they can leave outdated artifact yet
    # fire 'completed'). Allow failure too (if stage 1 fails, suggestions are still useful!

Copilot AI review requested due to automatic review settings August 5, 2026 15:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

.github/workflows/clang-format.yml:40

  • This step says it reformats only changed .c/.h lines, but the workflow also targets .cpp files (paths and --extensions include cpp). Update the step name to reflect the actual behavior so the workflow remains self-documenting.
      - name: Reformat only the changed .c/.h lines

Comment thread .github/workflows/clang-format-suggestions.yml Outdated
Copilot AI review requested due to automatic review settings August 5, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 5, 2026 16:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

.github/workflows/clang-format.yml:98

  • This workflow uses actions/upload-artifact@v6, but the repo’s workflows generally pin core actions/* dependencies to established major versions (for example actions/checkout@v4 in .github/workflows/makefile.yml:28). To reduce the chance of breakage from an unsupported/unknown major, pin to the same stable major used elsewhere.
        uses: actions/upload-artifact@v6

.github/workflows/clang-format.yml:26

  • This workflow uses actions/checkout@v6, but other workflows in this repo consistently use actions/checkout@v4 (e.g., .github/workflows/community_label_by_author.yml:17, .github/workflows/makefile.yml:28). Using a different (and potentially unsupported) major version here increases the risk of the workflow failing or behaving differently from the rest of the CI setup.

This issue also appears on line 98 of the same file.

      - uses: actions/checkout@v6

.github/workflows/clang-format-suggestions.yml:41

  • This workflow uses actions/download-artifact@v7. The repo’s other workflows pin GitHub-maintained actions/* to stable majors (notably actions/checkout@v4 in .github/workflows/community_label_by_author.yml:17 and .github/workflows/makefile.yml:28). Aligning this to the same stable major reduces the risk of CI breakage due to an unsupported/unknown major version.
        uses: actions/download-artifact@v7

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.

3 participants