GH-46528: [CI][Dev] Remove "archery lint" by kou · Pull Request #46686 · apache/arrow

kou · 2025-06-03T05:15:57Z

Rationale for this change

We can use pre-commit instead of "archery lint" for all linters/formatters.

What changes are included in this PR?

Remove "archery lint" CI job
Remove "archery lint" related codes including IWYU and clang-tidy because IWYU and clang-tidy aren't used for now
Remove "archery lint" related docs including IWYU and clang-tidy related docs

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

It's only affected to developers.

GitHub Issue: [CI][Dev] Lint job failed with timeout #46528

kou · 2025-06-03T05:51:10Z

ci/scripts/install_iwyu.sh

We'll revisit IWYU in the future.
It's tracked by #46543 .

kou · 2025-06-03T05:53:07Z

cpp/build-support/run_clang_tidy.py

We'll revisit clang-tidy in the future.
It's tracked by #46542 .

kou · 2025-06-03T05:54:40Z

docs/source/developers/cpp/development.rst

pre-commit installs pined clang-format automatically.

kou · 2025-06-03T06:09:04Z

docs/source/developers/guide/step_by_step/styling.rst

This ID is added in this PR.

kou · 2025-06-03T06:10:36Z

r/vignettes/developers/workflow.Rmd

We'll use Air in the future.
It's tracked by #46592 .

raulcd

Should we remove these bits from the LICENSE and NOTICE files about cpplint code?

arrow/LICENSE.txt

Lines 784 to 793 in acbad29

    
           -------------------------------------------------------------------------------- 
        
           This project includes code from the Google styleguide. 
        
           * cpp/build-support/cpplint.py is based on the scripts from the Google styleguide. 
        
           Copyright: 2009 Google Inc. All rights reserved. 
        
           Homepage: https://github.com/google/styleguide 
        
           License: 3-clause BSD

arrow/NOTICE.txt

Lines 20 to 21 in acbad29

    
           This product includes software from the google-lint project 
        
            * Copyright (c) 2009 Google Inc. All rights reserved.

kou · 2025-06-03T08:11:44Z

Good catch! We should remove them. I'll do it.

raulcd

This seems good to me, I haven't used archery lint lately since we've been adding more linters to pre-commit. @pitrou any concern here?
I've double checked numpydoc have it's own separate archery command.

pitrou · 2025-06-04T06:44:27Z

That's fine with me. @kszucs Any comments?

thisisnic · 2025-06-05T15:53:57Z

r/vignettes/developers/workflow.Rmd

Thanks for making this all a lot more concise!
We should leave in information about how to run it other than with pre-commit. Many R users are more comfortable with running lintr directly as it's much simpler (I've personally had issues getting pre-commit set up so won't be the only one), so it'd be great to give them both options.

OK.
Is it OK that running r/tools/lint.R for non pre-commit option? R developers need to install lintr manually with this approach.
(Can we remove ci/docker/linux-apt-lint.dockerfile? I want to remove ci/docker/linux-apt-lint.dockerfile.)

Or is running lintr::lint_package("arrow/r") directly better?

Running lintr::lint_package("arrow/r") is better for a non-pre-commit option, yes. arrow/r/lint.R could possibly be removed once it's unhooked from CI. The average R contributor will probably run lintr::lint_package to lint and the core dev team generally runs arrow/r/lint.sh to lint R and C++ at the same time.

I'm not sure we can remove linux-apt-lint.dockerfile just yet. IIRC the way that's set up, it will catch lints in the R packages internal C++ code but the current r.yml workflow just lints the R code so removing linux-apt-lint.dockerfile could cause us to merge R C++ code with lint issues. I think we could modify the R workflow to keep the functionality though. Happy to work on that.

OK. Let's use lintr::lint_package("arrow/r").

the core dev team generally runs arrow/r/lint.sh to lint R and C++ at the same time.

Hmm... This PR removes r/lint.sh...

Could you share pre-commit related issues you got? Can we fix them instead of avoiding pre-commit?

