Add ShellCheck and shfmt shell code quality analysis #2712

Frenzie · 2017-04-02T19:14:34Z

No description provided.

houqp · 2017-04-02T21:56:53Z

platform/kindle/koreader.sh

 	eips_print_bottom_centered "Starting KOReader . . ." 1
 fi

 # we keep maximum 500K worth of crash log
-cat crash.log 2> /dev/null | tail -c 500000 > crash.log.new
+< crash.log tail -c 500000 > crash.log.new


This will print out a misleading error message if crash.log does not exist. How about we change it to the following?

if [ -e crash.log ]; then tail -c 500000 crash.log > crash.log.new mv -f crash.log.new crash.log fi

Yes, I think that's much clearer and therefore better. Besides, as you pointed out the next line otherwise errors as well.

$ mv -f crash.log.new crash.log mv: cannot stat 'crash.log.new': No such file or directory

houqp · 2017-04-02T22:02:26Z

Nice! I saw this project the other day and was planning to add it to the travis CI. Thanks for doing this.

Frenzie · 2017-04-03T07:01:29Z

A related and potentially also interesting script is checkbashisms. It doesn't overlap completely with ShellCheck (which bases its recommendations on the shebang at the top of the file) because it points out that .ci/before_install.sh doesn't have any bashisms and can therefore be safely changed to /bin/sh.

Frenzie · 2017-04-03T10:49:16Z

.ci/script.sh

 source "${CI_DIR}/common.sh"

 travis_retry make fetchthirdparty
 make all
 make testfront
-luajit $(which luacheck) --no-color -q {reader,setupkoenv,datastorage}.lua frontend plugins
+luajit "$(which luacheck)" --no-color -q {reader,setupkoenv,datastorage}.lua frontend plugins
+find . -type f -name '*.sh' -not -path "./base/*" -not -path "./luajit-rocks/*" -print0 | xargs --null shellcheck


Would it be better to stick this directly after make fetchthirdparty? I think there's no functional reason for this to go all the way at the end and it'd error a lot quicker.

I was thinking about the same thing! Yes, makes total sense to not fun tests if static analysis is not even passing.

Frenzie · 2017-04-03T17:24:23Z

The stupid fail has nothing to do with this (Travis ran for 1+ hour? Didn't even know that was possible…). It's ready for a final review. I've already had a pass with the shellscript analysis at the end.

Frenzie · 2017-04-03T21:13:42Z

.ci/script.sh

 source "${CI_DIR}/common.sh"

 travis_retry make fetchthirdparty
+find . -type f -name '*.sh' -not -path "./base/*" -not -path "./luajit-rocks/*" -print0 | xargs --null shellcheck


It occurs to me that it might be better to grep for a few relevant shebangs as well. See, e.g., here which I came across while searching if there was a precompiled updated shellcheck.

echo 'Running shellcheck' git grep -l '^#!/usr/bin/env bash' | xargs shellcheck echo 'Running shfmt' git grep -l '^#!/usr/bin/env bash' | xargs shfmt -i 2 -w

I'm specifically thinking of kodev. But the find command is also important, because missing shebangs were a problem in some of the files.

houqp · 2017-04-04T15:21:01Z

Looks good to me 👍 But let's give it couple more days for testing since it touches many critical code path.

NiLuJe · 2017-04-07T17:18:34Z

platform/kindle/extensions/koreader/bin/koreader-ext.sh

@@ -115,11 +118,11 @@ set_cre_prop()
 	# Check that the config exists...
 	if [ -f "${cre_config}" ] ; then
 		# dos2unix
-		sed -e "s/$(echo -ne '\r')$//g" -i "${cre_config}"
+		sed -e "s/$(printf '\r')$//g" -i "${cre_config}"


Need to check how this actually behaves on devices

Yeah, that was okay the way it is and my change was mistaken because I forgot to escape '\r' properly. In this case it's easier to just disable the shellcheck warning (plus then no checking on device is required).

NiLuJe · 2017-04-07T17:20:54Z

platform/kindle/koreader.sh

@@ -125,7 +129,7 @@ export STARDICT_DATA_DIR="data/dict"
 export EXT_FONT_DIR="/mnt/us/fonts"

 # Only setup IPTables on evices where it makes sense to (FW 5.x & K4)
-if [ "${INIT_TYPE}" == "upstart" -o "$(uname -r)" == "2.6.31-rt11-lab126" ] ; then
+if [ "${INIT_TYPE}" = "upstart" ] || [ "$(uname -r)" = "2.6.31-rt11-lab126" ] ; then


Need to check

NiLuJe · 2017-04-07T17:21:18Z

platform/kindle/koreader.sh

 	fi
 fi

 # check if kpvbooklet was launched for more than once, if not we will disable pillow
 # there's no pillow if we stopped the framework, and it's only there on systems with upstart anyway
-if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "upstart" ] ; then
+if [ "${STOP_FRAMEWORK}" = "no" ] && [ "${INIT_TYPE}" = "upstart" ] ; then


