ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run #9045

MarcoGorelli · 2020-12-30T11:48:03Z

Previously, cmake format wasn't running (likely because there were two entry keys, so the command getting overridden) - furthermore, it was unnecessarily slow as it didn't take advantage of pre-commit's own machinery

github-actions · 2020-12-30T11:56:21Z

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

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

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

.pre-commit-config.yaml

kou · 2020-12-30T21:45:30Z

.pre-commit-config.yaml

-        entry: bash -c "pip install cmake-format && python run-cmake-format.py --check"
-        entry: echo
-        files: ^(.*/CMakeLists.txt|.*.cmake)$
+        entry: cmake-format --in-place --autosort=false


Why do you use raw cmake-format instead of our run-cmake-format.py wrapper?
We have configuration in the wrapper.

Hi @kou

It does the same thing. If you do it this way, then pre-commit will figure out which files to run it on, and it'll be much, much faster:

$ time pre-commit run cmake-format --all-files [WARNING] Unexpected key(s) present on git://github.com/pre-commit/pre-commit-hooks: sha CMake Format.............................................................Passed real 0m4.472s user 0m18.011s sys 0m0.259s $ time python run-cmake-format.py WARNING config_util.py:307: The following configuration options were ignored: max_subargs_per_line real 0m10.620s user 0m10.594s sys 0m0.017s

As far as I can tell, then only configuration in the wrapper is --in-place --autosort=false - have I missed anything else?

Also, like this, if you enable pre-commit (pre-commit install) then this'll run only on files you've staged, rather than on the wholecodebase each time (as it would if using run-cmake-format.py). You can still choose to run it on the whole codebase though, with --all-files (as in the example i posted above)

I couldn't reproduce the difference on my environment:

$ time pre-commit run cmake-format --all-files [WARNING] Unexpected key(s) present on git://github.com/pre-commit/pre-commit-hooks: sha CMake Format.............................................................Passed real 0m3.476s user 0m13.000s sys 0m0.297s $ time python3 run-cmake-format.py real 0m1.358s user 0m1.308s sys 0m0.046s

