improve linting by benoit-pierre · Pull Request #12023 · koreader/koreader

benoit-pierre · 2024-06-13T19:45:25Z

Use base/utils/shellcheck.sh to ensure the same standard is used.

Additionally: remove some CI dead code, and fix issues reported by newer shellcheck / shfmt versions.

This change is

Frenzie · 2024-06-13T19:48:36Z

 PARTITION=${DISK}1p1

 [ -e "${PARTITION}" ] && mount -o "${MOUNT_ARGS}" -t vfat "${PARTITION}" /mnt/sd
-


I'd rather keep the newlines at end of file.

OK, but it's a shfmt issue (with a newer version: 3.7.0), so how do you tell it to ignore those?

Out of curiosity, what's the rational?

Out of curiosity, what's the rational?

Better diffs, consistency, etc.

I guess we can use --ignore-blank-lines in base/utils/shellcheck.sh

--- i/utils/shellcheck.sh +++ w/utils/shellcheck.sh @@ -40,7 +40,7 @@ shfmt() { } diff() { - local cmd=(command diff --unified) + local cmd=(command diff --ignore-blank-lines --unified) if [[ -n "${CLICOLOR_FORCE}" ]]; then cmd+=(--color=always) fi

It says here that "shfmt enforces trailing newlines"
mvdan/sh#733 (comment)

Complain how?

# as is $ shfmt_v3.8.0_linux_amd64 -i 4 enable-usbms.sh | diff enable-usbms.sh -

# no newline $ shfmt_v3.8.0_linux_amd64 -i 4 enable-usbms.sh | diff enable-usbms.sh - 33c33 < sleep 1 \ No newline at end of file --- > sleep 1

# two newline $ shfmt_v3.8.0_linux_amd64 -i 4 enable-usbms.sh | diff enable-usbms.sh - 34d33 <

It's disable-usbms.sh:

▸ tail -3 platform/cervantes/disable-usbms.sh [ -e "${PARTITION}" ] && mount -o "${MOUNT_ARGS}" -t vfat "${PARTITION}" /mnt/sd ▸ for v in 3.4.3 3.7.0 3.8.0; do ./bin/shfmt_v${v}_linux_amd64 -i 4 -ci platform/cervantes/disable-usbms.sh | diff -u platform/cervantes/disable-usbms.sh -; done --- platform/cervantes/disable-usbms.sh 2024-06-13 22:59:13.312754354 +0200 +++ - 2024-06-13 23:03:42.134574804 +0200 @@ -23,4 +23,3 @@ PARTITION=${DISK}1p1 [ -e "${PARTITION}" ] && mount -o "${MOUNT_ARGS}" -t vfat "${PARTITION}" /mnt/sd - --- platform/cervantes/disable-usbms.sh 2024-06-13 22:59:13.312754354 +0200 +++ - 2024-06-13 23:03:42.136661755 +0200 @@ -23,4 +23,3 @@ PARTITION=${DISK}1p1 [ -e "${PARTITION}" ] && mount -o "${MOUNT_ARGS}" -t vfat "${PARTITION}" /mnt/sd - --- platform/cervantes/disable-usbms.sh 2024-06-13 22:59:13.312754354 +0200 +++ - 2024-06-13 23:03:42.139547704 +0200 @@ -23,4 +23,3 @@ PARTITION=${DISK}1p1 [ -e "${PARTITION}" ] && mount -o "${MOUNT_ARGS}" -t vfat "${PARTITION}" /mnt/sd -

Ohhhkay, giant miscommunication. I thought you were removing the trailing newline but you were removing an additional improperly trailing newline.

But why aren't we seeing it in the output right now? As stated, every version behaves exactly the same in this regard.

Because of the way the check was done with [[ "$(cat t)" = "$($shfmt -i 4 -ci t)" ]], see comment above.

I thought you were removing the trailing newline but you were removing an additional improperly trailing newline.

This incidentally is exactly the reason it should be there, so as not to produce diffs that are distracting at best and misleading at worst. But clearly I should head off to bed. ;-)

benoit-pierre · 2024-06-13T21:17:38Z

The plot thickens…

Running shfmt on platform/cervantes/suspend.sh
Warning: platform/cervantes/suspend.sh does not abide by coding style, diff for expected style:
--- actual/platform/cervantes/suspend.sh
+++ expect/platform/cervantes/suspend.sh
@@ -12,4 +12,3 @@
 
 # Suspend to RAM.
 echo mem >/sys/power/state
-

