Skip to content

GH-46496: [CI][Dev] Fix shellcheck SC2086 errors in ci/scripts directory - #46497

Merged
kou merged 3 commits into
apache:mainfrom
hiroyuki-sato:topic/shellcheck-sc2086
May 21, 2025
Merged

GH-46496: [CI][Dev] Fix shellcheck SC2086 errors in ci/scripts directory#46497
kou merged 3 commits into
apache:mainfrom
hiroyuki-sato:topic/shellcheck-sc2086

Conversation

@hiroyuki-sato

@hiroyuki-satohiroyuki-sato commented May 19, 2025

Copy link
Copy Markdown
Collaborator

Rationale for this change

We are trying to implement shellcheck on all sh files in #44748.

What changes are included in this PR?

SC2086 check requires quoting like ${url} -> "${url}".

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #46496has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting review Awaiting review label May 19, 2025
@hiroyuki-sato

hiroyuki-sato commented May 19, 2025

Copy link
Copy Markdown
CollaboratorAuthor

I modified many files at once. But, all of changes are ${var} -> "${var}". So, I believe review is not so hard.

Please let me know, it would be better to split this into multiple PRs.
(IMO, It is a little tedious to create PR for one by one)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this file in a separated PR?

We moved the Java implementation to apache/arrow-java.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Should I also remove ci/scripts/rust_build.sh too?

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index fe178e069..3ef23a08e 100644
--- a/.pre-commit-config.yaml+++ b/.pre-commit-config.yaml@@ -196,13 +196,31 @@ repos:
?^ci/scripts/c_glib_build\.sh$|
?^ci/scripts/c_glib_test\.sh$|
?^ci/scripts/conan_setup\.sh$|
+ ?^ci/scripts/csharp_build\.sh$|+ ?^ci/scripts/csharp_pack\.sh$|
?^ci/scripts/download_tz_database\.sh$|
?^ci/scripts/install_azurite\.sh$|
?^ci/scripts/install_ccache\.sh$|
?^ci/scripts/install_ceph\.sh$|
+ ?^ci/scripts/install_chromedriver\.sh$|+ ?^ci/scripts/install_cmake\.sh$|+ ?^ci/scripts/install_emscripten\.sh$|+ ?^ci/scripts/install_iwyu\.sh$|+ ?^ci/scripts/install_ninja\.sh$|+ ?^ci/scripts/install_numpy\.sh$|+ ?^ci/scripts/install_pandas\.sh$|+ ?^ci/scripts/install_python\.sh$|
?^ci/scripts/install_spark\.sh$|
+ ?^ci/scripts/install_vcpkg\.sh$|
?^ci/scripts/integration_dask\.sh$|
+ ?^ci/scripts/java_full_build\.sh$|+ ?^ci/scripts/matlab_build\.sh$|+ ?^ci/scripts/msys2_system_clean\.sh$|
?^ci/scripts/msys2_system_upgrade\.sh$|
+ ?^ci/scripts/python_sdist_build\.sh$|+ ?^ci/scripts/release_test\.sh$|+ ?^ci/scripts/ruby_test\.sh$|+ ?^ci/scripts/rust_build\.sh$|
?^ci/scripts/util_free_space\.sh$|
?^cpp/build-support/build-lz4-lib\.sh$|
?^cpp/build-support/build-zstd-lib\.sh$|

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used here:

${arrow_dir}/ci/scripts/rust_build.sh ${arrow_dir}${build_dir}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #46502

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 19, 2025
@kou

kou commented May 19, 2025

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g python

@kou

kou commented May 19, 2025

Copy link
Copy Markdown
Member

Could you open a new issue for MATLAB CI failures?

@github-actions

Copy link
Copy Markdown

Revision: f1917a8

Submitted crossbow builds: ursacomputing/crossbow @ actions-c037629e6f

