linux-automation / meta-lxatac

Build your own LXA TAC images and bundles
MIT License
5 stars 15 forks source link

CI: add a shellcheck job for all our shell scripts #86

Closed hnez closed 5 months ago

hnez commented 7 months ago

Pull Request #85 has brought the topic of shell scripts in meta-lxatac not passing shellcheck to my attention.

I consider even a shell script that passes shellcheck performing like the author intended a happy coincidence. One that does not pass shellcheck working correctly is a full blown miracle.

Make sure to check all scripts with shellcheck for each pull request.

Todo before merging:

hnez commented 7 months ago

The remaining issues are:

In meta-lxatac-software/recipes-rust/tacd/files/tacd-failsafe.sh line 6:
gpioset $(gpiofind DUT_PWR_EN)=1
        ^--------------------^ SC2046 (warning): Quote this to prevent word splitting.
          ^-----------------^ SC2312 (info): Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).

In meta-lxatac-software/recipes-rust/tacd/files/tacd-failsafe.sh line 7:
gpioset $(gpiofind DUT_PWR_DISCH)=0
        ^-----------------------^ SC2046 (warning): Quote this to prevent word splitting.
          ^--------------------^ SC2312 (info): Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).

In meta-lxatac-software/recipes-devtools/tac-gadget/files/gadget-common.sh line 19:
    elif [[ -s "${MAINDIR}"/*/UDC ]]; then
               ^----------------^ SC2144 (error): -s doesn't work with globs. Use a for loop.

The first two of these are fixed in #88. The last one requires some thinking and the fix is not as mechanical as the others.

hnez commented 5 months ago

Hi @Emantor,

do you think you could squeeze in a review for this PR? Otherwise I could try finding another shell scripting expert.

hnez commented 5 months ago

Hi @Emantor,

this is a rather large PR with no user-facing improvements, so I am fine with this not being on the fast track. But it would be cool to get it merged.