#1040 Issue `closed`: Use ShellCheck for ReaR scripts static analysis¶

Labels: enhancement, cleanup, fixed / solved / done, ReaR Project

jsmeix opened issue at 2016-10-19 12:38:¶

Currently rear scripts are only checked with "bash -n".

This is rather insufficient because "bash -n" detects only
hard syntax errors but not at all any of the zillions of bad things
that make bash scripts fail during runtime.

Hereby I will try to find out if ShellCheck could be used
for rear scripts static analysis so that we can find more
bugs before our users get hit by them during runtime.

For ShellCheck
https://github.com/koalaman/shellcheck

thomasmerz commented at 2022-02-04 16:29:¶

Hi @jsmeix , please have a look at my PR https://github.com/rear/rear/pull/2754 and review and merge if everything is fine for you. Kind regards from a valued SLES customer who's also fine with openSUSE 😉

pcahyna commented at 2022-02-04 16:52:¶

There are people working on this now (@antonvoznia. @thomasmerz). I propose to define some acceptance criteria here first.

thomasmerz commented at 2022-02-04 18:28:¶

I usually fix scripts top-down, straight forward and let some experts check/test the shellchecked version. Acceptance criteria should be that rear and the test-scripts are still doing what they expected to do 😉

I thought that those test-scripts might exist for exactly this: to ensure that any change don't break anything. But I don't know how to use them / how you use them to test/check any changes. It might also be reasonable to have a GitHub Action for tests… My PR #2754 can be used as a template for another testing-action for every change in any script…

Do you agree or do you need more details on some acceptance criteria?

thomasmerz commented at 2022-02-04 20:54:¶

@pcahyna , please review my PR https://github.com/rear/rear/pull/2756
Do you like one commit per changed script in the final PR or should I rebase and force-push into one single commit?

pcahyna commented at 2022-02-09 22:49:¶

Acceptance criteria should be that rear and the test-scripts are still doing what they expected to do

@thomasmerz Sorry for the late reply - I meant acceptance criteria for the automation (CI) that will run the shellcheck tests, not for the changes that will fix the problems.

My acceptance criteria for this automation are:

show only newly introduced problems, not all problems for a given PR. The number of ShellCheck-detected problems in current code is very high. One can not expect to have all of them fixed soon. At the same time, PR submitters and reviewers certainly do not want to be bothered by hundreds of errors in old code, they need to focus on what changed.
do not produce too many false positives (subjective, but if there is some warning that occurs all the time withous singifying a real problem, it is preferable to turn it off).
nice to have UI feature, not strictly necessary, but highly desirable: show the problems in the web UI in the diff view at the lines where the problems are detected.

From my POV especially the first item is a must have and I don't think your proposed solution achieves this.
Now the problem is how to coordinate: @antonvoznia needs to know whether to work on this subject in his diploma thesis or on something else. If he works on this, we plan to have it ready in about half a year (not because it is so difficult, but because other topics have higher priority). Of course, I understand that you or someone else might come with a solution sooner.

@antonvoznia can you please link an example PR to show the solution(s) you have been researching, so that others can see how the UX looks like?

jsmeix commented at 2022-02-10 07:25:¶

I would like to emphasise the "no false positives" acceptance criteria:

In the past I gave up on bash checking tools because the few I tested shortly
produced tons of false positives so all had been useless for me "out of the box".

In particular - as far as I remember - they produced many coding style warnings
which are no errors but mostly a matter of taste how one writes code, cf.
https://schlomo.schapiro.org/2015/04/warning-is-waste-of-my-time.html

Then they often detect things which they show as a problem but actually
the code is intentionally as is because the tool cannot understand the
meaning of code but only apply dumb static checking rules.

As far as I remember one can manually configure at least some of them
to exclude certain tests (e.g. by special comments in the code to skip
certain checks at specific code places) but I didn't had interest in that
when the default results tons of false positives so I would have to sort out
tons of issues before I could get something that might be useful in practice for me.
Simply put:
When taming such tools costs me more time
than making and testing my code
then it is not worth the effort.

My primary use case is detecting accidentally misspelled identifiers
like variables or functions or programs and things like that
i.e. typo detection, cf. https://github.com/rear/rear/issues/549
because I can only test one or a few main paths of the code
but not all the special cases and error handling paths.

Bottom line:
A code checking tool must provide me real net value
that actually helps me to avoid errors in my code.
I don't want some patronizing wisenheimer bot.

thomasmerz commented at 2022-02-10 10:23:¶

@pcahyna and @antonvoznia

