Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Add CMake format check #5941

Merged
merged 3 commits into from
Jun 14, 2024
Merged

[CI] Add CMake format check #5941

merged 3 commits into from
Jun 14, 2024

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented May 31, 2024

What changes were proposed in this pull request?

  1. Use cmake-format to check cmake files, including CMakeLists.txt & *.cmake.
  2. Fix the format issues.
  3. Refactor license check.

Example to use the format tool:

apt install python3-pip -y
pip3 install --user cmake-format
cmake-format --first-comment-is-literal True --in-place cpp/velox/CMakeLists.txt

Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@PHILO-HE PHILO-HE force-pushed the cmake-format branch 11 times, most recently from 66d216c to 5e2459d Compare June 3, 2024 06:23
Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

1 similar comment
Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

1 similar comment
Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

@PHILO-HE PHILO-HE changed the title [WIP] Add CMake format checker [CI] Add CMake format check Jun 3, 2024
@PHILO-HE PHILO-HE marked this pull request as ready for review June 3, 2024 07:56
Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jun 3, 2024

Run Gluten Clickhouse CI

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Jun 3, 2024

I tried to put code format check for cpp/cpp-ch together and then will remove ch_code_style.yml. But it's strange that if we do that (i.e., by removing 200e183), clang-format will report some error for some CH code.
See https://github.com/apache/incubator-gluten/actions/runs/9346082625/job/25720197384.

And I tried to use clang-format-15 in local (e.g., clang-format-15 -style=file -i cpp-ch/local-engine/Parser/AggregateFunctionParser.h), same format issue is reported. @liuneng1994, could you take a look?

dev/check.sh ${{github.event.pull_request.base.sha}}
git config --global --add safe.directory $GITHUB_WORKSPACE
cd $GITHUB_WORKSPACE/
fileList=$(find ./cpp ./cpp-ch -name CMakeLists.txt -o -name *.cmake)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zzcclp, I'm adding to CI job to format CMake code (CMakeLists.txt & *.cmake), which also covers cpp-ch module. Please take a look to see if it's ok to you. Thanks!

@zzcclp
Copy link
Contributor

zzcclp commented Jun 4, 2024

@liuneng1994 @baibaichen please help to review and check, thanks.

ulysses-you
ulysses-you previously approved these changes Jun 4, 2024
Copy link
Contributor

@ulysses-you ulysses-you left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

github-actions bot commented Jun 7, 2024

Run Gluten Clickhouse CI

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Jun 7, 2024

@liuneng1994, do you have any comment?

@liuneng1994
Copy link
Contributor

@liuneng1994, do you have any comment?

There is generally no problem, but subjectively, the effect of cmake-format seems to make some places more messy.

liuneng1994
liuneng1994 previously approved these changes Jun 11, 2024
Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

@PHILO-HE PHILO-HE merged commit f1bb1d6 into apache:main Jun 14, 2024
8 of 9 checks passed
@GlutenPerfBot
Copy link
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query log/native_5941_time.csv log/native_master_06_13_2024_142cf0fbc_time.csv difference percentage
q1 37.02 35.57 -1.454 96.07%
q2 23.57 23.78 0.208 100.88%
q3 38.90 39.61 0.706 101.82%
q4 31.53 32.76 1.234 103.91%
q5 71.12 67.54 -3.582 94.96%
q6 6.92 9.42 2.493 136.02%
q7 80.40 80.69 0.298 100.37%
q8 86.33 84.77 -1.557 98.20%
q9 123.10 120.70 -2.397 98.05%
q10 46.44 45.25 -1.182 97.46%
q11 19.84 19.79 -0.058 99.71%
q12 29.47 27.92 -1.555 94.72%
q13 37.04 39.43 2.390 106.45%
q14 22.36 18.98 -3.378 84.89%
q15 29.27 33.04 3.772 112.89%
q16 13.84 13.85 0.008 100.06%
q17 102.71 101.57 -1.139 98.89%
q18 148.31 144.97 -3.339 97.75%
q19 13.61 13.79 0.185 101.36%
q20 28.57 26.68 -1.894 93.37%
q21 256.94 260.83 3.888 101.51%
q22 12.04 13.91 1.876 115.59%
total 1259.33 1254.85 -4.478 99.64%

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.

6 participants