Fixing some style issues in shell scripts with shellcheck

[tesuji]

There are some style issues in shell scripts in this repository.
ShellCheck is an useful tool to find bugs in shell scripts.
One could run git ls-files '*.sh' to find all shell script files and pipe the result to xargs shellcheck.
Or just fix it file-by-file.

Note that this issue doesn't talk about including shellcheck tests in CI.

@rustbot modify labels: E-easy E-mentor T-infra

[pta2002]

Can I have a go at this? I think I can work on it.

[tesuji]

Sure. But it's known that there are some false-positive suggestions in shellcheck.
Most of them should be caught by CI (by falling to execute).

[pta2002]

Alright, I'll have a look at what fails CI then!

[pta2002]

Just a question, I've reached src/ci/docker/scripts/android-start-emulator.sh, which starts with a shebang referencing #!/bin/sh instead of the usual #!/usr/bin/env bash, and then goes on to use &>, which shellcheck warns is not a thing on POSIX sh. Should this be changed to bash or is there a reason for this?

rust/src/ci/docker/scripts/android-start-emulator.sh

Line 1 in fc2daaa

#!/bin/sh

[pta2002]

Also, what should I do about the unused variables that pop up here and there? Some like TARGET seem like they might be nice keeping if not just for making the file follow the same structure as the others, but since they're unused I'm not sure if I should delete them.

[tesuji]

then goes on to use &>, which shellcheck warns is not a thing on POSIX sh

Just use 2>&1 instead. I don't know if android has bash, so let's keep it sh.

unused variables that pop up here and there

You could grep for the unused variables in src/ci.
Most of them should be passed via environment variables.
You could ignore unused var warnings.

[pta2002]

So if they're unused they should probably be exported then, no? The ones shellcheck is warning about aren't

[tesuji]

You could remove it. File a draft PR to see if it run successfully.

[sasurau4]

The previous PR seems abondoned, I'll try it.

[jyn514]

The previous PR seems abondoned, I'll try it.

@sasurau4 the PR wasn't abandoned, it was rejected because it wasn't clear the improvements were worth the potential breakage. I see in #78666 you only fixed warnings shellcheck marked as 'error's, that might have a better chance.

[sjakobi]

Is there still work to do here now that #78666 has been merged?

[JohnTitor]

Triage: #78666 fixed the shellcheck errors and I don't think it's much worth applying shellcheck's suggestions to our source, while fighting a lot of regressions, so I'm going to close the issue. Thanks!