Skip to content

GH-46794: [CI][Dev] Fix shellcheck errors in the ci/scripts/csharp_test.sh #46795

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

Merged
merged 7 commits into from
Jun 18, 2025

Conversation

hiroyuki-sato
Copy link
Collaborator

@hiroyuki-sato hiroyuki-sato commented Jun 12, 2025

Rationale for this change

This is the sub issue #44748.

  • SC2155: Declare and assign separately to avoid masking return values.
  • SC2086: Double quote to prevent globbing and word splitting.
shellcheck ci/scripts/csharp_test.sh

In ci/scripts/csharp_test.sh line 33:
export PYTHONNET_PYDLL=$(${PYTHON} -m find_libpython)
       ^-------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.


In ci/scripts/csharp_test.sh line 35:
pushd ${source_dir}
      ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
pushd "${source_dir}"

For more information:
  https://www.shellcheck.net/wiki/SC2155 -- Declare and assign separately to ...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

What changes are included in this PR?

  • Declare and assign a variable separately.
  • Quoting like "${source_dir}"

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

Copy link

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

@github-actions github-actions bot added the awaiting review Awaiting review label Jun 12, 2025
@hiroyuki-sato
Copy link
Collaborator Author

It seems that test failure in "C# / AMD64 macOS 13 C# 8.0.x (pull_request)" unrelated this change.

https://github.com/apache/arrow/actions/runs/15611171739/job/43972190211?pr=46795#step:6:108

Passed!  - Failed:     0, Passed:  1450, Skipped:     0, Total:  1450, Duration: 5 s - Apache.Arrow.Tests.dll (net8.0)
[xUnit.net 00:00:03.30]     Apache.Arrow.Flight.Tests.FlightTests.EnsureCallRaisesRequestCancelled [FAIL]
  Failed Apache.Arrow.Flight.Tests.FlightTests.EnsureCallRaisesRequestCancelled [82 ms]
  Error Message:
   System.InvalidOperationException : Unable to get the status because the call is not complete.
  Stack Trace:
     at Grpc.Net.Client.Internal.GrpcCall`2.GetStatus()
   at Grpc.Net.Client.Internal.HttpClientCallInvoker.Callbacks`2.<>c.<.cctor>b__4_1(Object state)
   at Grpc.Core.AsyncCallState.GetStatus()
   at Grpc.Core.AsyncServerStreamingCall`1.GetStatus()
   at Grpc.Core.AsyncCallState.GetStatus()
   at Grpc.Core.AsyncServerStreamingCall`1.GetStatus()
   at Apache.Arrow.Flight.Tests.FlightTests.EnsureCallRaisesRequestCancelled() in /Users/runner/work/arrow/arrow/csharp/test/Apache.Arrow.Flight.Tests/FlightTests.cs:line 523
--- End of stack trace from previous location ---

@hiroyuki-sato hiroyuki-sato force-pushed the topic/shellcheck-csharp_test branch from aa5c72f to 92d6c33 Compare June 13, 2025 00:19
@hiroyuki-sato hiroyuki-sato changed the title GH-46794: [CI][Dev] fix shellcheck errors in the ci/scripts/csharp_test.sh [DNM] GH-46794: [CI][Dev] fix shellcheck errors in the ci/scripts/csharp_test.sh Jun 13, 2025
@hiroyuki-sato
Copy link
Collaborator Author

I confirmed that "C# / AMD64 macOS 13 C# 8.0.x (pull_request)" error relates to this PR.

Build passed without this change.
https://github.com/apache/arrow/actions/runs/15625785800/job/44019635324

I'm not sure why this change cause error yet.

diff --git a/ci/scripts/csharp_test.sh b/ci/scripts/csharp_test.sh
index a435a83525..f521afc01d 100755
--- a/ci/scripts/csharp_test.sh
+++ b/ci/scripts/csharp_test.sh
@@ -30,8 +30,9 @@ if [ -z "${PYTHON}" ]; then
   fi
 fi
 ${PYTHON} -m pip install pyarrow find-libpython
-export PYTHONNET_PYDLL=$(${PYTHON} -m find_libpython)
+PYTHONNET_PYDLL=$(${PYTHON} -m find_libpython)
+export PYTHONNET_PYDLL

-pushd ${source_dir}
+pushd "${source_dir}"
 dotnet test
 popd

@hiroyuki-sato
Copy link
Collaborator Author

"C# / AMD64 macOS 13 C# 8.0.x (pull_request)" succeed just added two echo statements.
https://github.com/apache/arrow/actions/runs/15627712157/job/44025027456?pr=46795

This reverts commit c050cc9.
@hiroyuki-sato
Copy link
Collaborator Author

The latest version is completely the same which is previous version. CI failure last time, but now passed.

C# / AMD64 macOS 13 C# 8.0.x (pull_request)C# / AMD64 macOS 13 C# 8.0.x (pull_request)

@hiroyuki-sato hiroyuki-sato changed the title [DNM] GH-46794: [CI][Dev] fix shellcheck errors in the ci/scripts/csharp_test.sh GH-46794: [CI][Dev] fix shellcheck errors in the ci/scripts/csharp_test.sh Jun 13, 2025
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for the great work you are doing with shellcheck! The previous failure seems unrelated to the change to me.
Just a small question but looks good to me.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 13, 2025
@hiroyuki-sato
Copy link
Collaborator Author

hiroyuki-sato commented Jun 13, 2025

@raulcd Thank you for your comment.

Now, I'm fixing files in the ci/scripts directory.
40/69 files passed.
Files with SCXXX(ex ci/scripts/cpp_build.sh) need to be modified.

