You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/30 11:48:13 UTC

[GitHub] [arrow] MarcoGorelli opened a new pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

MarcoGorelli opened a new pull request #9045:
URL: https://github.com/apache/arrow/pull/9045


   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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-757012526


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550925148



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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
+        files: CMakeLists\.txt$|^cpp/cmake_modules/
+        additional_dependencies:
+          - cmake_format==0.6.13

Review comment:
       > 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?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550428589



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       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)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: ARROW-11180: [CI] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r553795616



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       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`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550744765



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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
+        files: CMakeLists\.txt$|^cpp/cmake_modules/
+        additional_dependencies:
+          - cmake_format==0.6.13

Review comment:
       sorry, I glossed over that, fixed now




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli closed pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
MarcoGorelli closed pull request #9045:
URL: https://github.com/apache/arrow/pull/9045


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-810455739


   Thanks for updating - looks good, thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550925323



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] xhochy commented on a change in pull request #9045: ARROW-11180: [CI] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
xhochy commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r553939866



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       @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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550428220



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       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?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou closed pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
kou closed pull request #9045:
URL: https://github.com/apache/arrow/pull/9045


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-752475426


   <!--
     Licensed to the Apache Software Foundation (ASF) under one
     or more contributor license agreements.  See the NOTICE file
     distributed with this work for additional information
     regarding copyright ownership.  The ASF licenses this file
     to you under the Apache License, Version 2.0 (the
     "License"); you may not use this file except in compliance
     with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
     Unless required by applicable law or agreed to in writing,
     software distributed under the License is distributed on an
     "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
     KIND, either express or implied.  See the License for the
     specific language governing permissions and limitations
     under the License.
   -->
   
   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:
   
     * [Other pull requests](https://github.com/apache/arrow/pulls/)
     * [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: ARROW-11180: [CI] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r553795616



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       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`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
kou commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-810651397


   Thanks for confirming this.
   I'll merge this.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-757012526


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550695044



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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
+        files: CMakeLists\.txt$|^cpp/cmake_modules/
+        additional_dependencies:
+          - cmake_format==0.6.13

Review comment:
       Why do you specify `0.6.13`?
   We're using 0.5.2 as I said at https://github.com/apache/arrow/pull/9045#discussion_r550335310 .
   
   Anyway, we can't merge this until we fix the following issues:
   
     * 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?