Same as before

NiLuJe · 2017-04-07T17:21:24Z

platform/kindle/koreader.sh

 	count=$(lipc-get-prop -eiq com.github.koreader.kpvbooklet.timer count)
-	if [ "$count" == "" -o "$count" == "0" ] ; then
+	if [ "$count" = "" ] || [ "$count" = "0" ] ; then


NiLuJe · 2017-04-07T17:21:31Z

platform/kindle/koreader.sh

@@ -218,12 +224,12 @@ if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "upstart" ] ; then
 			PILLOW_SOFT_DISABLED="yes"
 		fi
 		# NOTE: We don't need to sleep at all if we've already SIGSTOPped awesome ;)
-		if [ "${NO_SLEEP}" == "no" -a "${AWESOME_STOPPED}" == "no" ] ; then
+		if [ "${NO_SLEEP}" = "no" ] && [ "${AWESOME_STOPPED}" = "no" ] ; then


NiLuJe · 2017-04-07T17:21:37Z

platform/kindle/koreader.sh

@@ -232,21 +238,23 @@ if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "upstart" ] ; then
 fi

 # stop cvm (sysv & framework up only)
-if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "sysv" ] ; then
+if [ "${STOP_FRAMEWORK}" = "no" ] && [ "${INIT_TYPE}" = "sysv" ] ; then


NiLuJe · 2017-04-07T17:22:19Z

platform/kindle/koreader.sh

@@ -280,7 +288,7 @@ if grep ${KOREADER_DIR}/fonts/linkfonts /proc/mounts > /dev/null 2>&1 ; then
 fi

 # Resume cvm (only if we stopped it)
-if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "sysv" ] ; then
+if [ "${STOP_FRAMEWORK}" = "no" ] && [ "${INIT_TYPE}" = "sysv" ] ; then


Frenzie · 2017-04-07T17:22:23Z

It's been changed to universal POSIX rather than a Bashism. But yes I can't check Kindle.

…

On Apr 7, 2017 19:18, "NiLuJe" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In platform/kindle/extensions/koreader/bin/koreader-ext.sh <#2712 (comment)>: > @@ -115,11 +118,11 @@ set_cre_prop() # Check that the config exists... if [ -f "${cre_config}" ] ; then # dos2unix - sed -e "s/$(echo -ne '\r')$//g" -i "${cre_config}" + sed -e "s/$(printf '\r')$//g" -i "${cre_config}" Need to check how this actually behaves on devices — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2712 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYBU7zONfm3RGLL0rzUtEKnAZCOtszks5rtm_rgaJpZM4Mw5lx> .

NiLuJe · 2017-04-07T17:22:25Z

platform/kindle/koreader.sh

 		cd / && env -u LD_LIBRARY_PATH /etc/init.d/framework start
 	else
 		cd / && env -u LD_LIBRARY_PATH start lab126_gui
 	fi
 fi

 # Display chrome bar if need be (upstart & framework up only)
-if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "upstart" ] ; then
+if [ "${STOP_FRAMEWORK}" = "no" ] && [ "${INIT_TYPE}" = "upstart" ] ; then


NiLuJe · 2017-04-07T17:22:31Z

platform/kindle/koreader.sh

@@ -325,16 +333,15 @@ if [ "${STOP_FRAMEWORK}" == "no" -a "${INIT_TYPE}" == "upstart" ] ; then
 	fi
 fi

-if [ "${INIT_TYPE}" == "upstart" -o "$(uname -r)" == "2.6.31-rt11-lab126" ] ; then
+if [ "${INIT_TYPE}" = "upstart" ] || [ "$(uname -r)" = "2.6.31-rt11-lab126" ] ; then


NiLuJe · 2017-04-07T17:23:18Z

platform/kobo/disable-wifi.sh

@@ -9,9 +9,9 @@ ifconfig eth0 down

 # Some sleep in between may avoid system getting hung
 # (we test if a module is actually loaded to avoid unneeded sleeps)
-if lsmod | grep -q $WIFI_MODULE ; then
+if lsmod | grep -q "$WIFI_MODULE" ; then


I'd probably prefer quotes AND curly braces, but that's just me ;).

