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
ARROW-4363: [CI] [C++] Add CMake format checks #3606
Conversation
- Add a top-level script that reformats all known CMake files in the source tree - Add a lint step in Travis that checks CMake files are properly formatted Requires [cmake_format](https://github.com/cheshirekow/cmake_format) and Python 3.
This is nice! Can you add instructions to https://github.com/apache/arrow/blob/master/cpp/README.md#continuous-integration about this new hurdle to a clean CI build? BTW I'm going to port cpp/README.md to Sphinx this week with any luck, but I will do that later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM
Thanks! I really like that we also have a consitent formatting for CMake now.
We should put this into the |
Hmm... how do I run that docker image and where are the executed steps? |
See https://github.com/apache/arrow/blob/master/docker-compose.yml#L189 Can we please document the docker-compose setup? https://issues.apache.org/jira/browse/ARROW-3364 |
Ok, I've tried to update the docker-compose setup, but there are too many things to fix right now (most notably, it wasn't switched to clang 7.0 and there's a weird hack for iwyu). |
Argh. I wish I had noticed that this changed the casing on all usages of functions in BuildUtils.cmake. You can pass |
I like lower case for all function definitions and calls. Should we mix cases for function definitions and calls? It seems that @wesm uses a rule that we use upper case for functions defined by us and lower case for other functions. (I'm not sure this is right.) |
Well, I'd be OK with making everything lower-case if you all would like that |
I don't mind either way. But whatever the rule, it should remain simple to express ;-) |
I'm happy with everything as long as it's automated ;) On the other side, I prefer to just use the default settings when using automated formatters. Thus the formatting is normally consistent how others also use it. |
I'll also admit the uppercase provides a nice visual cue for Arrow-specific functions. |
- Add a top-level script that reformats all known CMake files in the source tree - Add a lint step in Travis that checks CMake files are properly formatted Requires [cmake_format](https://github.com/cheshirekow/cmake_format) and Python 3. Author: Antoine Pitrou <antoine@python.org> Closes apache#3606 from pitrou/ARROW-4363-cmake-format and squashes the following commits: 762df29 <Antoine Pitrou> Add mention in cpp/README.md 9073af9 <Antoine Pitrou> ARROW-4363: Add CMake format checks
- Add a top-level script that reformats all known CMake files in the source tree - Add a lint step in Travis that checks CMake files are properly formatted Requires [cmake_format](https://github.com/cheshirekow/cmake_format) and Python 3. Author: Antoine Pitrou <antoine@python.org> Closes apache#3606 from pitrou/ARROW-4363-cmake-format and squashes the following commits: 762df29 <Antoine Pitrou> Add mention in cpp/README.md 9073af9 <Antoine Pitrou> ARROW-4363: Add CMake format checks
- Add a top-level script that reformats all known CMake files in the source tree - Add a lint step in Travis that checks CMake files are properly formatted Requires [cmake_format](https://github.com/cheshirekow/cmake_format) and Python 3. Author: Antoine Pitrou <antoine@python.org> Closes apache#3606 from pitrou/ARROW-4363-cmake-format and squashes the following commits: 762df29 <Antoine Pitrou> Add mention in cpp/README.md 9073af9 <Antoine Pitrou> ARROW-4363: Add CMake format checks
Requires cmake_format and Python 3.