I'm not sure we can remove linux-apt-lint.dockerfile just yet. IIRC the way that's set up, it will catch lints in the R packages internal C++ code but the current r.yml workflow just lints the R code so removing linux-apt-lint.dockerfile could cause us to merge R C++ code with lint issues. I think we could modify the R workflow to keep the functionality though.

Oh, I missed that the r.yml workflow also runs lintr:

arrow/.github/workflows/r.yml

Lines 334 to 351 in b630f48

- name: Run lintr

if: ${{ matrix.config.rversion == 'release' }}

env:

NOT_CRAN: "true"

GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

shell: Rscript {0}

working-directory: r

run: |

Sys.setenv(

RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),

MAKEFLAGS = paste0("-j", parallel::detectCores()),

ARROW_R_DEV = TRUE,

"_R_CHECK_FORCE_SUGGESTS_" = FALSE

)

# we use pak for package installation since it is faster, safer and more convenient

pak::local_install()

pak::pak("lintr")

lintr::expect_lint_free()

(BTW, I didn't know that lintr::expect_lint_free() exists. I changed the pre-commit configuration to use lintr::expect_lint_free() and removes r/tools/lint.R.)

I want to remove the "Run lintr" step too because it's already done by pre-commit

arrow/.github/workflows/dev.yml

Lines 66 to 68 in b630f48

- name: Run pre-commit

run: |

pre-commit run --all-files --color=always --show-diff-on-failure

and archery lint

arrow/.github/workflows/dev.yml

Lines 71 to 78 in b630f48

- name: Execute Docker Build

env:

ARCHERY_DOCKER_USER: ${{ secrets.DOCKERHUB_USER }}

ARCHERY_DOCKER_PASSWORD: ${{ secrets.DOCKERHUB_TOKEN }}

UBUNTU: 22.04

run: |

source ci/scripts/util_enable_core_dumps.sh

archery docker run -e GITHUB_ACTIONS=true ubuntu-lint

.

And pre-commit and archery lint check not only R code but also R C++ code. So we will not merge R C++ code with lint issues.

Thanks. You're using Ubuntu 22.04, right?

How about the following?

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c3835ac0f1..e3e8abdeed 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -356,7 +356,10 @@ repos: ?^swift/gen-protobuffers\.sh$| ) - repo: https://github.com/scop/pre-commit-shfmt - rev: v3.11.0-1 + # v3.11.0-1 or later requires pre-commit 3.2.0 or later but Ubuntu + # 22.04 ships pre-commit 2.17.0. We can use update this rev after + # Ubuntu 22.04 reached EOL (June 2027). + rev: v3.10.0-1 hooks: - id: shfmt alias: shell

I've opened #46798 for this.

FYI: You can use pre-commit run --all-files r only for R as described in the changed document.
In the case, shfmt isn't used. So the error isn't occurred.

#46799 will fix this.

I feel like I did get some error when I ran the full command but can't recall now as I've just updated pre-commit. Thanks @kou! :)

OK. I'll merge this.

kszucs · 2025-06-05T16:05:03Z

We have duplicated most of the linters in both archery and pre-commit hooks also causing version and configuration mismatches for certain tools, so I strongly support this change.

r/vignettes/developers/workflow.Rmd

We can use pre-commit instead of "archery lint".

Co-authored-by: Bryce Mecum <petridish@gmail.com>

kou · 2025-06-12T01:58:34Z

I'll merge this in a few days if nobody objects it.

conbench-apache-arrow · 2025-06-13T15:35:44Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c314e80.

There were 73 benchmark results with an error:

Commit Run on arm64-t4g-2xlarge-linux at 2025-06-13 08:54:39Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1
and 71 more (see the report linked below)

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

### Rationale for this change We can use pre-commit instead of "archery lint" for all linters/formatters. ### What changes are included in this PR? * Remove "archery lint" CI job * Remove "archery lint" related codes including IWYU and clang-tidy because IWYU and clang-tidy aren't used for now * Remove "archery lint" related docs including IWYU and clang-tidy related docs ### Are these changes tested? Yes. ### Are there any user-facing changes? No. It's only affected to developers. * GitHub Issue: apache#46528 Lead-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Bryce Mecum <petridish@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou requested review from assignUser, jonkeane, raulcd and thisisnic as code owners June 3, 2025 05:15

