-
Notifications
You must be signed in to change notification settings - Fork 3.8k
GH-46763: [CI][Dev] fix shellcheck errors in the ci/scripts/ccache_setup.sh #46766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-46763: [CI][Dev] fix shellcheck errors in the ci/scripts/ccache_setup.sh #46766
Conversation
|
@github-actions crossbow submit -g cpp -g ruby |
Revision: 74cc885 Submitted crossbow builds: ursacomputing/crossbow @ actions-efb3af9cc5 |
So I tried crossbow ruby and cpp
|
@github-actions crossbow submit test-ubuntu-22.04-cpp-bundled |
Revision: 74cc885 Submitted crossbow builds: ursacomputing/crossbow @ actions-4830b2d151
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses shellcheck warnings in the ccache_setup.sh
script by quoting the environment variable and grouping the echo commands, and it updates the pre-commit configuration to include the new script.
- Quote
$GITHUB_ENV
and wrap echo statements in a grouped redirect block inci/scripts/ccache_setup.sh
- Add
ci/scripts/ccache_setup.sh
to the shell hook patterns in.pre-commit-config.yaml
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
ci/scripts/ccache_setup.sh | Combined multiple echo statements into a single { ... } >> "$GITHUB_ENV" block and added quoting |
.pre-commit-config.yaml | Included the new ccache_setup.sh script in the shell hook file patterns |
Why is |
Ah, |
We don't use Crossbow for diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml
index 8c4388fc0f..106cc1fd86 100644
--- a/.github/workflows/cpp.yml
+++ b/.github/workflows/cpp.yml
@@ -29,6 +29,7 @@ on:
- '.github/workflows/cpp.yml'
- 'ci/conda_env_*'
- 'ci/docker/**'
+ - 'ci/scripts/ccache_setup.sh'
- 'ci/scripts/cpp_*'
- 'ci/scripts/install_azurite.sh'
- 'ci/scripts/install_gcs_testbench.sh'
@@ -45,6 +46,7 @@ on:
- '.github/workflows/cpp.yml'
- 'ci/conda_env_*'
- 'ci/docker/**'
+ - 'ci/scripts/ccache_setup.sh'
- 'ci/scripts/cpp_*'
- 'ci/scripts/install_azurite.sh'
- 'ci/scripts/install_gcs_testbench.sh'
diff --git a/.github/workflows/cpp_extra.yml b/.github/workflows/cpp_extra.yml
index 3df63568e9..e982afde91 100644
--- a/.github/workflows/cpp_extra.yml
+++ b/.github/workflows/cpp_extra.yml
@@ -27,6 +27,7 @@ on:
- '.github/workflows/cpp_extra.yml'
- 'ci/conda_env_*'
- 'ci/docker/**'
+ - 'ci/scripts/ccache_setup.sh'
- 'ci/scripts/cpp_*'
- 'ci/scripts/install_azurite.sh'
- 'ci/scripts/install_gcs_testbench.sh'
@@ -45,6 +46,7 @@ on:
- '.github/workflows/cpp_extra.yml'
- 'ci/conda_env_*'
- 'ci/docker/**'
+ - 'ci/scripts/ccache_setup.sh'
- 'ci/scripts/cpp_*'
- 'ci/scripts/install_azurite.sh'
- 'ci/scripts/install_gcs_testbench.sh'
diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml
index 8cb16049f0..af52e73320 100644
--- a/.github/workflows/ruby.yml
+++ b/.github/workflows/ruby.yml
@@ -29,6 +29,7 @@ on:
- '.github/workflows/ruby.yml'
- 'ci/docker/**'
- 'ci/scripts/c_glib_*'
+ - 'ci/scripts/ccache_setup.sh'
- 'ci/scripts/cpp_*'
- 'ci/scripts/msys2_*'
- 'ci/scripts/ruby_*'
@@ -43,6 +44,7 @@ on:
- '.github/workflows/ruby.yml'
- 'ci/docker/**'
- 'ci/scripts/c_glib_*'
+ - 'ci/scripts/ccache_setup.sh'
- 'ci/scripts/cpp_*'
- 'ci/scripts/msys2_*'
- 'ci/scripts/ruby_*' |
…che_setup.sh Co-Authored-by: Sutou Kouhei <kou@ clear-code.com>
Thanks. I applied suggested change. Should we separate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit e00c01a. There were 71 benchmark results with an error:
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. |
…che_setup.sh (apache#46766) ### Rationale for this change This is the sub issue apache#44748. * SC2086: (info): Double quote to prevent globbing and word splitting. * SC2129: Consider using { cmd1; cmd2; } >> file instead of individual redirects. ``` shellcheck ci/scripts/ccache_setup.sh In ci/scripts/ccache_setup.sh line 22: echo "ARROW_USE_CCACHE=ON" >> $GITHUB_ENV ^-- SC2129 (style): Consider using { cmd1; cmd2; } >> file instead of individual redirects. ^---------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: echo "ARROW_USE_CCACHE=ON" >> "$GITHUB_ENV" In ci/scripts/ccache_setup.sh line 23: echo "CCACHE_COMPILERCHECK=content" >> $GITHUB_ENV ^---------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: echo "CCACHE_COMPILERCHECK=content" >> "$GITHUB_ENV" In ci/scripts/ccache_setup.sh line 24: echo "CCACHE_COMPRESS=1" >> $GITHUB_ENV ^---------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: echo "CCACHE_COMPRESS=1" >> "$GITHUB_ENV" In ci/scripts/ccache_setup.sh line 25: echo "CCACHE_COMPRESSLEVEL=6" >> $GITHUB_ENV ^---------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: echo "CCACHE_COMPRESSLEVEL=6" >> "$GITHUB_ENV" In ci/scripts/ccache_setup.sh line 26: echo "CCACHE_MAXSIZE=1G" >> $GITHUB_ENV ^---------^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: echo "CCACHE_MAXSIZE=1G" >> "$GITHUB_ENV" For more information: https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ... https://www.shellcheck.net/wiki/SC2129 -- Consider using { cmd1; cmd2; } >>... palolovalley:arrow hsato$ vi ci/scripts/ccache_setup.sh ``` ### What changes are included in this PR? * SC2086: Quoting like "$GITHUB_ENV" * SC2129: combine multiple commands using `{}` ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#46763 Authored-by: Hiroyuki Sato <hiroysato@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
This is the sub issue #44748.
What changes are included in this PR?
{}
Are these changes tested?
Yes.
Are there any user-facing changes?
No.