Why is that file not being checked on my side? o.O

benoit-pierre · 2024-06-13T21:18:49Z

The fix:

 .ci/helper_shellchecks.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git i/.ci/helper_shellchecks.sh w/.ci/helper_shellchecks.sh
index f273d53e51..bdd69b3cb9 100755
--- i/.ci/helper_shellchecks.sh
+++ w/.ci/helper_shellchecks.sh
@@ -5,6 +5,6 @@ CI_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "${CI_DIR}/common.sh"
 
 # shellcheck disable=2016
-mapfile -t shellscript_locations < <({ git grep -lE '^#!(/usr)?/bin/(env )?(bash|sh)' | sed "/^plugins\/terminal.koplugin\/shfm$/d" && git submodule --quiet foreach '[ "$path" = "base" -o "$path" = "platform/android/luajit-launcher" ] || git grep -lE "^#!(/usr)?/bin/(env )?(bash|sh)" | sed "s|^|$path/|"' && git ls-files ./*.sh; } | sort | uniq)
+mapfile -t shellscript_locations < <({ git grep -lE '^#!(/usr)?/bin/(env )?(bash|sh)' | sed "/^plugins\/terminal.koplugin\/shfm$/d" && git submodule --quiet foreach '[ "$path" = "base" -o "$path" = "platform/android/luajit-launcher" ] || git grep -lE "^#!(/usr)?/bin/(env )?(bash|sh)" | sed "s|^|$path/|"' && git ls-files './*.sh'; } | sort | uniq)
 
 ./base/utils/shellcheck.sh "${shellscript_locations[@]}"

Different bash config I guess?

benoit-pierre · 2024-06-13T21:20:14Z

Base potentially suffers from the same issue.

benoit-pierre · 2024-06-13T21:24:43Z

Nope, some stray shell scripts at the root of the checkout:

▸ ls *.sh
 env.sh   pouet.sh

So the glob matches, and is not evaluated by git ls-files ….

- don't stop at the first error, report all issues - always run luacheck directly: so a luacheck install for another LUA version or using a shell script launcher works (the docker image wrapper already uses luajit) - don't disable colors in luacheck output - leave the terminal attributes to default at exit

Quote argument to `git ls-files`: ensure the glob is evaluated by git, not bash.

benoit-pierre requested a review from Frenzie as a code owner June 13, 2024 19:45

benoit-pierre marked this pull request as draft June 13, 2024 19:45

Frenzie reviewed Jun 13, 2024

View reviewed changes

Comment thread platform/linux/koreader.sh

Frenzie added this to the 2024.06 milestone Jun 13, 2024

Frenzie approved these changes Jun 13, 2024

View reviewed changes

benoit-pierre force-pushed the pr/shellcheck branch from e12b849 to 3d9312c Compare June 13, 2024 21:14

mergen3107 referenced this pull request Jun 13, 2024

Newline strikes back

1bb1ced

benoit-pierre mentioned this pull request Jun 15, 2024

ci: fix shellchecks helper koreader/koreader-base#1822

Merged

benoit-pierre added 6 commits June 15, 2024 22:54

bump base

8874936

ci: use base shellcheck helper

4533e75

ci: drop dead code

4ee021d

ci: fix shellchecks helper

924f7a0

Quote argument to `git ls-files`: ensure the glob is evaluated by git, not bash.

lint: fix issues reported by newer shellcheck / shfmt

a53d12e

benoit-pierre force-pushed the pr/shellcheck branch from 3d9312c to a53d12e Compare June 15, 2024 20:54

benoit-pierre marked this pull request as ready for review June 15, 2024 20:57

Frenzie merged commit 088ae7d into koreader:master Jun 15, 2024

benoit-pierre deleted the pr/shellcheck branch June 15, 2024 21:03

		PARTITION=${DISK}1p1

		[ -e "${PARTITION}" ] && mount -o "${MOUNT_ARGS}" -t vfat "${PARTITION}" /mnt/sd

Uh oh!

Uh oh!

Conversation

benoit-pierre commented Jun 13, 2024 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benoit-pierre commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benoit-pierre commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benoit-pierre commented Jun 13, 2024

Uh oh!

benoit-pierre commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benoit-pierre commented Jun 13, 2024 •

edited by Frenzie

Loading

benoit-pierre commented Jun 13, 2024 •

edited

Loading

benoit-pierre commented Jun 13, 2024 •

edited

Loading

benoit-pierre commented Jun 13, 2024 •

edited

Loading