show only newly introduced problems, not all problems for a given PR. The number of ShellCheck-detected problems in current code is very high. One can not expect to have all of them fixed soon.

"One" (man or machine) can - please review my PR #2756 😜

thomasmerz commented at 2022-02-10 10:27:¶

@jsmeix

In the past I gave up on bash checking tools because the few I tested shortly produced tons of false positives so all had been useless for me "out of the box".

In particular - as far as I remember - they produced many coding style warnings which are no errors but mostly a matter of taste how one writes code, cf.

I agree. But this can easily solved because shellcheck could only warn above a specific "Minimum severity of errors to consider (error, warning, info, style)" so that only important warnings and errors are shown. I also dislike info and even style because I sometimes use my "own style" 🤣

In my PR #2754 I already kept this in mind before you even spoke it out loudly:

--severity=warning --exclude=SC1091,SC1090

Details for excludes:
https://github.com/koalaman/shellcheck/wiki/SC1090
https://github.com/koalaman/shellcheck/wiki/SC1091

Plus: shellcheck can be disabled for each single occurrence where "one" knows it better (or wants to ignore the warning or because a solution is to hard to find…) which is already done:
https://github.com/rear/rear/search?q=shellcheck&type=code

pcahyna commented at 2022-02-10 11:35:¶

@pcahyna and @antonvoznia

show only newly introduced problems, not all problems for a given PR. The number of ShellCheck-detected problems in current code is very high. One can not expect to have all of them fixed soon.

"One" (man or machine) can - please review my PR #2756 stuck_out_tongue_winking_eye