github-actions bot added Component: R Component: C++ Component: Documentation awaiting committer review Awaiting committer review labels Jun 3, 2025

kou commented Jun 3, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 3, 2025

raulcd reviewed Jun 3, 2025

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 3, 2025

raulcd approved these changes Jun 4, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 4, 2025

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jun 5, 2025

thisisnic reviewed Jun 5, 2025

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 10, 2025

amoeba reviewed Jun 10, 2025

View reviewed changes

r/vignettes/developers/workflow.Rmd Outdated Show resolved Hide resolved

amoeba reviewed Jun 10, 2025

View reviewed changes

r/vignettes/developers/workflow.Rmd Outdated Show resolved Hide resolved

github-actions bot removed the awaiting changes Awaiting changes label Jun 10, 2025

github-actions bot added the awaiting change review Awaiting change review label Jun 10, 2025

kou and others added 5 commits June 11, 2025 05:53

apacheGH-46528: [CI][Dev] Remove "archery lint"

dbe3b7c

We can use pre-commit instead of "archery lint".

Remove needless license information

e1d1e1b

Use lintr::expect_lint_free()

2d1fed0

Keep direct lintr usage

961816c

Co-authored-by: Bryce Mecum <petridish@gmail.com>

Remove duplicated lintr step

478eb89

kou force-pushed the ci-remove-archery-lint branch from 69ef60a to 478eb89 Compare June 10, 2025 20:53

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 12, 2025

kou mentioned this pull request Jun 13, 2025

[CI][Dev] Add support for pre-commit 2.17.0 provided by Ubuntu 22.04 #46798

Closed

kou merged commit c314e80 into apache:main Jun 13, 2025
43 of 44 checks passed

kou deleted the ci-remove-archery-lint branch June 13, 2025 07:10

kou removed the awaiting changes Awaiting changes label Jun 13, 2025

kou mentioned this pull request Jun 13, 2025

[CI][Dev] Lint job failed with timeout #46528

Closed

github-actions bot added the awaiting changes Awaiting changes label Jun 13, 2025

kou mentioned this pull request Jul 24, 2025

GH-47167: [C++][Dev] Update clang-format dependency #47168

Merged

	--------------------------------------------------------------------------------

	This project includes code from the Google styleguide.

	* cpp/build-support/cpplint.py is based on the scripts from the Google styleguide.

	Copyright: 2009 Google Inc. All rights reserved.
	Homepage: https://github.com/google/styleguide
	License: 3-clause BSD

	This product includes software from the google-lint project
	* Copyright (c) 2009 Google Inc. All rights reserved.

	- name: Run lintr
	if: ${{ matrix.config.rversion == 'release' }}
	env:
	NOT_CRAN: "true"
	GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
	shell: Rscript {0}
	working-directory: r
	run: \|
	Sys.setenv(
	RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
	MAKEFLAGS = paste0("-j", parallel::detectCores()),
	ARROW_R_DEV = TRUE,
	"_R_CHECK_FORCE_SUGGESTS_" = FALSE
	)
	# we use pak for package installation since it is faster, safer and more convenient
	pak::local_install()
	pak::pak("lintr")
	lintr::expect_lint_free()

	- name: Run pre-commit
	run: \|
	pre-commit run --all-files --color=always --show-diff-on-failure

	- name: Execute Docker Build
	env:
	ARCHERY_DOCKER_USER: ${{ secrets.DOCKERHUB_USER }}
	ARCHERY_DOCKER_PASSWORD: ${{ secrets.DOCKERHUB_TOKEN }}
	UBUNTU: 22.04
	run: \|
	source ci/scripts/util_enable_core_dumps.sh
	archery docker run -e GITHUB_ACTIONS=true ubuntu-lint

Conversation

kou commented Jun 3, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

kou commented Jun 3, 2025

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jun 4, 2025

Uh oh!

thisisnic Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kszucs commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

kou commented Jun 12, 2025

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kou commented Jun 3, 2025 •

edited by github-actions bot

Loading

kou Jun 3, 2025 •

edited

Loading

thisisnic Jun 5, 2025 •

edited

Loading