🤔 not sure why that would be, but point taken. Though there would still be a difference if you run it as a git commit hook (i.e. if you've done pre-commit install in your environment) because like this it'll only run on staged files, while the Python script runs on all of them each time, so this'll give faster feedback to devs

It seems that pre-commit runs cmake-format for each target file.
But run-cmake-format.py runs one cmake-format for all target files.

We don't want to reformat imported CMake files: https://github.com/apache/arrow/blob/master/run-cmake-format.py#L28-L29
( @xhochy Do you still want to avoid reformatting imported CMake files? The comment was added by you at 79c93c7. )
Could you use run-cmake-format.py to keep the configuration for it in one place?

BTW, we can't maintain the target list by current allow list style. The patterns is out of date. It still has renamed files. Can we change the target list to deny list style? Here is a list of imported CMake files:

cpp/cmake_modules/FindNumPy.cmake

cpp/cmake_modules/FindPythonLibsNew.cmake

cpp/cmake_modules/UseCython.cmake (Oh... This is already reformatted...)

If run-cmake-format.py accepts a target file like run-cmake-format.py cpp/CMakeLists.txt and it processes only the given file, can we remove the performance difference between run-cmake-format.py and cmake-format?

sure, will get back to this by the end of the will and will open a jira ticket as instructed

you could use pass_filenames: false in pre-commit so that it'll just run python run-cmake-format.py, like this configuration will all be there

It seems that pre-commit runs cmake-format for each target file.

I don't think so, it should just run cmake-format file1 file2 ... filen

@xhochy Do you still want to avoid reformatting imported CMake files?

Yes, this helps with diffing against their upstream. We did do changes to them for our use case and this makes updating them simpler.

kou · 2020-12-31T21:41:35Z

.pre-commit-config.yaml

+        entry: cmake-format --in-place --autosort=false
+        files: CMakeLists\.txt$|^cpp/cmake_modules/
+        additional_dependencies:
+          - cmake_format==0.6.13


Why do you specify 0.6.13?
We're using 0.5.2 as I said at #9045 (comment) .

Anyway, we can't merge this until we fix the following issues:

ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run #9045 (comment)

https://github.com/apache/arrow/pull/9045/checks?check_run_id=1629598956

Could you fix them?

sorry, I glossed over that, fixed now

Anyway, we can't merge this until we fix the following issues:

* [#9045 (comment)](https://github.com/apache/arrow/pull/9045#issuecomment-752475426) * https://github.com/apache/arrow/pull/9045/checks?check_run_id=1629598956

Could you fix them?

Thanks for fixing the latter.
Could you also fix the former?

kou · 2020-12-31T21:58:17Z

.pre-commit-config.yaml

-        entry: bash -c "pip install cmake-format && python run-cmake-format.py --check"
-        entry: echo
-        files: ^(.*/CMakeLists.txt|.*.cmake)$
+        entry: cmake-format --in-place --autosort=false


I couldn't reproduce the difference on my environment:

$ time pre-commit run cmake-format --all-files [WARNING] Unexpected key(s) present on git://github.com/pre-commit/pre-commit-hooks: sha CMake Format.............................................................Passed real 0m3.476s user 0m13.000s sys 0m0.297s $ time python3 run-cmake-format.py real 0m1.358s user 0m1.308s sys 0m0.046s

github-actions · 2021-01-08T21:45:00Z

https://issues.apache.org/jira/browse/ARROW-11180

kou

Could you check my comments?

Could you also revert .cmake changes in the exclude list?

run-cmake-format.py

MarcoGorelli · 2021-03-21T12:54:19Z

closing, but feel free to let me know if you're still interested in this and I'll pick it back up (though it doesn't look like this repo is making a big use of pre-commit anyway)

kou · 2021-03-21T21:00:40Z

Sorry. I didn't notice re-review request.
I'll review this.

kou · 2021-03-22T22:15:22Z

@MarcoGorelli I found some problems:

python run-cmake-format.py (no paths arguments) doesn't work because PATTERNS data aren't suitable for glob().
We also need exclude cpp/src/arrow/util/config.h.cmake because it's not a CMake file. (It may be better that we rename the .cmake extension of the file.)

I've fixed them and rebased on the current master.

Could you check this? If it's OK to you, I'll merge this.

MarcoGorelli · 2021-03-30T17:48:47Z

Thanks for updating - looks good, thanks!

kou · 2021-03-30T23:52:05Z

Thanks for confirming this.
I'll merge this.

Previously, cmake format wasn't running (likely because there were two `entry` keys, so the command getting overridden) - furthermore, it was unnecessarily slow as it didn't take advantage of pre-commit's own machinery Closes apache#9045 from MarcoGorelli/cmakelint Lead-authored-by: Marco Gorelli <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: FlightRPC Component: Gandiva Component: C++ Component: Go Component: Java Component: Python Component: Parquet labels Dec 30, 2020

kou reviewed Dec 30, 2020

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

kou reviewed Dec 30, 2020

View reviewed changes

MarcoGorelli force-pushed the cmakelint branch from cb28835 to c56dc12 Compare December 31, 2020 08:32

kou requested changes Dec 31, 2020

View reviewed changes

MarcoGorelli marked this pull request as draft January 7, 2021 18:18

MarcoGorelli changed the title ~~Fixup pre-commit-config.yaml so that cmake format runs~~ ARROW-11180: [CI] cmake-format pre-commit hook doesn't run Jan 8, 2021

MarcoGorelli marked this pull request as ready for review January 8, 2021 08:03

kou changed the title ~~ARROW-11180: [CI] cmake-format pre-commit hook doesn't run~~ ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run Jan 8, 2021

kou reviewed Jan 8, 2021

View reviewed changes

run-cmake-format.py Outdated Show resolved Hide resolved

run-cmake-format.py Show resolved Hide resolved

run-cmake-format.py Outdated Show resolved Hide resolved

MarcoGorelli force-pushed the cmakelint branch 5 times, most recently from 507d580 to 63c9814 Compare January 24, 2021 09:33

kszucs force-pushed the master branch from d238279 to 437c8c9 Compare January 25, 2021 21:53

MarcoGorelli force-pushed the cmakelint branch 2 times, most recently from bee4016 to 9e647f1 Compare January 31, 2021 13:38

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

MarcoGorelli force-pushed the cmakelint branch from ee35ba7 to f79b8c5 Compare February 14, 2021 15:50

MarcoGorelli requested a review from kou February 14, 2021 15:51

MarcoGorelli closed this Mar 21, 2021

kou reopened this Mar 21, 2021

MarcoGorelli and others added 4 commits March 23, 2021 06:45

fixup pre-commit cmake-format hook

73a2374

Sort alphabetically

895df6d

Fix a bug that target CMake files aren't listed without paths arguments

73ddc60

Format

1e01dd9

kou force-pushed the cmakelint branch from b004bc6 to 1e01dd9 Compare March 22, 2021 22:07

kou closed this in 356630b Mar 30, 2021

MarcoGorelli deleted the cmakelint branch March 31, 2021 18:13

kou mentioned this pull request Jun 23, 2021

ARROW-8459: [Dev][Archery] Use a more recent cmake-format #10571

Closed

5 tasks

asfimport mentioned this pull request Mar 30, 2021

[Developer] cmake-format pre-commit hook doesn't run #27085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run #9045

ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run #9045

MarcoGorelli commented Dec 30, 2020

github-actions bot commented Dec 30, 2020

kou Dec 30, 2020

MarcoGorelli Dec 31, 2020

MarcoGorelli Dec 31, 2020

kou Dec 31, 2020

MarcoGorelli Jan 1, 2021

kou Jan 2, 2021

kou Jan 2, 2021

MarcoGorelli Jan 7, 2021

MarcoGorelli Jan 8, 2021

xhochy Jan 8, 2021

kou Dec 31, 2020

MarcoGorelli Jan 1, 2021

kou Jan 2, 2021

kou Dec 31, 2020

github-actions bot commented Jan 8, 2021

kou left a comment

MarcoGorelli commented Mar 21, 2021

kou commented Mar 21, 2021

kou commented Mar 22, 2021

MarcoGorelli commented Mar 30, 2021

kou commented Mar 30, 2021

ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run #9045

ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run #9045

Conversation

MarcoGorelli commented Dec 30, 2020

github-actions bot commented Dec 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2021

kou left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Mar 21, 2021

kou commented Mar 21, 2021

kou commented Mar 22, 2021

MarcoGorelli commented Mar 30, 2021

kou commented Mar 30, 2021