I have seen shellcheck output on ReaR previously and I am afraid that you have seriously underestimated the amount of errors / warnings that it will produce. I am quite sure that the 14 changes proposed in the PR will be far from enough. In particular, I suspect ShellCheck has not run for virtually any script under usr/share/rear/ where most of the program logic resides (don't know why usr/share/rear/lib/write-protect-functions.sh is an exception).

jsmeix commented at 2022-02-10 11:54:¶

Only FYI:

# find usr/sbin/rear usr/share/rear/ -type f | xargs grep -i 'shellcheck'

usr/share/rear/output/USB/Linux-i386/100_create_efiboot.sh:# and https://github.com/koalaman/shellcheck/wiki/SC2155
usr/share/rear/restore/BORG/default/400_restore_backup.sh:# shellcheck disable=SC2168
usr/share/rear/restore/BORG/default/300_load_archives.sh:# shellcheck disable=SC2168
usr/share/rear/restore/BORG/default/300_load_archives.sh:# shellcheck disable=SC2168
usr/share/rear/restore/BORG/default/300_load_archives.sh:        # shellcheck disable=SC2034
usr/share/rear/verify/BORG/default/400_check_archive_access.sh:# shellcheck disable=SC2154
usr/share/rear/verify/BORG/default/400_check_archive_access.sh:            # shellcheck disable=SC2154
usr/share/rear/verify/BORG/default/400_check_archive_access.sh:            # shellcheck disable=SC2015
usr/share/rear/lib/borg-functions.sh:        # shellcheck disable=SC2034
usr/share/rear/lib/borg-functions.sh:    # shellcheck disable=SC2154
usr/share/rear/lib/borg-functions.sh:    # shellcheck disable=SC2086,SC2154
usr/share/rear/lib/borg-functions.sh:    # shellcheck disable=SC2086
usr/share/rear/lib/borg-functions.sh:    # shellcheck disable=SC2086
usr/share/rear/backup/BORG/default/900_umount_usb.sh:    # shellcheck disable=SC2154
usr/share/rear/backup/BORG/default/800_prune_old_backups.sh:# shellcheck disable=SC2168
usr/share/rear/backup/BORG/default/500_make_backup.sh:# shellcheck disable=SC2168
usr/share/rear/backup/BORG/default/500_make_backup.sh:# shellcheck disable=SC2168
usr/share/rear/prep/BORG/default/300_init_archive.sh:# shellcheck disable=SC2168
usr/share/rear/prep/BORG/default/300_init_archive.sh:            # shellcheck disable=SC2086,2154
usr/share/rear/prep/BORG/default/100_set_vars.sh:    # shellcheck disable=SC203

I don't want zillions of such comments needed in the code.

On first glance all or most of the above seem to come
from one single pull request
https://github.com/rear/rear/pull/2382
cf.
https://github.com/rear/rear/pull/2382/files

pcahyna commented at 2022-02-10 11:59:¶

The person writing the BORG integration obviously made use of shellcheck, but virtually no other contributor did.

pcahyna commented at 2022-02-10 12:00:¶

and yes, such comments are annoying. If a particular idiom is used in ReaR without being a problem, it should be disabled in shellcheck config.

jsmeix commented at 2022-02-10 12:13:¶

That's what I meant:
A single pull request caused already noticeable effort for ShellCheck
which basically proves my above
https://github.com/rear/rear/issues/1040#issuecomment-1034576705

Probably major work is neded to have a general ShellCheck
in a sufficiently well preconfigured state that it is of actual help.

As time permits I might retry how ShellCheck behaves
for all our scripts in usr/share/rear

jsmeix commented at 2022-02-10 12:34:¶

Was easier than expected because
there is a RPM package ShellCheck for openSUSE Leap 15.3

# for s in usr/sbin/rear $( find usr/share/rear/ -type f -name '*.sh' ) ; do shellcheck $s ; done | grep -o 'SC....:' | wc -l
7187

# for s in usr/sbin/rear $( find usr/share/rear/ -type f -name '*.sh' ) ; do shellcheck $s ; done | grep -o 'SC....:' | sort | uniq -c | wc -l
88

# for s in usr/sbin/rear $( find usr/share/rear/ -type f -name '*.sh' ) ; do shellcheck $s ; done | grep -o 'SC....:' | sort | uniq -c | sort -n
      1 SC1066:
      1 SC2000:
      1 SC2018:
      1 SC2019:
      1 SC2030:
      1 SC2031:
      1 SC2050:
      1 SC2066:
      1 SC2076:
      1 SC2091:
      1 SC2103:
      1 SC2119:
      1 SC2120:
      1 SC2178:
      1 SC2179:
      1 SC2221:
      1 SC2222:
      1 SC2235:
      2 SC1001:
      2 SC1075:
      2 SC1087:
      2 SC2003:
      2 SC2038:
      2 SC2063:
      2 SC2126:
      2 SC2143:
      2 SC2218:
      2 SC2229:
      2 SC2234:
      3 SC1007:
      3 SC1097:
      3 SC2059:
      3 SC2064:
      3 SC2140:
      3 SC2223:
      4 SC2010:
      4 SC2016:
      4 SC2053:
      4 SC2115:
      4 SC2125:
      4 SC2199:
      5 SC2029:
      5 SC2089:
      5 SC2090:
      6 SC1091:
      6 SC2021:
      6 SC2045:
      6 SC2094:
      6 SC2129:
      7 SC2009:
      7 SC2027:
      7 SC2043:
      7 SC2044:
      7 SC2124:
      8 SC2005:
      9 SC2012:
      9 SC2013:
      9 SC2188:
     16 SC2116:
     16 SC2153:
     20 SC2174:
     21 SC2236:
     26 SC2002:
     27 SC2001:
     30 SC1090:
     32 SC2006:
     33 SC2004:
     34 SC2166:
     34 SC2196:
     35 SC2181:
     35 SC2231:
     40 SC2068:
     59 SC2128:
     60 SC2145:
     63 SC2219:
     65 SC2254:
     69 SC2015:
     75 SC2164:
    122 SC2206:
    131 SC2207:
    138 SC2046:
    192 SC2154:
    206 SC2162:
    233 SC2155:
    316 SC2034:
    571 SC2168:
    574 SC2148:
   3717 SC2086:

so by default shellcheck found 7187 issues
that belong to 88 different cases

jsmeix commented at 2022-02-10 12:40:¶

I expected it!
https://github.com/koalaman/shellcheck/wiki/SC2086
is "Double quote to prevent globbing and word splitting"

I assume in very most cases we know that quoting is not needed like

foo="something"
grep $foo file

and in some cases we even intentionally do not quote like

# Ensure $foo is a single non empty and non blank word
# (no quoting because test " " returns zero exit code):
test $foo ...

Have fun with checking 3717 code places
whether or not quoting is right at each of them ;-)

jsmeix commented at 2022-02-10 12:57:¶

But the SCnnnn that appear rarely may more likely indicate real errors
like the one SC1066

In usr/share/rear/prep/Linux-s390/305_include_s390_tools.sh line 7:
test -d "$bootdir" || $bootdir='/boot/'
                       ^-- SC1066: Don't use $ on the left side of assignments.

where the code is

local bootdir="$( echo -n /boot/ )"
test -d "$bootdir" || $bootdir='/boot/'

I do not understand what the intent behind that code is,
cf. https://github.com/rear/rear/wiki/Coding-Style :-(

