Add GitHub cpp-linter-action

[wgtmac]

[wgtmac]

Is this something worth adding? @Fokko @raulcd @zhjwpku

[zhjwpku]

Is this something worth adding? @Fokko @raulcd @zhjwpku

+1

Is there something wrong with github? I see you are putting cpp-linter in pre-commit.yml while pre-commit in cpp-linter.yml

[wgtmac]

Oops, my bad. I just copied pre-commit.yml and edited the wrong file. Now it is fixed :)

[Fokko]

I like it, and I think it is worth a try 🙌

[lidavidm]

Hmm, in that screenshot, 'memory not found' seems like a spurious error? Will this be noisy?

[wgtmac]

Hmm, in that screenshot, 'memory not found' seems like a spurious error? Will this be noisy?

I have also noticed that. I need to learn more about its configuration and improve it gradually.

[raulcd]

I think we can add it and see if it's helpful. If we find too many spurious errors we can try to fine tune it or remove it if not helpful.
Maybe we want to add extra-args: '-std=c++20 -Wall'?
https://cpp-linter.github.io/cpp-linter-action/inputs-outputs/#extra-args

[wgtmac]

https://github.com/apache/iceberg-cpp/actions/runs/12544655200

cpp-linter/[email protected] is not allowed to be used in apache/iceberg-cpp.

[raulcd]

cpp-linter/[email protected] is not allowed to be used in apache/iceberg-cpp.

Thanks for trying!

[wgtmac]

@raulcd It seems that this issue still exists.

[raulcd]

I've been able to reproduce the memory error locally:

$ clang-tidy-18 -p ../iceberg-cpp -checks=file  --format-style file ../iceberg-cpp/api/iceberg/table.h --extra-arg=-std=c++17 --enable-module-headers-parsing 
Error while trying to load a compilation database:
Could not auto-detect compilation database from directory "../iceberg-cpp"
No compilation database found in /home/raulcd/code/arrow/../iceberg-cpp or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
/home/raulcd/code/arrow/.clang-tidy:30:1: error: unknown key 'AnalyzeTemporaryDtors'
AnalyzeTemporaryDtors: true
^~~~~~~~~~~~~~~~~~~~~
Error parsing /home/raulcd/code/arrow/.clang-tidy: Invalid argument
/home/raulcd/code/arrow/.clang-tidy:30:1: error: unknown key 'AnalyzeTemporaryDtors'
AnalyzeTemporaryDtors: true
^~~~~~~~~~~~~~~~~~~~~
Error parsing /home/raulcd/code/arrow/.clang-tidy: Invalid argument
/home/raulcd/code/arrow/.clang-tidy:30:1: error: unknown key 'AnalyzeTemporaryDtors'
AnalyzeTemporaryDtors: true
^~~~~~~~~~~~~~~~~~~~~
Error parsing /home/raulcd/code/arrow/.clang-tidy: Invalid argument
/home/raulcd/code/arrow/.clang-tidy:30:1: error: unknown key 'AnalyzeTemporaryDtors'
AnalyzeTemporaryDtors: true
^~~~~~~~~~~~~~~~~~~~~
Error parsing /home/raulcd/code/arrow/.clang-tidy: Invalid argument
2 errors generated.
Error while processing /home/raulcd/code/arrow/../iceberg-cpp/api/iceberg/table.h.
error: invalid argument '-std=c++17' not allowed with 'C' [clang-diagnostic-error]
/home/raulcd/code/arrow/../iceberg-cpp/api/iceberg/table.h:22:10: error: 'memory' file not found [clang-diagnostic-error]
   22 | #include <memory>
      |          ^~~~~~~~
Found compiler error(s).

We might need to generate a compilation database first and use the database attribute: https://cpp-linter.github.io/cpp-linter-action/inputs-outputs/#database. It seems like it is detecting as if it's a C project by default without it.
If I build and use the build folder with -p it is successful:

$ clang-tidy-18 -p /home/raulcd/code/iceberg-cpp/build -checks=file  --format-style file ../api/iceberg/table.h --extra-arg=-std=c++17 --enable-module-headers-parsing

[wgtmac]

I have inserted a build step before the cpp-linter to create the compile_commands.json. Now it seems working.

[lidavidm]

It appears either the token doesn't have comment permissions or the token isn't being passed into the action

[wgtmac]

It appears either the token doesn't have comment permissions or the token isn't being passed into the action

I have added permission for pull-requests: write but still hit the following error:

ERROR:CPP Linter:response returned 403 from POST https://api.github.com/repos/apache/iceberg-cpp/issues/20/comments with message: {"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/issues/comments#create-an-issue-comment","status":"403"}
  Traceback (most recent call last):
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/v2.13.3/venv/bin/cpp-linter", line 8, in <module>
      sys.exit(main())
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/v2.13.3/venv/lib/python3.10/site-packages/cpp_linter/__init__.py", line 81, in main
      rest_api_client.post_feedback(files=files, args=args, clang_versions=clang_versions)
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/v2.13.3/venv/lib/python3.10/site-packages/cpp_linter/rest_api/github_api.py", line 264, in post_feedback
      self.update_comment(
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/v2.13.3/venv/lib/python3.10/site-packages/cpp_linter/rest_api/github_api.py", line [361](https://github.com/apache/iceberg-cpp/actions/runs/12554533849/job/35003344629?pr=20#step:4:369), in update_comment
      self.api_request(url=comments_url, method=req_meth, data=payload)
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/v2.13.3/venv/lib/python3.10/site-packages/cpp_linter/rest_api/__init__.py", line 124, in api_request
      response.raise_for_status()
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/v2.13.3/venv/lib/python3.10/site-packages/requests/models.py", line 1024, in raise_for_status
      raise HTTPError(http_error_msg, response=self)
  requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: https://api.github.com/repos/apache/iceberg-cpp/issues/20/comments

[lidavidm]

The token still says only read permissions https://github.com/apache/iceberg-cpp/actions/runs/12554533849/job/35003344629?pr=20#step:1:17

[lidavidm]

It seems since the PR comes from a fork, Github will never let you get anything but read access. https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

I think you'd either have to somehow make this work with pull_request_target (can be tricky) or just give up on the comment functionality.

[lidavidm]

Note that pull_request_target (1) only runs from the base branch, not the PR and (2) doesn't check out the PR HEAD (it gets the base branch) so it's not suitable for this by default. (You can manually check out the PR HEAD but it gets easy to have accidental vulnerabilities this way)

[wgtmac]

Thanks @lidavidm for providing the detail! After reading relevant documentation, I agree that it is really tricky to workaround the token permission. Without comments on PR, the cpp-linter action seems not that useful because we already have the pre-commit check to do similar things.

Is it possible to elevate permission on a specific token? @Fokko

[lidavidm]

I think it's a security issue: the write permission doesn't discriminate between who opened the PR or where you can write to, so someone could open a malicious PR to write to the repository.

pre-commit only runs clang-format, not clang-tidy - I think it'd still be useful even without the PR comment?

[wgtmac]

pre-commit only runs clang-format, not clang-tidy - I think it'd still be useful even without the PR comment?

That makes sense! Let me enable it for now. We can improve or discard it after we have experience with more PRs.

[wgtmac]

I think this PR is ready to merge. @Fokko @Xuanwo

[Xuanwo]

Thank you @wgtmac for this. This changes look nice to me, I will merge it to unblock the future developement.