##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       I couldn't reproduce the difference on my environment:
   
   ```console
   $ 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
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550745036



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       :thinking: 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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
kou commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-804431484


   @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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r554201859



##########
File path: run-cmake-format.py
##########
@@ -21,58 +21,20 @@
 import pathlib
 import subprocess
 import sys
+import argparse
+from pathlib import Path

Review comment:
       Do wee need them?

##########
File path: run-cmake-format.py
##########
@@ -21,58 +21,20 @@
 import pathlib
 import subprocess
 import sys
+import argparse
+from pathlib import Path
 
 
-patterns = [
-    'cpp/CMakeLists.txt',
-    # Keep an explicit list of files to format as we don't want to reformat
-    # files we imported from other location.
-    'cpp/cmake_modules/BuildUtils.cmake',
-    'cpp/cmake_modules/DefineOptions.cmake',
-    'cpp/cmake_modules/FindArrow.cmake',
-    'cpp/cmake_modules/FindArrowCUDA.cmake',
-    'cpp/cmake_modules/FindArrowDataset.cmake',
-    'cpp/cmake_modules/FindArrowFlight.cmake',
-    'cpp/cmake_modules/FindArrowFlightTesting.cmake',
-    'cpp/cmake_modules/FindArrowPython.cmake',
-    'cpp/cmake_modules/FindArrowPythonFlight.cmake',
-    'cpp/cmake_modules/FindArrowTesting.cmake',
-    'cpp/cmake_modules/FindBrotli.cmake',
-    'cpp/cmake_modules/FindClangTools.cmake',
-    'cpp/cmake_modules/FindFlatbuffersAlt.cmake',
-    'cpp/cmake_modules/FindGLOG.cmake',
-    'cpp/cmake_modules/FindGandiva.cmake',
-    'cpp/cmake_modules/FindInferTools.cmake',
-    'cpp/cmake_modules/FindLLVMAlt.cmake',
-    'cpp/cmake_modules/FindLz4.cmake',
-    'cpp/cmake_modules/FindParquet.cmake',
-    'cpp/cmake_modules/FindPlasma.cmake',
-    'cpp/cmake_modules/FindPython3Alt.cmake',
-    'cpp/cmake_modules/FindRE2.cmake',
-    'cpp/cmake_modules/FindRapidJSONAlt.cmake',
-    'cpp/cmake_modules/FindSnappyAlt.cmake',
-    'cpp/cmake_modules/FindThrift.cmake',
-    'cpp/cmake_modules/FindZSTD.cmake',
-    'cpp/cmake_modules/Findc-aresAlt.cmake',
-    'cpp/cmake_modules/FindgRPCAlt.cmake',
-    'cpp/cmake_modules/FindgflagsAlt.cmake',
-    'cpp/cmake_modules/Findjemalloc.cmake',
-    'cpp/cmake_modules/SetupCxxFlags.cmake',
-    'cpp/cmake_modules/ThirdpartyToolchain.cmake',
-    'cpp/cmake_modules/san-config.cmake',
-    'cpp/cmake_modules/UseCython.cmake',
-    'cpp/src/**/CMakeLists.txt',
-    'cpp/tools/**/CMakeLists.txt',
-    'java/gandiva/CMakeLists.txt',
-    'python/CMakeLists.txt',
-]
+PATTERNS = ['cpp/cmake_modules/*.cmake', '*CMakeLists.txt']
+EXCLUDE = ['cpp/cmake_modules/FindNumPy.cmake',
+'cpp/cmake_modules/FindPythonLibsNew.cmake', 'cpp/cmake_modules/UseCython.cmake']

Review comment:
       ```suggestion
   EXCLUDE = [
       'cpp/cmake_modules/FindNumPy.cmake',
       'cpp/cmake_modules/FindPythonLibsNew.cmake',
       'cpp/cmake_modules/UseCython.cmake',
   ]
   ```
   
   This style will be easy to add/remove items.

##########
File path: run-cmake-format.py
##########
@@ -21,58 +21,20 @@
 import pathlib
 import subprocess
 import sys
+import argparse
+from pathlib import Path
 
 
-patterns = [
-    'cpp/CMakeLists.txt',
-    # Keep an explicit list of files to format as we don't want to reformat
-    # files we imported from other location.

Review comment:
       Could you keep this comment to show why we have the list?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550926812



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       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 79c93c77542524042e26761c5107127f51e5c193. )
   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`?
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r554201859



##########
File path: run-cmake-format.py
##########
@@ -21,58 +21,20 @@
 import pathlib
 import subprocess
 import sys
+import argparse
+from pathlib import Path

Review comment:
       Do wee need them?

##########
File path: run-cmake-format.py
##########
@@ -21,58 +21,20 @@
 import pathlib
 import subprocess
 import sys
