[GLUTEN-11027][VL][CI] Add clang-tidy to check cpp code

[xinghuayu007]

What changes are proposed in this pull request?

How was this patch tested?

Related issue: #11027

[zhouyuan]

@xinghuayu007 @PHILO-HE based on the issue description, the target here is to check the CPP code format? We already have a format checker with clang-format here, is there a gap in this existing tool?
https://github.com/apache/incubator-gluten/blob/main/.github/workflows/code_format.yml

[xinghuayu007]

@xinghuayu007 @PHILO-HE based on the issue description, the target here is to check the CPP code format? We already have a format checker with clang-format here, is there a gap in this existing tool? https://github.com/apache/incubator-gluten/blob/main/.github/workflows/code_format.yml

@zhouyuan , there are different. 'clang-format' focuses solely on code appearance and layout, like automatically adjusts indentation, line breaks, spacing, brace positioning, alignment, etc. 'clang-tidy' focuses on code quality, logic, and potential errors, like checks for potential bugs, suggests using more modern C++ features.

[PHILO-HE]

Sorry for delayed response. Some comments. Please confirm if they make sense. Thanks.

[PHILO-HE]

It seems redundant to run the build for native code. Maybe, we can use the following action to reuse the binary produced in the main workflow. Please verify it.

- name: Download artifact from in-progress workflow
  uses: dawidd6/action-download-artifact@v3
  with:
    github_token: ${{secrets.GITHUB_TOKEN}}
    workflow: velox_backend_x86.yml
    name: velox-native-lib-centos-7-${{ github.sha }}
    commit: ${{ github.sha }}
    workflow_conclusion: ""
    check_artifacts: true
    search_artifacts: true
    if_no_artifact_found: warn

[xinghuayu007]

Good ideal.

[PHILO-HE]

If dawidd6/action-download-artifact requires that the base workflow must complete, I think it is acceptable, since our goal is to apply the clang tidy check and the time by clang tidy may not be too much if only the files changed by PR are checked.

Also note an existing workflow is using this action.
https://github.com/apache/incubator-gluten/blob/4feac9bba366e032c7ad50fccc603d6fe3c29f8f/.github/workflows/test_report.yml#L34

[xinghuayu007]

It is possible to run 'clang tidy check' workflow after the completion of the whole 'Velox Backend (x86)' workflow to reuse the cache of 'build-native-lib-centos-7'. But I face another problem, that there can not exist two trigger on the workflow 'clang tidy check' like the follow:

`name: Clang Tidy Check

on:
pull_request:
paths:
- '.github/workflows/cpp_clang_tidy.yml'
- 'cpp//*.cc'
- 'cpp//*.h'
workflow_run:
workflows: ["Velox Backend (x86)"]
types:
- completed`

[PHILO-HE]

Then, we may have to add a check step to determine if this clang-tidy workflow needs to be skipped. Please confirm it. Thanks.

- name: Check for specific path changes
   id: filter
   uses: tj-actions/changed-files@v45
   with:
   files: |
        cpp/**/*.cc
        cpp/**/*.h

- name: Download artifact
   # Only run this step if the files we care about were changed
   if: steps.filter.outputs.any_changed == 'true'
   uses: dawidd6/action-download-artifact@v3
   xxxx

See the link for tj-actions/changed-files: https://github.com/tj-actions/changed-files

[xinghuayu007]

I found another problem: https://github.com/apache/incubator-gluten/actions/runs/20520437287

The action tj-actions/changed-files@v45 is not allowed in apache/incubator-gluten because all actions must be from a repository owned by your enterprise

I will use another method to implement this action.

[PHILO-HE]

@xinghuayu007, you can choose another version verified by GitHub Marketplace, e.g., v46.

https://github.com/marketplace/actions/changed-files

[PHILO-HE]

Maybe, we have to switch to use dorny/paths-filter, which is allowed by Apache infrastructure.

https://github.com/apache/infrastructure-actions/blob/b18408d79fe0ee2d3aedef6032b59aaf68ec87ca/actions.yml#L304

[xinghuayu007]

ok

[zhouyuan]

if cpp/build/compile_commands.json is required could we add this to some existing job?

[xinghuayu007]

You mean all jobs in CI/CD generates 'cpp/build/compile_commands.json'?

[PHILO-HE]

Some comments. Thanks.

[PHILO-HE]

With workflow_run, it seems this new workflow will be triggered after merged to main branch.

[PHILO-HE]

@xinghuayu007, is this PR ready? I assume we can merge this first version, then create a follow-up PR to see if it works, and fix possible issues there.

[xinghuayu007]

@xinghuayu007, is this PR ready? I assume we can merge this first version, then create a follow-up PR to see if it works, and fix possible issues there.

It is ready. Please merge it.

[PHILO-HE]

Looks good. cc @zhouyuan

[baibaichen]

This PR did not run Spark 41 unit tests and is now breaking the CI. I will fix it in #11380