cpp-linter-action icon indicating copy to clipboard operation
cpp-linter-action copied to clipboard

Lines-changed-only seems not to work for clang-tidy

Open regit opened this issue 8 months ago • 4 comments

What events trigger your workflow?

on:
  pull_request:
    branches: [master]
    paths: ['**.c', '**.h', '**.am']
  push:
    paths: ['**.c', '**.h', '**.am']

What OS does your workflow use?

runs-on: ubuntu-latest
container: ubuntu:22.04

How is cpp-linter-action configured?

- uses: cpp-linter/cpp-linter-action@v2
        id: linter
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        with:
          style: '' # Empty for no format check
          tidy-checks: '' # Use .clang-tidy config file. 
          version: ${{ env.LLVM_VERSION }}
          # only 'update' a single comment in a pull request's thread. 
          thread-comments: ${{ github.event_name == 'pull_request' && 'update' }}
          lines-changed-only: true
          tidy-review: true
          passive-reviews: true

What was the unexpected behavior?

I'm testing the action and it seems to detect correctly the line changed in the file I did update to trigger some error.

The job is running the command:

/usr/bin/clang-tidy-14 --line-filter=[{"name": "src/source-af-packet.c", "lines": [[1, 2], [932, 935], [940, 943]]}] --extra-arg= --fix-errors src/source-af-packet.c

which returns no warning despite the introduction of invalid code in these places.

When I run a derivated same command on my system:

clang-tidy --line-filter='[{"name": "src/source-af-packet.c", "lines": [[1, 2], [932, 935], [940, 943]]}]' --extra-arg= src/source-af-packet.c

I got no error too but if I do

clang-tidy --line-filter='[{"name": "source-af-packet.c", "lines": [[1, 2], [932, 935], [940, 943]]}]' --extra-arg= src/source-af-packet.c

then I got the correct behavior.

Is there an option to skip the source directory in the action ?

Failing job url: https://github.com/regit/suricata/actions/runs/14709331910/job/41277429460

regit avatar Apr 28 '25 14:04 regit

[!tip]

  1. clang-tidy always works better when you give it a compile_commands.json (AKA compilation database).
  2. cpp-linter works better with source paths and clang-tidy when the compilation database's path is explicitly defined via our database option.

Peculiar observations

It is odd that the line-filter uses a path different from the path to the analyzed source (clang-tidy --line-filter='[{"name": "source-af-packet.c", ... src/source-af-packet.c). If you're implicitly using a compilation data base (letting clang-tidy auto-detect one), then the src/ part of the path might be getting excluded there which would explain why your customized command works.

It is also weird that --extra-arg is passed to clang-tidy without a value. I think this is a bug somehow.

2bndy5 avatar Apr 28 '25 20:04 2bndy5

Tip

  1. clang-tidy always works better when you give it a compile_commands.json (AKA compilation database).
  2. cpp-linter works better with source paths and clang-tidy when the compilation database's path is explicitly defined via our database option.

Peculiar observations

It is odd that the line-filter uses a path different from the path to the analyzed source (clang-tidy --line-filter='[{"name": "source-af-packet.c", ... src/source-af-packet.c). If you're implicitly using a compilation data base (letting clang-tidy auto-detect one), then the src/ part of the path might be getting excluded there which would explain why your customized command works.

It is also weird that --extra-arg is passed to clang-tidy without a value. I think this is a bug somehow.

Yes, I'm using a compilation database and the src directory is indeed not present in the file definition

{ "directory": "/home/eric/git/suricata/src", "file": "source-af-packet.c", "output": "source-af-packet.o", "arguments": "lot of things"}

I'm responsible of the generation of the compile_commands.json (via a Makefile target). I can see if I can tune that and get it work both in editor and in the clang-tidy command.

regit avatar Apr 28 '25 20:04 regit

Is there an option to skip the source directory in the action ?

If everything needed (including compilation database) is located in src/ then you could tell cpp-linter to cd src with

- uses: cpp-linter/cpp-linter-action@v2
  with:
    repo-root: src

or using github actions syntax

- uses: cpp-linter/cpp-linter-action@v2
  working-directory: src

2bndy5 avatar Apr 28 '25 20:04 2bndy5

Oh. Looks like repo-root does not behave well with lines-changed-only. I see the new run has warnings when looking for the changed files returned from GH REST API.

WARNING:CPP Linter:Could not find src/app-layer-ftp.c! Did you checkout the repo?

2bndy5 avatar Apr 28 '25 21:04 2bndy5

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jun 28 '25 02:06 github-actions[bot]

Oh. Looks like repo-root does not behave well with lines-changed-only. I see the new run has warnings when looking for the changed files returned from GH REST API.

Began tracking this upstream in cpp-linter/cpp-linter#146

2bndy5 avatar Jun 28 '25 03:06 2bndy5

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Aug 29 '25 02:08 github-actions[bot]