+import argparse
+from pathlib import Path
 
 
-patterns = [
-    'cpp/CMakeLists.txt',
-    # Keep an explicit list of files to format as we don't want to reformat
-    # files we imported from other location.
-    'cpp/cmake_modules/BuildUtils.cmake',
-    'cpp/cmake_modules/DefineOptions.cmake',
-    'cpp/cmake_modules/FindArrow.cmake',
-    'cpp/cmake_modules/FindArrowCUDA.cmake',
-    'cpp/cmake_modules/FindArrowDataset.cmake',
-    'cpp/cmake_modules/FindArrowFlight.cmake',
-    'cpp/cmake_modules/FindArrowFlightTesting.cmake',
-    'cpp/cmake_modules/FindArrowPython.cmake',
-    'cpp/cmake_modules/FindArrowPythonFlight.cmake',
-    'cpp/cmake_modules/FindArrowTesting.cmake',
-    'cpp/cmake_modules/FindBrotli.cmake',
-    'cpp/cmake_modules/FindClangTools.cmake',
-    'cpp/cmake_modules/FindFlatbuffersAlt.cmake',
-    'cpp/cmake_modules/FindGLOG.cmake',
-    'cpp/cmake_modules/FindGandiva.cmake',
-    'cpp/cmake_modules/FindInferTools.cmake',
-    'cpp/cmake_modules/FindLLVMAlt.cmake',
-    'cpp/cmake_modules/FindLz4.cmake',
-    'cpp/cmake_modules/FindParquet.cmake',
-    'cpp/cmake_modules/FindPlasma.cmake',
-    'cpp/cmake_modules/FindPython3Alt.cmake',
-    'cpp/cmake_modules/FindRE2.cmake',
-    'cpp/cmake_modules/FindRapidJSONAlt.cmake',
-    'cpp/cmake_modules/FindSnappyAlt.cmake',
-    'cpp/cmake_modules/FindThrift.cmake',
-    'cpp/cmake_modules/FindZSTD.cmake',
-    'cpp/cmake_modules/Findc-aresAlt.cmake',
-    'cpp/cmake_modules/FindgRPCAlt.cmake',
-    'cpp/cmake_modules/FindgflagsAlt.cmake',
-    'cpp/cmake_modules/Findjemalloc.cmake',
-    'cpp/cmake_modules/SetupCxxFlags.cmake',
-    'cpp/cmake_modules/ThirdpartyToolchain.cmake',
-    'cpp/cmake_modules/san-config.cmake',
-    'cpp/cmake_modules/UseCython.cmake',
-    'cpp/src/**/CMakeLists.txt',
-    'cpp/tools/**/CMakeLists.txt',
-    'java/gandiva/CMakeLists.txt',
-    'python/CMakeLists.txt',
-]
+PATTERNS = ['cpp/cmake_modules/*.cmake', '*CMakeLists.txt']
+EXCLUDE = ['cpp/cmake_modules/FindNumPy.cmake',
+'cpp/cmake_modules/FindPythonLibsNew.cmake', 'cpp/cmake_modules/UseCython.cmake']

Review comment:
       ```suggestion
   EXCLUDE = [
       'cpp/cmake_modules/FindNumPy.cmake',
       'cpp/cmake_modules/FindPythonLibsNew.cmake',
       'cpp/cmake_modules/UseCython.cmake',
   ]
   ```
   
   This style will be easy to add/remove items.

##########
File path: run-cmake-format.py
##########
@@ -21,58 +21,20 @@
 import pathlib
 import subprocess
 import sys
+import argparse
+from pathlib import Path
 
 
-patterns = [
-    'cpp/CMakeLists.txt',
-    # Keep an explicit list of files to format as we don't want to reformat
-    # files we imported from other location.

Review comment:
       Could you keep this comment to show why we have the list?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550335701



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-803573971


   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)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on pull request #9045: ARROW-11180: [Developer] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
kou commented on pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#issuecomment-803659131


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] MarcoGorelli commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
MarcoGorelli commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r553501322



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] kou commented on a change in pull request #9045: Fixup pre-commit-config.yaml so that cmake format runs

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r550335310



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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
+        files: CMakeLists\.txt$|^cpp/cmake_modules/
+        additional_dependencies:
+          - cmake_format

Review comment:
       We need to ensure using cmake-format 0.5.2.
   Can we specify version?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] xhochy commented on a change in pull request #9045: ARROW-11180: [CI] cmake-format pre-commit hook doesn't run

Posted by GitBox <gi...@apache.org>.
xhochy commented on a change in pull request #9045:
URL: https://github.com/apache/arrow/pull/9045#discussion_r553939866



##########
File path: .pre-commit-config.yaml
##########
@@ -40,9 +40,10 @@ repos:
       - id: cmake-format
         name: CMake Format
         language: python
-        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

Review comment:
       @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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org