You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AntoinePrv (via GitHub)" <gi...@apache.org> on 2025/07/22 14:54:25 UTC
[PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
AntoinePrv opened a new pull request, #47168:
URL: https://github.com/apache/arrow/pull/47168
### Rationale for this change
Update clang-format to better match modern IDEs.
### What changes are included in this PR?
- Update clang-format
- Reformat code base with newer version
### Are these changes tested?
Yes
### Are there any user-facing changes?
No
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 merged PR #47168:
URL: https://github.com/apache/arrow/pull/47168
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3741537709
> > Is this because we want to have a system that has defaulted precommit/clang-format that matches the latest requirement?
>
> It's for only pre-commit. We don't need to care about clang-format version. pre-commit installs clang-format automatically.
Oh, I mean even on older Ubuntu we can still have a recent-enough pre-commit, right? Why does this have anything to do with the version of Ubuntu? (Is it because on Ubuntu 24.04, the defaulted pre-commit is recent-enough, so people don't have to install one?)
> ```diff
> diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml
> index b763cfbbbc..59171ddcaa 100644
> --- a/.github/workflows/dev.yml
> +++ b/.github/workflows/dev.yml
> @@ -41,8 +41,8 @@ jobs:
>
> lint:
> name: Lint C++, Python, R, Docker, RAT
> - # Use Ubuntu 22.04 to ensure working pre-commit on Ubuntu 22.04.
> - runs-on: ubuntu-22.04
> + # Use Ubuntu 24.04 to ensure working pre-commit on Ubuntu 24.04.
> + runs-on: ubuntu-24.04
> if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
> timeout-minutes: 15
> steps:
> ```
Thanks a lot! Let me try 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3737289430
> How about requiring (recommending?) Ubuntu 24.04 or later (not 22.04) for development in our documentation?
Is this because we want to have a system that has defaulted precommit/clang-format that matches the latest requirement?
Another question is, other than clarifying in the document, what else need to be changed to let the CI pass?
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3110282441
Supporting old pre-commit (system pre-commit) is for new contributors on Ubuntu 22.04.
If we have enough documentation (or something) for new contributors, we can require more newer pre-commit (and identify).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3733370641
> I've made a commit in this PR with an update to the clang-format version change from v20 to a less aggressive v18, and related code formatting.
That's fine with me on the principle. The lint CI job is failing, though.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3748750277
Thank you all. Let me 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3113304605
A possible direction to help both could be to leverage [pixi](https://pixi.sh/).
I'm using it locally and in many other projects (C++/Rust/Python): it combines conda dependencies and run commands in managed environments.
Once installed (which is fairly easy IMHO), a one command `pixi run lint`, `pixi run test` _etc_. could download and install all required dependencies as needed, execute intermediary steps (with caching), and run the commands.
What's great is that all type of dependencies (Python itself, R, compilers...) can be pulled, on all platforms.
In the case of `pre-commit`, I've already set it up to lint/format with the same executable used for both one-stop command, and through `pre-commit`.
I'm not claiming it could handle all Arrow development cases, but it certainly make for a great on-boarding.
Using it also does not prevent from not using it.
I've been meaning to write a blog post about how I use it in development. If that seems like an interesting direction, I could come back when done, and will be willing to contribute a configuration.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3733040671
I think it becomes more reasonable now to move forward this PR, since:
1. We have officially upgraded to C++20, which already requires a more recent clang (#48414). Actually we are using v18 in our CI ([1] [2]).
2. I added several concepts in #48459 ([3]), which is formatted differently between current v14 (in precommit) and v15+ (in my local). This is quite annoying and I had to manually modify it to match the required v14 format. The clang-format behavior change wrt C++20 concepts can be found in [4] [5].
I've made a commit in this PR with an update to the clang-format version change from v20 to a less aggressive v18, and related code formatting. @kou @pitrou @AntoinePrv what do you think?
Thanks.
[1] https://github.com/apache/arrow/blob/7fe39d470f6b1fb803772dbb6dccfb4d26af54fe/.github/workflows/cpp.yml#L84
[2] https://github.com/apache/arrow/blob/7fe39d470f6b1fb803772dbb6dccfb4d26af54fe/.env#L60
[3] https://github.com/apache/arrow/blob/3fb84ff2c66deea205b45cad697ab724ea98ad5d/cpp/src/arrow/compute/kernels/hash_aggregate.cc#L283-L285
[4] https://github.com/llvm/llvm-project/issues/56283
[5] https://releases.llvm.org/15.0.0/tools/clang/docs/ReleaseNotes.html#clang-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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3747297870
Thank you @kou .
> * Could you open an issue if we want to defer documentation?
Yes, #48850 filed.
> * Is 18.1.8 OK? It seems that the latest available version is 21.1.8: [pre-commit/mirrors-clang-format/tags](https://github.com/pre-commit/mirrors-clang-format/tags)
I used v18 to match the version used in the rest of our CI, see https://github.com/apache/arrow/pull/47168#issuecomment-3733040671 .
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3112436767
`pre-commit` is very easy to install via either conda or pip. Are there other dependencies we are getting from these sources?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3116247819
I don't have a strong opinion how to care about new contributors. I don't object the Pixi (or pip) approach if we have the official document how to prepare pre-commit with Pixi or something.
I chose the supporting system pre-commit approach just because we don't need to write additional document how to prepare pre-commit.
It may be better that we start a discussion thread on `dev@arrow.apache.org` https://lists.apache.org/list.html?dev@arrow.apache.org to collect more opinions.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3112453177
apache/arrow has many language implementations. We can't assume that all new contributors are familiar with Python, conda and/or pip.
For example, this thread https://github.com/apache/arrow/pull/46686#discussion_r2129176670 considered about new R contributors.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3103193661
:warning: GitHub issue #47167 **has been automatically assigned in GitHub** to PR creator.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3103364252
Without it, that sends back `pre-commit to 15.*`
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3741563528
> Oh, I mean even on older Ubuntu we can still have a recent-enough pre-commit, right?
Yes, if a user installs pre-commit by themselves. (A user can't install pre-commit by `apt install`. It's old for this PR's configuration.)
> Why does this have anything to do with the version of Ubuntu? (Is it because on Ubuntu 24.04, the defaulted pre-commit is recent-enough, so people don't have to install one?)
Yes. We can use `apt install pre-commit` to install pre-commit for Apache Arrow on Ubuntu 24.04. I don't have a strong opinion what is documented as I said before: https://github.com/apache/arrow/pull/47168#issuecomment-3116247819
I just thought that requiring Ubuntu 24.04 may be easier to be documented. I'm OK with other approach such as using pip or conda.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3734269812
How about requiring (recommending?) Ubuntu 24.04 or later (not 22.04) for development in our documentation?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3750606873
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit a1ec5a9a1decb163eef26deecf60a6903f9aa125.
There weren't enough matching historic benchmark results to make a call on whether there were regressions.
The [full Conbench report](https://github.com/apache/arrow/runs/60377410869) has more details.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3103336344
The Python failures are unrelated, I just merged the fix on main, if you rebase they will go away.
It seems the newer clang-format pre-commit (from 16.0.0) fails on Ubuntu 22.04 because `textproto` was added to identify [2.5.20](https://github.com/pre-commit/identify/commit/99968e41a8ed1a0b1a8c42e80f65be5d5ee574c3) but Ubuntu seems to be using indentify 2.4.10-1 (https://packages.ubuntu.com/jammy/python3-identify).
@kou is using the system pre-commit a requirement? I am unsure what the discussion was when we moved from archery lint to pre-commit. Some info on when this was added:
- https://github.com/apache/arrow/issues/46798
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3743233064
Thank you @kou for clarifying! I agree that requiring Ubuntu 24.04 might be easiest in terms of documentation. I personally also think this might not be a big deal for developers, since I personal haven't been using precommit at all until very recently I started to work on this PR. In other words, those developers who don't use precommit may not feel it at all.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3743798165
Hi @kou @pitrou , shall we move on with this one? We can later add documentation wrt the required Ubuntu version for developers.
Thanks. cc @AntoinePrv
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3748469088
Thanks for picking up the work @zanmato1984
> > * Is 18.1.8 OK? It seems that the latest available version is 21.1.8: [pre-commit/mirrors-clang-format/tags](https://github.com/pre-commit/mirrors-clang-format/tags)
>
> I used v18 to match the version used in the rest of our CI, see [#47168 (comment)](https://github.com/apache/arrow/pull/47168#issuecomment-3733040671) .
The biggest blocker for was with pre-commit (recent clang-format hooks did not support earlier versions), apart from that I don't see why not go to the most recent (for me that is better because I constrain the `clangd` LSP I use to the version of `clang-format` used in Arrow so that my editor formatting does not diverge).
But yes, most important is that all versions match in Arrow CI.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47167: [C++][Dev] Update clang-format dependency [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47168:
URL: https://github.com/apache/arrow/pull/47168#issuecomment-3737501455
> > How about requiring (recommending?) Ubuntu 24.04 or later (not 22.04) for development in our documentation?
>
> Is this because we want to have a system that has defaulted precommit/clang-format that matches the latest requirement?
It's for only pre-commit. We don't need to care about clang-format version. pre-commit installs clang-format automatically.
(I'm OK with what is documented. We require Ubuntu 24.04, we require pip/conda/..., or something.)
> Another question is, other than clarifying in the document, what else need to be changed to let the CI pass?
```diff
diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml
index b763cfbbbc..59171ddcaa 100644
--- a/.github/workflows/dev.yml
+++ b/.github/workflows/dev.yml
@@ -41,8 +41,8 @@ jobs:
lint:
name: Lint C++, Python, R, Docker, RAT
- # Use Ubuntu 22.04 to ensure working pre-commit on Ubuntu 22.04.
- runs-on: ubuntu-22.04
+ # Use Ubuntu 24.04 to ensure working pre-commit on Ubuntu 24.04.
+ runs-on: ubuntu-24.04
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
timeout-minutes: 15
steps:
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org