I can change it, but note that unquoted vars are basically always problematic whereas a lack of curly braces is only problematic in the case of $array[key] (should be ${array[key]}).

But more to the point, are you aware of a tool that would auto-check for that if we did that?

Actually I think I prefer without curly braces because they're visual noise, although I don't feel very strongly about it. :-P

NiLuJe said he prefer both ;) I think it's better to keep it consistent to have the braces everywhere.

NiLuJe · 2017-04-07T17:23:26Z

platform/kobo/disable-wifi.sh

    usleep 200000
-    rmmod -r $WIFI_MODULE
+    rmmod -r "$WIFI_MODULE"


Same as just before

NiLuJe · 2017-04-07T17:25:00Z

platform/ubuntu-touch/koreader.sh

 fi

 # we're always starting from our working directory
-cd $KOREADER_DIR
+cd "$KOREADER_DIR" || exit


NiLuJe · 2017-04-07T17:25:06Z

platform/ubuntu-touch/koreader.sh

@@ -22,7 +22,7 @@ export LD_LIBRARY_PATH=${KOREADER_DIR}/libs:$LD_LIBRARY_PATH
 export TESSDATA_PREFIX="data"

 # export external font directory
-export EXT_FONT_DIR="~/fonts"
+export EXT_FONT_DIR="$HOME/fonts"


NiLuJe · 2017-04-07T17:26:34Z

Did a quick eyeballing, most of it looks okay, I mentioned a few personal formatting pet peeves, and there are a couple of things I'm not quite sure about and would like to check on actual devices, because I've been bitten by crappy busybox behavior before, and that's the reason some of those tests were written that way.. ;).

EDIT: I'm referring to the -a/-o switch. I actually vastly prefer the other syntax, so I definitely remember having to resort to that to fix some weird issues somewhere at some point...

Frenzie · 2017-04-07T18:35:59Z

EDIT: I'm referring to the -a/-o switch. I actually vastly prefer the other syntax, so I definitely remember having to resort to that to fix some weird issues somewhere at some point...

It's not a matter of preference. The behavior of -a and -o is insufficiently defined and is bound to lead to unintended consequences. It's objectively bad mojo to use it. Note that you can use && and || the standard way (as done here) and in a Bashism way (e.g., [ $asdasdfs && $asdfasdf ]). The latter will almost certainly lead to unintended consequences in non-Bash shells while [ $asdasdfs ] && [ $asdfasdf ] will not (unless BusyBox is truly, severely, extremely broken).

The basic idea is that writing scripts that'll run on sh from 20 years ago will work on everything, most important alternative shells like Dash and zsh, but also including @#$@#$ DIAF BusyBox. (Seriously, why is it used so much when the basic GNU utils are so minuscule by modern standards?) Besides that there are some things that are just always bad. (like -a and -o and unquoted variables). :-)

Frenzie · 2017-04-07T19:53:54Z

Btw, we could also enforce a specific style (4 spaces, tabs, line length) with an only slightly more complicated application of this principle:

[ "$(cat enable-wifi.sh )" != "$(shfmt enable-wifi.sh)" ] && shfmt ./enable-wifi.sh | diff enable-wifi.sh -

But that's not for this PR, if only because that'd drown the actual code quality issues at play here.

Frenzie · 2017-04-08T11:10:27Z

Okay, just the $PREFIX issue left.

* add a bunch of curly braces * change back Kindle BusyBox sed statement * kobo/koreader.sh: remove useless $PREFIX

Frenzie · 2017-04-08T19:29:31Z

.ci/script.sh

 make all
 make testfront
-luajit $(which luacheck) --no-color -q {reader,setupkoenv,datastorage}.lua frontend plugins
+luajit "$(which luacheck)" --no-color -q {reader,setupkoenv,datastorage}.lua frontend plugins


Actually now that I think about it is there a reason to stick this after make all? Shouldn't this also go directly after make fetchparty?

* Caching Luarocks should shave a minute off install process

Hzj-jie · 2017-04-09T07:29:04Z

This should fix #2740.

Frenzie · 2017-04-09T07:31:02Z

Would you do the honors? :-)

Frenzie · 2017-04-09T08:42:52Z

Bravely merging to get it into the next nightly.

houqp · 2017-04-10T05:40:33Z

platform/kobo/koreader.sh

 # shellcheck disable=SC2094
-case "$PRODUCT" in "dragon" | "dahlia" ) cat /sys/class/graphics/fb0/rotate > /sys/class/graphics/fb0/rotate ;; esac
+cat "/sys/class/graphics/fb0/rotate" > "/sys/class/graphics/fb0/rotate"


