You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/07/22 06:33:39 UTC
[PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
LuciferYang opened a new pull request, #838:
URL: https://github.com/apache/iceberg-cpp/pull/838
## What
`cpp-linter/cpp-linter-action` is frozen at v2.15.1. Its v2.16 release pulled in
an untrusted dependency and was blocked by ASF Infra
(apache/infrastructure-actions#325), and the ASF gateway ignores newer versions.
The pinned action still runs, but it can no longer be updated.
## How
The action is a thin wrapper around the `cpp-linter` PyPI package, and the block
does not touch that package. This installs it plus matching clang tools with
pip and calls the `cpp-linter` CLI with the same options the action used. The
`checks-failed` output and the Fail fast step stay as they were. Because it is a
`run:` step and not a `uses:` reference, it adds no new third-party action to the
ASF allowlist.
I also looked at emitting SARIF and uploading it with
`github/codeql-action/upload-sarif`, which is allowed since it is a `github/*`
action. That route loses the thread comment and needs an extra conversion step,
so calling the CLI directly stays closer to what we have now.
## Verified
I ran the CLI locally against this repo's `.clang-format` and `.clang-tidy` on a
file containing a `NULL` and a mis-named member. Both clang-format and
clang-tidy ran, emitted `::warning` annotations, and wrote `checks-failed` to
`GITHUB_OUTPUT`. The PR also carries a temporary probe commit that trips the
linter on purpose so CI exercises the new path; I will drop it before merge.
---
This contribution was authored with assistance from Claude Opus 4.8, in line with
the Iceberg guidelines for AI-assisted contributions
(https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions).
All changes were reviewed and tested by the author.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628066752
##########
src/iceberg/temp_linter_probe.cc:
##########
@@ -0,0 +1,16 @@
+// TEMP verification file — will be dropped before merge.
+// Intentionally violates .clang-tidy to confirm the new cpp-linter CLI runs.
Review Comment:
- https://github.com/apache/iceberg-cpp/pull/838
<img width="1984" height="1744" alt="image" src="https://github.com/user-attachments/assets/1a32d29f-47a5-45aa-a3e6-18aa960f284d" />
It looks like the check is taking effect. I will revert this file and run the tests again.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636487196
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
I would prefer decoupling them into CLANG_FORMAT_VERSION and CLANG_TIDY_VERSION first because they may diverge in the future anyway. I'm fine to choose whatever version. We need to add a comment on either side to notify they should be in sync. WDYT?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3635882556
##########
.github/workflows/cpp-linter.yml:
##########
@@ -48,6 +48,8 @@ jobs:
env:
SCCACHE_DIR: ${{ github.workspace }}/.sccache
SCCACHE_CACHE_SIZE: "2G"
+ CPP_LINTER_VERSION: "1.13.0"
+ CLANG_TOOLS_VERSION: "22.1.8"
Review Comment:
```suggestion
CLANG_TOOLS_VERSION: "22.1.5" # Don't forget to update .pre-commit-config.yaml
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628066752
##########
src/iceberg/temp_linter_probe.cc:
##########
@@ -0,0 +1,16 @@
+// TEMP verification file — will be dropped before merge.
+// Intentionally violates .clang-tidy to confirm the new cpp-linter CLI runs.
Review Comment:
<img width="1984" height="1744" alt="image" src="https://github.com/user-attachments/assets/1a32d29f-47a5-45aa-a3e6-18aa960f284d" />
It looks like the check is taking effect. I will revert this file and run the tests again.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3635763584
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Done. The versions now sit in the job's `env` block (`CPP_LINTER_VERSION`, `CLANG_TOOLS_VERSION`), and `--version` takes its major from `${CLANG_TOOLS_VERSION%%.*}`, so the clang version is only written once.
I left one thing out: `.pre-commit-config.yaml` pins its own clang-format (v22.1.5), so it and the CI job aren't sharing a source. Actions has no way to read a version across files without extra tooling, so I didn't want to widen this PR for it. I can open a separate issue if aligning them is worth it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3635894129
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Let me test this out.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636953409
##########
.github/workflows/cpp-linter.yml:
##########
@@ -48,6 +48,11 @@ jobs:
env:
SCCACHE_DIR: ${{ github.workspace }}/.sccache
SCCACHE_CACHE_SIZE: "2G"
+ CPP_LINTER_VERSION: "1.13.0"
+ # clang-format is kept in sync with .pre-commit-config.yaml; clang-tidy is
+ # pinned separately because that version is not published on PyPI.
+ CLANG_FORMAT_VERSION: "22.1.5"
+ CLANG_TIDY_VERSION: "22.1.8"
Review Comment:
The comment says the pinned `clang-tidy` version is not published on PyPI, but the workflow installs `clang-tidy==${CLANG_TIDY_VERSION}` via `pip`. If that statement is accurate, this step will fail at runtime. Either (a) adjust the installation method for `clang-tidy` (e.g., install via apt/LLVM packages and ensure it’s on `PATH`), or (b) correct the comment / version pins to reflect the actual source and availability.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628144688
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,36 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Good catch, done. Pinned setup-python to 3.13 and switched to `python -m pip install` so it uses the interpreter the action set up. I kept the run step as `cpp-linter` (its console entry point) since the package has no `__main__`, so it can't be called with `python -m`; it's installed into the same interpreter, so they stay in sync.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3637088139
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Before we settle on 22.1.5, one thing I hit while testing the pin. Running clang-format 22.1.5 (the version pre-commit pins) over the current tree, 4 files come back as non-conforming: `catalog/rest/error_handlers.cc`, `error_handlers.h`, `test/error_handlers_test.cc`, and `util/error_collector.h`. They pass under 22.1.8 (what CI runs today), but 22.1.5 wants to reformat them.
So the tree is effectively formatted to 22.1.8 right now, while pre-commit declares 22.1.5. The two are already out of sync. If CI's clang-format goes to 22.1.5, any later PR touching those files would fail the linter on formatting its author never changed.
I'd lean toward putting both CI and pre-commit on 22.1.8 instead: `CLANG_FORMAT_VERSION`, `CLANG_TIDY_VERSION`, and pre-commit all land on one version, and nothing needs reformatting. Same "keep them in sync" outcome you asked for, without the churn. WDYT?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636953447
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +96,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_FORMAT_VERSION}" "clang-tidy==${CLANG_TIDY_VERSION}"
Review Comment:
The comment says the pinned `clang-tidy` version is not published on PyPI, but the workflow installs `clang-tidy==${CLANG_TIDY_VERSION}` via `pip`. If that statement is accurate, this step will fail at runtime. Either (a) adjust the installation method for `clang-tidy` (e.g., install via apt/LLVM packages and ensure it’s on `PATH`), or (b) correct the comment / version pins to reflect the actual source and availability.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3635878970
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Since `.pre-commit-config.yaml` pins clang-format to 22.1.5, shouldn't we use the same version here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3637575445
##########
src/iceberg/temp_linter_probe.cc:
##########
@@ -0,0 +1,17 @@
+// TEMP verification file — will be dropped before merge.
+// Confirms cpp-linter runs clang-tidy with the pip-installed binary
+// (via --version=${pythonLocation}/bin) after dropping clang-format.
+namespace iceberg {
Review Comment:
New C++ source files in this repo consistently include the Apache Software Foundation license header (e.g., src/iceberg/name_mapping.cc:1-18). This file currently lacks that header, which can cause licensing/compliance checks to fail if the file ends up being merged.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628515663
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Should we parameterize these versions somewhere?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628010953
##########
src/iceberg/temp_linter_probe.cc:
##########
@@ -0,0 +1,16 @@
+// TEMP verification file — will be dropped before merge.
+// Intentionally violates .clang-tidy to confirm the new cpp-linter CLI runs.
Review Comment:
This file intentionally introduces clang-tidy/naming violations and will cause the linter job to fail as long as it remains in the PR. If it’s only meant as a temporary probe, it should be removed from the PR before merge (or moved to a non-reviewed location / excluded from the linter) so main stays green.
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,36 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Consider pinning the Python version and invoking pip via the selected interpreter. Without `python-version`, `setup-python` may change behavior over time, and using bare `pip` can accidentally target a different interpreter than the one set up by the action.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628334625
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
Review Comment:
https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628066752
have previously tested this by introducing a faulty temporary file.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636472208
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Sorry I clicked the suggestion by accident, now I've reverted it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628049657
##########
src/iceberg/temp_linter_probe.cc:
##########
@@ -0,0 +1,16 @@
+// TEMP verification file — will be dropped before merge.
+// Intentionally violates .clang-tidy to confirm the new cpp-linter CLI runs.
Review Comment:
This is a temporary test for lint validation and will be deleted after testing.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628047105
##########
src/iceberg/temp_linter_probe.cc:
##########
@@ -0,0 +1,16 @@
+// TEMP verification file — will be dropped before merge.
Review Comment:
This is a temporary test for lint validation and will be deleted after testing.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636690409
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +93,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_TOOLS_VERSION}" "clang-tidy==${CLANG_TOOLS_VERSION}"
+ - name: Run cpp-linter
id: linter
if: github.event_name == 'pull_request'
continue-on-error: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- with:
- style: file
- tidy-checks: ''
- version: 22
- files-changed-only: true
- lines-changed-only: true
- thread-comments: true
- ignore: 'build|cmake_modules|ci|src/iceberg/catalog/hive/gen-cpp'
- database: build
- verbosity: 'debug'
- # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
- extra-args: '-std=c++23 -I$PWD/src -I$PWD/build/src -I$PWD/build/_deps/sqlpp23-src/include -I/usr/include/postgresql -I/usr/include/mysql -fno-builtin-std-forward_like'
+ # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
+ run: |
+ cpp-linter \
+ --style=file \
+ --tidy-checks='' \
+ --version="${CLANG_TOOLS_VERSION%%.*}" \
+ --files-changed-only=true \
+ --lines-changed-only=true \
+ --thread-comments=true \
+ --ignore='build|cmake_modules|ci|src/iceberg/catalog/hive/gen-cpp' \
+ --database=build \
+ --verbosity=debug \
+ --extra-arg='-std=c++23' \
+ --extra-arg="-I${PWD}/src" \
+ --extra-arg="-I${PWD}/build/src" \
+ --extra-arg="-I${PWD}/build/_deps/sqlpp23-src/include" \
+ --extra-arg='-I/usr/include/postgresql' \
+ --extra-arg='-I/usr/include/mysql' \
+ --extra-arg='-fno-builtin-std-forward_like'
Review Comment:
The workflow pins/installs clang tools via the PyPI wheels (`clang-format`, `clang-tidy`), which typically install unversioned binaries (`clang-format`, `clang-tidy`). But the linter invocation passes `--version="${CLANG_TOOLS_VERSION%%.*}"`, which may make cpp-linter look for version-suffixed executables (e.g., `clang-format-22`, `clang-tidy-22`) and fail to find the tools even though they were installed. Either ensure the expected `*-<major>` names exist, or drop `--version` so cpp-linter uses the tools found on PATH.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636953503
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +96,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_FORMAT_VERSION}" "clang-tidy==${CLANG_TIDY_VERSION}"
Review Comment:
This introduces new runtime supply-chain dependencies via PyPI downloads in CI. If your project’s security posture requires tighter controls, consider adding a constraints/lock approach (e.g., a `requirements.txt`/constraints file with hashes and `--require-hashes`) so CI installs are reproducible and resistant to dependency substitution.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#issuecomment-5043153984
cc @wgtmac
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636953475
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +96,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_FORMAT_VERSION}" "clang-tidy==${CLANG_TIDY_VERSION}"
+ - name: Run cpp-linter
id: linter
if: github.event_name == 'pull_request'
continue-on-error: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- with:
- style: file
- tidy-checks: ''
- version: 22
- files-changed-only: true
- lines-changed-only: true
- thread-comments: true
- ignore: 'build|cmake_modules|ci|src/iceberg/catalog/hive/gen-cpp'
- database: build
- verbosity: 'debug'
- # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
- extra-args: '-std=c++23 -I$PWD/src -I$PWD/build/src -I$PWD/build/_deps/sqlpp23-src/include -I/usr/include/postgresql -I/usr/include/mysql -fno-builtin-std-forward_like'
+ # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
+ run: |
+ cpp-linter \
+ --style=file \
+ --tidy-checks='' \
+ --version="${CLANG_FORMAT_VERSION%%.*}" \
+ --files-changed-only=true \
Review Comment:
`${CLANG_FORMAT_VERSION%%.*}` relies on Bash-specific parameter expansion. GitHub Actions currently defaults to `bash` on Ubuntu, but this can break if the job/workflow shell defaults change. Consider making this robust by explicitly setting `shell: bash` for the step, or by computing/pinning the major version in an env var (e.g., `CLANG_VERSION_MAJOR: '22'`) and using that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636860079
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==1.13.0" "clang-format==22.1.8" "clang-tidy==22.1.8"
Review Comment:
Split them as you suggested: `CLANG_FORMAT_VERSION` (22.1.5, matching pre-commit) and `CLANG_TIDY_VERSION` (22.1.8, since clang-tidy has no 22.1.5 on PyPI). Added a comment on the format version pointing at `.pre-commit-config.yaml` as the sync reminder.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#issuecomment-5065615397
@wgtmac Thanks for your thorough review and help.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3628170281
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +91,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
Review Comment:
Because this workflow’s `pull_request.paths-ignore` includes `.github/**` (see top of this file), a PR that *only* changes this workflow will not trigger the job, so the new `cpp-linter` CLI path may not get exercised by CI before merge. If this PR no longer includes a non-ignored change (e.g., the temporary probe file), consider adding a temporary `workflow_dispatch:` trigger or narrowing the ignore rules so this workflow change can be validated in CI before merging.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636462801
##########
.github/workflows/cpp-linter.yml:
##########
@@ -48,6 +48,8 @@ jobs:
env:
SCCACHE_DIR: ${{ github.workspace }}/.sccache
SCCACHE_CACHE_SIZE: "2G"
+ CPP_LINTER_VERSION: "1.13.0"
+ CLANG_TOOLS_VERSION: "22.1.5" # Don't forget to update .pre-commit-config.yaml
Review Comment:
```suggestion
CLANG_TOOLS_VERSION: "22.1.8"
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3636592005
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +93,38 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_TOOLS_VERSION}" "clang-tidy==${CLANG_TOOLS_VERSION}"
Review Comment:
The workflow installs Python packages with pip but doesn’t upgrade pip first. In this repo’s other workflow (`.github/workflows/test.yml`), pip is upgraded before installs; doing the same here reduces the chance of CI breakage due to an outdated bundled pip (e.g., new wheel tags / TLS handling) and keeps workflows consistent.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3637158441
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +96,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_FORMAT_VERSION}" "clang-tidy==${CLANG_TIDY_VERSION}"
Review Comment:
Do we need clang-format in this job at all? The pre-commit workflow already runs the pinned clang-format hook over all files on every PR. Keeping formatting there and making cpp-linter tidy-only would avoid duplicate checks and the version-sync problem. Could we drop `CLANG_FORMAT_VERSION` and the `clang-format` package here, and set `--style` to an empty string to disable format checks?
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +96,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-format==${CLANG_FORMAT_VERSION}" "clang-tidy==${CLANG_TIDY_VERSION}"
+ - name: Run cpp-linter
id: linter
if: github.event_name == 'pull_request'
continue-on-error: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- with:
- style: file
- tidy-checks: ''
- version: 22
- files-changed-only: true
- lines-changed-only: true
- thread-comments: true
- ignore: 'build|cmake_modules|ci|src/iceberg/catalog/hive/gen-cpp'
- database: build
- verbosity: 'debug'
- # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
- extra-args: '-std=c++23 -I$PWD/src -I$PWD/build/src -I$PWD/build/_deps/sqlpp23-src/include -I/usr/include/postgresql -I/usr/include/mysql -fno-builtin-std-forward_like'
+ # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
+ run: |
+ cpp-linter \
+ --style=file \
+ --tidy-checks='' \
+ --version="${CLANG_FORMAT_VERSION%%.*}" \
Review Comment:
Even with a tidy-only setup, deriving `--version=22` would still bypass the pip pin. cpp-linter looks for `clang-tidy-22` first, and the probe run selected `/usr/bin/clang-tidy-22` at 22.1.2 instead of the installed 22.1.8. Could we point this to `${pythonLocation}/bin`, or omit `--version`, so the pip-installed binary is actually used?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#issuecomment-5060565095
Nice work! Thanks @LuciferYang!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: replace cpp-linter-action with cpp-linter CLI [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #838:
URL: https://github.com/apache/iceberg-cpp/pull/838#discussion_r3637709575
##########
.github/workflows/cpp-linter.yml:
##########
@@ -91,24 +95,40 @@ jobs:
with:
path: ${{ github.workspace }}/.sccache
key: sccache-cpp-linter-ubuntu-${{ github.run_id }}
- - uses: cpp-linter/cpp-linter-action@0f6d1b8d7e38b584cbee606eb23d850c217d54f8 # v2.15.1
+ - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
+ if: github.event_name == 'pull_request'
+ with:
+ python-version: '3.13'
+ - name: Install cpp-linter and clang tools
+ if: github.event_name == 'pull_request'
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install "cpp-linter==${CPP_LINTER_VERSION}" "clang-tidy==${CLANG_TIDY_VERSION}"
+ - name: Run cpp-linter
id: linter
if: github.event_name == 'pull_request'
continue-on-error: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- with:
- style: file
- tidy-checks: ''
- version: 22
- files-changed-only: true
- lines-changed-only: true
- thread-comments: true
- ignore: 'build|cmake_modules|ci|src/iceberg/catalog/hive/gen-cpp'
- database: build
- verbosity: 'debug'
- # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
- extra-args: '-std=c++23 -I$PWD/src -I$PWD/build/src -I$PWD/build/_deps/sqlpp23-src/include -I/usr/include/postgresql -I/usr/include/mysql -fno-builtin-std-forward_like'
+ # need '-fno-builtin-std-forward_like', see https://github.com/llvm/llvm-project/issues/101614
+ run: |
+ cpp-linter \
+ --style='' \
+ --tidy-checks='' \
Review Comment:
This PR description says the CLI is invoked with the same options the action used, but this invocation disables clang-format entirely via `--style=''` (the previous workflow used `style: file`). If the intent is to keep clang-format enforcement only in pre-commit, consider updating the PR description (and/or a short note here) to avoid confusion about the behavior change; otherwise, switch back to `--style=file` and ensure clang-format is available in the job.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org