-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-46909: [CI][Dev] Fix shellcheck errors in the ci/scripts/install_sccache.sh #46910
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-46909: [CI][Dev] Fix shellcheck errors in the ci/scripts/install_sccache.sh #46910
Conversation
|
|
|
Do we need set default This script allows one argument like But In this case, arrow/ci/scripts/install_sccache.sh Lines 29 to 57 in d97e7ab
|
raulcd
left a comment
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.
I would make PREFIX mandatory and require 2 arguments instead of 1.
We could also default to /usr/local/bin but that would be incorrect on Windows, as we already always call with 2 arguments we should make it mandatory in my opinion.
|
@raulcd Thank you for your comment. The second argument has been made mandatory. |
|
@raulcd Please take a look when you get a chance. |
assignUser
left a comment
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.
Thanks for the continues effort to fix up our scripts!
ci/scripts/install_sccache.sh
Outdated
| SHA_ARGS="--check --status" | ||
| SHA_ARGS=(--check --status) | ||
|
|
||
| # Busybox sha256sum uses different flags | ||
| if sha256sum --version 2>&1 | grep -q BusyBox; then | ||
| SHA_ARGS="-sc" | ||
| SHA_ARGS+=(-sc) |
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.
Oh this would have just overwritten each other before, nice catch!
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.
Hmm. I think that we need to overwrite --check --status with -sc because BusyBox based sha256sum doesn't accept --check --status.
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.
@assignUser @kou Thank you for your comment. I changed to SHA_ARGS="-sc".
|
@github-actions crossbow submit -g cpp |
|
Revision: 106aebc Submitted crossbow builds: ursacomputing/crossbow @ actions-402ea01d3f |
|
@github-actions crossbow submit -g cpp |
|
Revision: 61da45e Submitted crossbow builds: ursacomputing/crossbow @ actions-da7a3516e9 |
ci/scripts/install_sccache.sh
Outdated
|
|
||
| # Busybox sha256sum uses different flags | ||
| if sha256sum --version 2>&1 | grep -q BusyBox; then | ||
| SHA_ARGS="-sc" |
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.
| SHA_ARGS="-sc" | |
| SHA_ARGS=("-sc") |
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.
Thanks.
What do you think SHA_ARGS=(-sc)? Because The previous part SHA_ARGS=(--check --status) doesn't use double quote. If we use quote in this place, It seems better to change SHA_ARGS=("--check" "--status")
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.
(-sc) is fine here.
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.
Thanks. I changed to (-sc).
|
@github-actions crossbow submit -g cpp |
|
Revision: 2b91057 Submitted crossbow builds: ursacomputing/crossbow @ actions-b8a648159c |
|
It seems that CI failure unrelates to this PR. |
|
@github-actions crossbow submit example-cpp-tutorial test-ubuntu-22.04-cpp-bundled test-ubuntu-24.04-cpp-minimal-with-formats |
|
Revision: 2b91057 Submitted crossbow builds: ursacomputing/crossbow @ actions-95e52a850d
|
|
@github-actions crossbow submit test-ubuntu-22.04-cpp-bundled |
|
Revision: 2b91057 Submitted crossbow builds: ursacomputing/crossbow @ actions-1c22e82ca8
|
|
@kou All CIs passed. Please take a look when you get a chance. |
kou
left a comment
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 3ebe7ee. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 54 possible false positives for unstable benchmarks that are known to sometimes produce them. |
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.