By chance I see now (Feb 23 2022) where that code comes from:
See the code in prep/GNU/Linux/300_include_grub_tools.sh
that sets grubdir via

local grubdir="$( echo -n /boot/grub* )"

where 'shopt -s nullglob' results nothing when nothing matches
but that is not needed here to set a fixed bootdir="/boot"
so I simplified it and fixed that ShellCheck issue SC1066
"Don't use $ on the left side of assignments" via
https://github.com/rear/rear/commit/ea0a74bd50493c7472600e2a523b06ae62ee508c

pcahyna commented at 2022-02-10 13:20:¶

But the SCnnnn that appear rarely may more likely indicate real errors like

That's my experience as well, ShellCheck has sometimes found real issues. So, I would prefer to relax the "no false positives" to a more pragmatic "benefits shall outweight costs". Of course, the judgment will be subjective.

Have fun with checking 3717 code places
whether or not quoting is right at each of them ;-)

The beauty of my first acceptance criterion "show only newly introduced problems, not all problems for a given PR. " (cf. what I wrote "The number of ShellCheck-detected problems in current code is very high. One can not expect to have all of them fixed soon") is that you don't have to check the 3717 places. If done properly, the check will be done only on newly added code, and you won't see warning for the old code at all. This way, the quality of the code will monotonically improve (or at least, not decrease) without too much additional effort.
Of course, it is not perfect, because even in newly added code they may be valid uses of the construct that ShellCheck is complaining about. That said, you will need to review them only once and you will not need to add any # shellcheck disable clutter, because the next run will ignore them. But if a particular false positive occurs too often, one may disable it globally.

jsmeix commented at 2022-02-10 14:21:¶

I wrote "no false positives" only to emphasise that
the absolute number of false positives is the real problem
because human time is the precious thing.

Of course in practice the pragmatic "benefits shall outweight costs" matters.

Or even more to the point: Efficiency is what actually matters.
When a human spends N hours on ReaR what matters in the end is
that his time results maximum benefit for ReaR.
For example it could be better to spend ones time
to improve the code to care more about possible errors
(not ShellCheck errors but e.g. missing error handling code)
cf. "Try hard to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style
than fixing ShellCheck issues.

thomasmerz commented at 2022-02-10 14:59:¶

Hi all,

I want to make the following suggestion and go step-by-step before we're stuck in the "hell of bureaucracy":

implement a user-friendly github-action for shellcheck
which only shecks warnings and errors
with my already shellcheck'ed scripts (I indeed didn't check all "files" due to the fact, that my PR is only shellcheck'ing files that match file --brief --mime-type "$sh", which means: file must be a shell-script)
This should be a very good and solid base for upcoming PRs and more shellcheck'ed files. Your ( @jsmeix ) list includes even info and style from shellcheck, which could/should be ignored (as I also understand from you?). If you let shellcheck run on all files you will get even much more absolutely nonsense warnings due to checking Makefiles, "example"-files, text-files, doc(umentation), config-files for network-setup, Buildfiles, scripts without a valid shebang (SC2148), … and so on…
after my two connected PRs feel free to let @antonvoznia deepdive and cleanup some more excluded SC-codes with severity warning and above.
if you do not set repo-settings to benefit of the shellcheck action like this - nothing changes for any PR. It will only be transparent that shellcheck is complaining (but my first setup won't annoy anyone if both PRs are merged):

So, are you still interested in getting a github-action for shellcheck "for free" with a first-round-shellcheck'ed set of many (but not all) scripts? 🤔

thomasmerz commented at 2022-02-10 15:02:¶

We really would be faster achieve an improvement by going small(er) steps and not a single huge one with 1000s of changes. My PRs are fitting together and are solving the problem that no automatic shellcheck is available for PRs and pushes to any files in the defined "paths".

thomasmerz commented at 2022-02-10 15:05:¶

BTW: I'm also on openSUSE Leap 15.3 since 12.x, but I prefer the docker-container to the SUSE repos because it's using always latest version. Leap-Repos are "sometimes" a little bit behind the latest versions.

thomasmerz commented at 2022-02-10 15:30:¶

@jsmeix

I expected it! https://github.com/koalaman/shellcheck/wiki/SC2086 is "Double quote to prevent globbing and word splitting"

I assume in very most cases we know that quoting is not needed like
…
and in some cases we even intentionally do not quote like
…
Have fun with checking 3717 code places whether or not quoting is right at each of them ;-)

You could let shellcheck ignore (informational-only?) SC2086 by a single line in .shellcheckrc or directly on the commandline for all 3717 occurrences. Let's concentrate on real warnings and errors.

If it would be helpful we could also discuss this issue via phone - I or you could ask some SUSE-collegues we know if they might help both of us (Sascha Weber or Ralph Roth I know "well") to find together.

pcahyna commented at 2022-02-10 15:56:¶

If you let shellcheck run on all files you will get even much more absolutely nonsense warnings due to checking (...) scripts without a valid shebang

Unfortunately, scripts without a valid shebang comprise most of ReaR's codebase. While the warning about missing shebang is indeed nonsense for them, by excluding the scripts completely one gets rid of most of the useful warnings together with the nonsense ones.

thomasmerz commented at 2022-02-10 16:51:¶

@pcahyna I understand. But what about my suggestion and start "small"? Let us be growing with the tasks :-)

jsmeix commented at 2022-02-11 09:05:¶

Yes,
step by step,
first things first,
no rush.

What I will do - as time permits - is looking throught the issues in
https://github.com/rear/rear/issues/1040#issuecomment-1034870262
to get some basic understanding how ShellCheck behaves in practice
for the ReaR code and how to control it to make it possibly useful.

I.e. first I will maually use ShellCheck and when I understand things
then I can start thinking about if and how that could be automated.

Finally we can decide if we could have an automated ShellCheck
i.e. when the automated ShellCheck benefits outweigh the costs.

jsmeix commented at 2022-02-11 09:11:¶

By the way:
I am in homeoffice.
I have no company phone.
I give my private phone number only to private contacts.
In general phone calls exclude others (in contrast to e.g. public GitHub issues).
In general phone calls and things like that are against
https://xahteiwi.eu/resources/presentations/no-we-wont-have-a-video-call-for-that/
or for those who prefer video over text
https://www.youtube.com/watch?v=NVnci3tyDa4

thomasmerz commented at 2022-02-11 09:34:¶

By the way:

Ok, I respect this, everything is fine for me. This was just an offer to clarify some possible misunderstandings on both sides by a talk (which is sometimes easier than writing).

jsmeix commented at 2022-02-16 09:32:¶

I won't find noticeable time for this issue until ReaR 2.7 was released, cf.
https://github.com/rear/rear/issues/2751
so I set this issue's milestone to ReaR v2.8 i.e. after ReaR 2.7 was released.

antonvoznia commented at 2022-03-06 14:35:¶

Hello all,

I apologize for the late reply.

Firstly I want to mention that I can solve the static analysis (whether via ShellCheck nor) for ReaR as part of my diploma's subject.
But I will start in 2-3 months because I currently have another task as a priority.

@thomasmerz Please let me know if you'll wait or you are going to solve it right now that I won't waste time.

However, I've already tried something with CheckShell and existed GitHub actions from there:
https://github.com/marketplace?type=actions&query=shellcheck+

One good possible solution (of course subjectively) is reviewdog:
https://github.com/marketplace/actions/run-shellcheck-with-reviewdog

There you can take a look at the test PR:
https://github.com/antonvoznia/rear/pull/33
or
https://github.com/antonvoznia/rear/pull/10

As mentioned @pcahyna needed criteria above the reviewdog partionatly fits them:

"show only newly introduced problems, not all problems for a given PR."
You can see that reviewdog adds notes only for new or changed code lines .
Reviewdog doesn't write a lot of errors and warnings just because a developer had changed a file (it should be better investigated, please see below conclusion, sometimes the statement is incorrect).
"nice to have UI feature".
The reviewdog adds for wrong code description of a warning or an error and the link to the wiki page of ShellCheck documentation.
It is a good feature and gets more quickly to review the code.

With reviewdog still exist some problems:

"do not produce too many false positives" - sometimes reviewdog checks the code with respect to the ShellCheck standard (not ReaR code style standard).
It would be a problem and make some stuff for ReaR contributors.
sometimes it acts unexpectedly:
https://github.com/antonvoznia/rear/pull/11/files
Here reviewdog marked lines as bad, but actually, the lines weren't changed by me.

As I wrote, I can further investigate ShellCheck and reviewdog (or something another) in 2-3 months.

Feel free to ask me for something.

Thanks,
Anton

jsmeix commented at 2022-03-07 13:45:¶

As expected:
Starting with the SCnnnn that appear rarely reveals more likely real errors
that can be fixed with reasonable efficiency because of less false positives.
But it is still some major pain to deal with arbitrary code places
where it is often unknown (i.e. undocumented) what that code is meant to do
so reverse engineering with much imagination is needed.
For examples see my recent commits that are mentioned here just above
and below.

thomasmerz commented at 2022-03-07 16:47:¶

@antonvoznia

@thomasmerz Please let me know if you'll wait or you are going to solve it right now that I won't waste time.

I'm just a contributor and not related to the ReaR project other than using ReaR intensively at work and at home. So @schlomo and @jsmeix and others have to decide what and how to continue with shellcheck.

My latest updates include:

a shellcheck solution that doesn't annotate directly in GitHub GUI, but that can also be highly optimized what files to check. If someone prefers this GitHub annotation thing than one of the linked solutions from you should really fit much better. So I'm also fine with this "more modern" solution, but personally I like it this way, too.
some shellcheck'ed scripts that fit to the PR mentioned before. Because of the way what files are chosen for shellcheck'ing it's not completely checking all scripts/files of ReaR. This would be a really good base to start with, as it is also highly discussed and many suggestions from @schlomo for example I've been taking care of.

I really don't want to "steal" or "crash" your thesis. But it would be a pity to throw my pre-work away without paying some respect 🤷🏻‍♂️

jsmeix commented at 2022-03-09 09:07:¶

In my above
https://github.com/rear/rear/issues/1040#issuecomment-1034870262
the items

      1 SC2119:
      1 SC2120:

are

In usr/share/rear/skel/default/etc/scripts/system-setup.d/00-functions.sh line 9:
        my_udevsettle
        ^-----------^ SC2119: Use my_udevsettle "$@" if function's $1 should mean script's $1.

In usr/share/rear/skel/default/etc/scripts/system-setup.d/00-functions.sh line 15:
my_udevsettle() {
^-- SC2120: my_udevsettle references arguments, but none are ever passed.

where both point to
https://github.com/koalaman/shellcheck/wiki/SC2120
that reads

If the parameters are optional and you currently just don't want to use them,
you can ignore this message.

and I think this is the case here so I will add

# shellcheck disable=SC2119
# shellcheck disable=SC2120

comments at that code places.

jsmeix commented at 2022-03-09 13:41:¶

During my recent commits that show up here since
https://github.com/rear/rear/issues/1040#issuecomment-1041290436
I got some initial understanding how far ShellCheck could be useful for ReaR:

First and foremost my general finding from my current (limited) point of view:
ShellCheck is much more useful for ReaR than I had expected.
Much more often than expected things that ShellCheck reports
point to real issues (a.k.a. "bugs") in our code that really need to be fixed.

BUT:
Fixing things that ShellCheck reports means much more work than I had expected
because what ShellCheck reports is (of course) in most cases only some syntactical
"strange looking code" that can somehow indirectly points to a real issue
but what ShellCheck reports is in most cases not the real issue itself
so only "fixing" what ShellCheck directly reports would often make things worse
because then the real issue would get concealed behind "well looking code".

SO:
Fixing things that ShellCheck reports means one has to investigate
what the reported "strange looking code" is actually meant to do
which often requires reverse engineering and that in most cases
even with things which one does not have on one's own computer
so one can neither reproduce the real issue nor verify one's fix,
cf. https://github.com/rear/rear/issues/1040#issuecomment-1060704029

RESULT:
From my current (limited) point of view first and foremost ShellCheck reports
that are limited to only the currently changed/added code would help ReaR
because this way the one who makes some new "strange looking code"
should be able to see the real issue behind and fix that properly
because he knows what his current code is actually meant to do
(or at least the area of that code is in his current focus of mind).

jsmeix commented at 2022-03-09 13:55:¶

Regarding how to use ShellCheck generally in ReaR
i.e. how to use ShellCheck also for old code in ReaR:

With current GitHub master code I did:

# for e in error warning info style ; \
do for s in usr/sbin/rear $( find usr/share/rear/ -type f -name '*.sh' ) ; \
   do shellcheck -s bash -S $e $s ; \
   done >shellcheck.$e.out ; \
done

# for e in error warning info style ; \
do echo shellcheck.$e.out ; \
   grep -o 'SC....:' shellcheck.$e.out | wc -l ; \
done

shellcheck.error.out
685
shellcheck.warning.out
2192
shellcheck.info.out
6313
shellcheck.style.out
6588

# for e in error warning info style ; \
do read -p shellcheck.$e.out ; \
   grep -o 'SC....:' shellcheck.$e.out | sort | uniq -c | sort -n ; \
   echo ; \
done

shellcheck.error.out
      1 SC2066:
      2 SC1087:
      2 SC2218:
      3 SC1097:
      3 SC2199:
      4 SC2045:
     40 SC2068:
     59 SC2145:
    571 SC2168:

shellcheck.warning.out
      1 SC2066:
      1 SC2091:
      2 SC1087:
      2 SC2038:
      2 SC2063:
      2 SC2218:
      2 SC2229:
      3 SC1007:
      3 SC1097:
      3 SC2064:
      3 SC2140:
      3 SC2199:
      4 SC2010:
      4 SC2053:
      4 SC2115:
      4 SC2125:
      5 SC2089:
      5 SC2090:
      6 SC2045:
      7 SC2027:
      7 SC2043:
      7 SC2044:
      7 SC2124:
      9 SC2188:
     16 SC2153:
     20 SC2174:
     30 SC1090:
     34 SC2166:
     40 SC2068:
     57 SC2128:
     59 SC2145:
     65 SC2254:
     75 SC2164:
    121 SC2206:
    130 SC2207:
    138 SC2046:
    192 SC2154:
    232 SC2155:
    316 SC2034:
    571 SC2168:

shellcheck.info.out
      1 SC2066:
      1 SC2091:
      2 SC1001:
      2 SC1087:
      2 SC2038:
      2 SC2063:
      2 SC2218:
      2 SC2229:
      3 SC1007:
      3 SC1097:
      3 SC2059:
      3 SC2064:
      3 SC2140:
      3 SC2199:
      3 SC2223:
      4 SC2010:
      4 SC2016:
      4 SC2053:
      4 SC2115:
      4 SC2125:
      5 SC2029:
      5 SC2089:
      5 SC2090:
      6 SC1091:
      6 SC2021:
      6 SC2045:
      6 SC2094:
      7 SC2009:
      7 SC2027:
      7 SC2043:
      7 SC2044:
      7 SC2124:
      9 SC2012:
      9 SC2013:
      9 SC2188:
     16 SC2153:
     20 SC2174:
     30 SC1090:
     34 SC2166:
     34 SC2196:
     35 SC2231:
     40 SC2068:
     57 SC2128:
     59 SC2145:
     65 SC2254:
     69 SC2015:
     75 SC2164:
    121 SC2206:
    130 SC2207:
    138 SC2046:
    192 SC2154:
    206 SC2162:
    232 SC2155:
    316 SC2034:
    571 SC2168:
   3717 SC2086:

shellcheck.style.out
      1 SC2066:
      1 SC2091:
      2 SC1001:
      2 SC1087:
      2 SC2003:
      2 SC2038:
      2 SC2063:
      2 SC2126:
      2 SC2143:
      2 SC2218:
      2 SC2229:
      2 SC2234:
      3 SC1007:
      3 SC1097:
      3 SC2059:
      3 SC2064:
      3 SC2140:
      3 SC2199:
      3 SC2223:
      4 SC2010:
      4 SC2016:
      4 SC2053:
      4 SC2115:
      4 SC2125:
      5 SC2029:
      5 SC2089:
      5 SC2090:
      6 SC1091:
      6 SC2021:
      6 SC2045:
      6 SC2094:
      6 SC2129:
      7 SC2009:
      7 SC2027:
      7 SC2043:
      7 SC2044:
      7 SC2124:
      8 SC2005:
      9 SC2012:
      9 SC2013:
      9 SC2188:
     16 SC2116:
     16 SC2153:
     20 SC2174:
     21 SC2236:
     26 SC2002:
     27 SC2001:
     30 SC1090:
     32 SC2006:
     33 SC2004:
     34 SC2166:
     34 SC2196:
     35 SC2181:
     35 SC2231:
     40 SC2068:
     57 SC2128:
     59 SC2145:
     63 SC2219:
     65 SC2254:
     69 SC2015:
     75 SC2164:
    121 SC2206:
    130 SC2207:
    138 SC2046:
    192 SC2154:
    206 SC2162:
    232 SC2155:
    316 SC2034:
    571 SC2168:
   3717 SC2086:

I will start with what ShellCheck considers to be an "error"
and afterwards step by step as time permits we can proceed
with "warning" then "info" and finally perhaps even "style" issues.

jsmeix commented at 2022-04-26 13:14:¶

Now the only ShellCheck error that is left is
SC2145: "Argument mixes string and array. Use * or separate argument."

# for e in error warning info style ; \
  do for s in usr/sbin/rear $( find usr/share/rear/ -type f -name '*.sh' ) ; \
     do shellcheck -s bash -S $e -e SC2168 $s ; \
     done >shellcheck.$e.out ; \
  done

# for e in error warning info style ; \
  do echo shellcheck.$e.out ; grep -o 'SC....:' shellcheck.$e.out | wc -l ; \
  done

shellcheck.error.out
47
shellcheck.warning.out
1547
shellcheck.info.out
5669
shellcheck.style.out
5944

# grep -o 'SC....:' shellcheck.error.out | sort | uniq -c
     47 SC2145:

github-actions commented at 2022-06-26 03:24:¶

Stale issue message

github-actions commented at 2022-08-27 03:50:¶

Stale issue message

github-actions commented at 2022-10-27 03:26:¶

Stale issue message

github-actions commented at 2022-12-27 02:24:¶

Stale issue message

thomasmerz commented at 2022-12-27 10:20:¶

ping @gdha

github-actions commented at 2023-02-27 02:39:¶

Stale issue message

jsmeix commented at 2023-02-27 08:55:¶

See
https://github.com/rear/rear/pull/2847#issuecomment-1431497212

github-actions commented at 2023-04-29 02:19:¶

Stale issue message

thomasmerz commented at 2023-04-29 13:41:¶

@jsmeix , this issue should already be fixed by https://github.com/rear/rear/pull/2847 , isn't it?

jsmeix commented at 2023-05-12 13:25:¶

Since also
https://github.com/rear/rear/pull/2976
is merged
I consider this issue to be sufficiently done.
If issues appear they will be handled via separated GitHub issues.

Many thanks to
@antonvoznia @thomasmerz @pcahyna @jamacku @lzaoral
and all the other @rear/contributors
who helped and contributed here!

I wish you all a relaxed and recovering weekend!

[Export of Github issue for rear/rear.]

#1040 Issue closed: Use ShellCheck for ReaR scripts static analysis¶

jsmeix opened issue at 2016-10-19 12:38:¶

thomasmerz commented at 2022-02-04 16:29:¶

pcahyna commented at 2022-02-04 16:52:¶

thomasmerz commented at 2022-02-04 18:28:¶

thomasmerz commented at 2022-02-04 20:54:¶

pcahyna commented at 2022-02-09 22:49:¶

jsmeix commented at 2022-02-10 07:25:¶

thomasmerz commented at 2022-02-10 10:23:¶

thomasmerz commented at 2022-02-10 10:27:¶

pcahyna commented at 2022-02-10 11:35:¶

jsmeix commented at 2022-02-10 11:54:¶

pcahyna commented at 2022-02-10 11:59:¶

pcahyna commented at 2022-02-10 12:00:¶

jsmeix commented at 2022-02-10 12:13:¶

jsmeix commented at 2022-02-10 12:34:¶

jsmeix commented at 2022-02-10 12:40:¶

jsmeix commented at 2022-02-10 12:57:¶

pcahyna commented at 2022-02-10 13:20:¶

jsmeix commented at 2022-02-10 14:21:¶

thomasmerz commented at 2022-02-10 14:59:¶

thomasmerz commented at 2022-02-10 15:02:¶

thomasmerz commented at 2022-02-10 15:05:¶

thomasmerz commented at 2022-02-10 15:30:¶

pcahyna commented at 2022-02-10 15:56:¶

thomasmerz commented at 2022-02-10 16:51:¶

jsmeix commented at 2022-02-11 09:05:¶

jsmeix commented at 2022-02-11 09:11:¶

thomasmerz commented at 2022-02-11 09:34:¶

jsmeix commented at 2022-02-16 09:32:¶

antonvoznia commented at 2022-03-06 14:35:¶

jsmeix commented at 2022-03-07 13:45:¶

thomasmerz commented at 2022-03-07 16:47:¶

jsmeix commented at 2022-03-09 09:07:¶

jsmeix commented at 2022-03-09 13:41:¶

jsmeix commented at 2022-03-09 13:55:¶

jsmeix commented at 2022-04-26 13:14:¶

github-actions commented at 2022-06-26 03:24:¶

github-actions commented at 2022-08-27 03:50:¶

github-actions commented at 2022-10-27 03:26:¶

github-actions commented at 2022-12-27 02:24:¶

thomasmerz commented at 2022-12-27 10:20:¶

github-actions commented at 2023-02-27 02:39:¶

jsmeix commented at 2023-02-27 08:55:¶

github-actions commented at 2023-04-29 02:19:¶

thomasmerz commented at 2023-04-29 13:41:¶

jsmeix commented at 2023-05-12 13:25:¶

#1040 Issue `closed`: Use ShellCheck for ReaR scripts static analysis¶