The original commit has case "$PRODUCT" in "dragon" | "dahlia" ), do we not care about that anymore?

Correct, I believe this more likely to be future-proof unless a third deviant behavior is added to the mix. On those two this line results in the desired orientation (because they give you the opposite of what you want) while on other devices it's just harmless repetition.

houqp requested changes Apr 2, 2017

View reviewed changes

Frenzie force-pushed the shellcheck branch 3 times, most recently from fc8dea0 to 3a63da0 Compare April 3, 2017 10:41

Frenzie changed the title ~~(WIP) Add ShellCheck code quality analysis~~ Add ShellCheck and shfmt shell code quality analysis Apr 3, 2017

Frenzie commented Apr 3, 2017

View reviewed changes

Add ShellCheck and shfmt shell code quality analysis

d0edeeb

Frenzie force-pushed the shellcheck branch from 3a63da0 to d0edeeb Compare April 3, 2017 13:02

Frenzie commented Apr 3, 2017

View reviewed changes

fix kodev shellcheck issue

2a0078e

Frenzie added the enhancement label Apr 4, 2017

houqp approved these changes Apr 4, 2017

View reviewed changes

Frenzie mentioned this pull request Apr 4, 2017

update luajit-launcher #2725

Closed

houqp mentioned this pull request Apr 7, 2017

Workaround to support new kobo firmwares (+4.0) without needing KSM8 #2731

Merged

NiLuJe reviewed Apr 7, 2017

View reviewed changes

platform/kobo/disable-wifi.sh Outdated

usleep 200000

rmmod -r $WIFI_MODULE

rmmod -r "$WIFI_MODULE"

Copy link

Member

NiLuJe Apr 7, 2017

Choose a reason for hiding this comment

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

Same as just before

NiLuJe reviewed Apr 7, 2017

View reviewed changes

Merge branch 'master' into shellcheck

0afd67a

Frenzie referenced this pull request in Frenzie/koreader Apr 8, 2017

More Kobo startup scripts tweaks...

7db252a

Frenzie force-pushed the shellcheck branch from cbcd591 to 80e7b4c Compare April 8, 2017 09:54

Frenzie force-pushed the shellcheck branch from 8d64a3d to 2b773b6 Compare April 8, 2017 14:40

Frenzie added 2 commits April 8, 2017 16:43

Shell scripts: address review comments

1196367

* add a bunch of curly braces * change back Kindle BusyBox sed statement * kobo/koreader.sh: remove useless $PREFIX

kobo/koreader.sh: fix rotation issue caused by koreader#2731

5e83bcf

Frenzie force-pushed the shellcheck branch from 2b773b6 to 5e83bcf Compare April 8, 2017 14:44

Frenzie commented Apr 8, 2017

View reviewed changes

Frenzie force-pushed the shellcheck branch 5 times, most recently from c3207cf to 1ce2850 Compare April 8, 2017 21:34

Travis: speed up

d7cdf14

* Caching Luarocks should shave a minute off install process

Frenzie force-pushed the shellcheck branch from 1ce2850 to d7cdf14 Compare April 8, 2017 21:53

Frenzie mentioned this pull request Apr 9, 2017

Kobo Aura HD display is upside down #2740

Closed

Frenzie merged commit e5bcdee into koreader:master Apr 9, 2017

Frenzie mentioned this pull request Apr 9, 2017

Travis: speed up by caching base and running luacheck earlier #2741

Merged

Frenzie deleted the shellcheck branch April 9, 2017 11:28

houqp reviewed Apr 10, 2017

View reviewed changes

Frenzie mentioned this pull request Apr 10, 2017

Regression: Wifi manager doesn't open on first attempt to connect Wi-Fi #2750

Closed

Add ShellCheck and shfmt shell code quality analysis #2712

Add ShellCheck and shfmt shell code quality analysis #2712

Conversation

Frenzie commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houqp commented Apr 2, 2017 • edited

Frenzie commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Apr 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houqp commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie commented Apr 7, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe commented Apr 7, 2017 • edited

Frenzie commented Apr 7, 2017 • edited

Frenzie commented Apr 7, 2017

Frenzie commented Apr 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hzj-jie commented Apr 9, 2017

Frenzie commented Apr 9, 2017

Frenzie commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houqp commented Apr 2, 2017 •

edited

Frenzie commented Apr 3, 2017 •

edited

NiLuJe commented Apr 7, 2017 •

edited

Frenzie commented Apr 7, 2017 •

edited