TaskStatus
example-python-minimal-build-fedora-condaGitHub Actions
example-python-minimal-build-ubuntu-venvGitHub Actions
test-conda-python-3.10GitHub Actions
test-conda-python-3.10-hdfs-2.9.2GitHub Actions
test-conda-python-3.10-hdfs-3.2.1GitHub Actions
test-conda-python-3.10-pandas-latest-numpy-latestGitHub Actions
test-conda-python-3.11GitHub Actions
test-conda-python-3.11-dask-latestGitHub Actions
test-conda-python-3.11-dask-upstream_develGitHub Actions
test-conda-python-3.11-hypothesisGitHub Actions
test-conda-python-3.11-pandas-latest-numpy-1.26GitHub Actions
test-conda-python-3.11-pandas-latest-numpy-latestGitHub Actions
test-conda-python-3.11-pandas-nightly-numpy-nightlyGitHub Actions
test-conda-python-3.11-pandas-upstream_devel-numpy-nightlyGitHub Actions
test-conda-python-3.11-spark-masterGitHub Actions
test-conda-python-3.12GitHub Actions
test-conda-python-3.12-cpython-debugGitHub Actions
test-conda-python-3.13GitHub Actions
test-conda-python-3.9GitHub Actions
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5GitHub Actions
test-conda-python-emscriptenGitHub Actions
test-cuda-python-ubuntu-22.04-cuda-11.7.1GitHub Actions
test-debian-12-python-3-amd64GitHub Actions
test-debian-12-python-3-i386GitHub Actions
test-fedora-39-python-3GitHub Actions
test-ubuntu-22.04-python-3GitHub Actions
test-ubuntu-22.04-python-313-freethreadingGitHub Actions
test-ubuntu-24.04-python-3GitHub Actions

@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

I created #46498 for MATLAB CI Failure.

@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 19, 2025
@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

It seems that the following error are know issues.

Rest of two are the same error.

  • example-python-minimal-build-fedora-conda
  • example-python-minimal-build-ubuntu-venv

https://github.com/ursacomputing/crossbow/actions/runs/15107317228/job/42458657050#step:3:13178
https://github.com/ursacomputing/crossbow/actions/runs/15107317390/job/42458658267#step:3:15548

E ImportError: Pandas requires version '10.0.1' or newer of 'pyarrow' (version '0.1.dev17552+gf1917a8' currently installed).

@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

@github-actions crossbow submit example-python-minimal-build-fedora-conda

@hiroyuki-sato

hiroyuki-sato commented May 20, 2025

Copy link
Copy Markdown
CollaboratorAuthor

It seems that I don't have permission crossbow. It worked.

@github-actions

Copy link
Copy Markdown

Revision: 709f074

Submitted crossbow builds: ursacomputing/crossbow @ actions-0b8afc1ec8

TaskStatus
example-python-minimal-build-fedora-condaGitHub Actions

@raulcd

raulcd commented May 20, 2025

Copy link
Copy Markdown
Member

The failure on the example-python-minimal-* is a known issue if your fork is missing some dev tags, see:

You can either push the dev tags from the apache/arrow repo to your fork or ignore those failures.

We should fix the issue I've shared as this is recurring for new contributions :)

@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

@raulcd Thanks! I pushed tag to my repository.
#44720 (comment)

@raulcd

Copy link
Copy Markdown
Member

@github-actions crossbow submit example-python-minimal-build-fedora-conda

@github-actions

Copy link
Copy Markdown

Revision: 709f074

Submitted crossbow builds: ursacomputing/crossbow @ actions-33609c0757

TaskStatus
example-python-minimal-build-fedora-condaGitHub Actions

@hiroyuki-sato

Copy link
Copy Markdown
CollaboratorAuthor

@kou crossbow failure are known issues. I think crossbow passed. Please take a look when you get a chance.

kou
kou approved these changes May 21, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou
kou merged commit ddfa762 into apache:mainMay 21, 2025
@koukou removed the awaiting change review Awaiting change review label May 21, 2025
@github-actionsgithub-actionsBot added the awaiting merge Awaiting merge label May 21, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting mergeAwaiting merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@hiroyuki-sato@kou@raulcd