After fixing all files in the ci/scripts direcotry, I'll try to change like ci/scripts/*.sh,
Then will fix cpp, dev and ...
Finally removed all of files part.

Here is rough calculation per directories.

for i in $( find . -name '*.sh' -print ) ; do   echo $i ;   shellcheck $i |     grep -E -o 'SC[0-9]+' |      sort -u |      perl -pe '{ s/\n/ /}END{ print("\n"); }' ; done
fail directory
1 c_glib
71 ci
64 cpp
44 dev
5 python
3 r
2 swift
ci/scripts/c_glib_build.sh

ci/scripts/c_glib_test.sh

ci/scripts/ccache_setup.sh

ci/scripts/conan_build.sh

ci/scripts/conan_setup.sh

ci/scripts/cpp_build.sh
SC1090 SC1091 SC2007 SC2086 SC2155 SC2223 SC2236 SC2242
ci/scripts/cpp_test.sh

ci/scripts/csharp_build.sh

ci/scripts/csharp_pack.sh

ci/scripts/csharp_test.sh
SC2086 SC2155
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_conda.sh

ci/scripts/install_dask.sh
SC2086 SC2102
ci/scripts/install_emscripten.sh

ci/scripts/install_gcs_testbench.sh
SC2068 SC2154 SC2206 SC2223 SC2309
ci/scripts/install_iwyu.sh

ci/scripts/install_minio.sh
SC2046 SC2086
ci/scripts/install_ninja.sh

ci/scripts/install_numba.sh
SC1091 SC2086
ci/scripts/install_numpy.sh

ci/scripts/install_pandas.sh

ci/scripts/install_python.sh

ci/scripts/install_sccache.sh
SC2086 SC2166
ci/scripts/install_spark.sh

ci/scripts/install_vcpkg.sh

ci/scripts/integration_arrow_build.sh

ci/scripts/integration_arrow.sh
SC2046 SC2086 SC2102 SC2223
ci/scripts/integration_dask.sh

ci/scripts/integration_hdfs.sh
SC2034 SC2086 SC2155
ci/scripts/integration_skyhook.sh
SC2034 SC2086 SC2115 SC2143
ci/scripts/integration_spark.sh

ci/scripts/matlab_build.sh

ci/scripts/msys2_setup.sh
SC2086 SC2206
ci/scripts/msys2_system_clean.sh

ci/scripts/msys2_system_upgrade.sh

ci/scripts/nanoarrow_build.sh

ci/scripts/python_benchmark.sh
SC1091 SC2086 SC2164
ci/scripts/python_build_emscripten.sh
SC1090 SC2086
ci/scripts/python_build.sh
SC1091 SC2034 SC2086 SC2223 SC2236
ci/scripts/python_sdist_build.sh

ci/scripts/python_sdist_test.sh
SC1091 SC2012 SC2086
ci/scripts/python_test_emscripten.sh
SC2012 SC2086
ci/scripts/python_test.sh
SC1091 SC2034 SC2086 SC2223
ci/scripts/python_wheel_macos_build.sh
SC2006 SC2086 SC2155 SC2223
ci/scripts/python_wheel_unix_test.sh

ci/scripts/python_wheel_xlinux_build.sh
SC2002 SC2006 SC2045 SC2086 SC2223
ci/scripts/r_build.sh

ci/scripts/r_deps.sh
SC2006 SC2027 SC2086 SC2223
ci/scripts/r_docker_configure.sh
SC2006 SC2046 SC2086 SC2223
ci/scripts/r_install_system_dependencies.sh
SC2006 SC2223
ci/scripts/r_revdepcheck.sh

ci/scripts/r_sanitize.sh
SC2086 SC2155 SC2223
ci/scripts/r_test.sh
SC1091 SC2086 SC2223
ci/scripts/r_valgrind.sh
SC2046 SC2086 SC2223
ci/scripts/r_windows_build.sh
SC2011 SC2034 SC2035 SC2046 SC2086 SC2155 SC2223
ci/scripts/release_test.sh

ci/scripts/ruby_test.sh

ci/scripts/rust_build.sh

ci/scripts/swift_test.sh
SC2035 SC2086
ci/scripts/util_download_apache.sh
SC2048 SC2181
ci/scripts/util_enable_core_dumps.sh

ci/scripts/util_free_space.sh

ci/scripts/util_log.sh

ci/scripts/util_wait_for_it.sh
SC2046 SC2064 SC2086 SC2128 SC2206

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jun 13, 2025
@assignUser
Copy link
Member

assignUser commented Jun 13, 2025

Thanks, great work!
Just making sure you know about this snippet to apply auto fixes (given the amount of files we still need to fix...)

for file in "${files[@]}"; do
  shellcheck -f diff $file | git apply
done

@hiroyuki-sato
Copy link
Collaborator Author

@assignUser Thank you for your advice. I'll try -f option.

Could someone merge this PR if this PR no problem?

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou changed the title GH-46794: [CI][Dev] fix shellcheck errors in the ci/scripts/csharp_test.sh GH-46794: [CI][Dev] Fix shellcheck errors in the ci/scripts/csharp_test.sh Jun 18, 2025
@kou kou merged commit e75ecda into apache:main Jun 18, 2025
22 of 23 checks passed
@kou kou removed the awaiting merge Awaiting merge label Jun 18, 2025
@github-actions github-actions bot added the awaiting merge Awaiting merge label Jun 18, 2025
@hiroyuki-sato hiroyuki-sato deleted the topic/shellcheck-csharp_test branch June 18, 2025 03:38
Copy link

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

There were 119 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge Awaiting merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants