#2847 PR merged: Add Shell linter - Differential-ShellCheck

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

jamacku opened issue at 2022-08-05 13:02:

Differential ShellCheck is a GitHub action that performs differential ShellCheck scans on files changed via PR and reports results directly in PR.

Since this repository contains a lot of shell scripts I think you might find it useful.

Documentation is available at: @redhat-plumbers-in-action/differential-shellcheck. Let me know If you are missing some feature or setting. I'm always happy to extend functionality.

Examples - screenshots

  • Type: Enhancement

  • Impact: Low

  • Reference to related issue (URL): #1040

  • How was this pull request tested?

Manually, status should be showed below.

It is deployed and running on a few repositories already and it's working as expected.

  • Brief description of the changes in this pull request:

Adding workflow that executes differential-shellcheck action when PR is opened against master branch.

Alternative to: #2754 and #2756

/cc @lzaoral @pcahyna

jsmeix commented at 2022-08-08 11:22:

A general note on using ShellCheck for ReaR scripts:
https://github.com/rear/rear/issues/1040#issuecomment-1062932894

Conditions for using ShellCheck for ReaR scripts:
https://github.com/rear/rear/issues/1040#issuecomment-1034278415

* show only newly introduced problems
...
* do not produce too many false positives

According to "How does it work"
https://github.com/redhat-plumbers-in-action/differential-shellcheck#readme
it seems "differential ShellCheck scans on files changed via PR"
means that only the differences in ShellCheck output are reported
so that old / already existing ShellCheck issues are not reported
but only new ShellCheck issues because of changes in a PR.

If this is true the first condition
"show only newly introduced problems"
should be fulfilled with Differential-ShellCheck.

Regarding the second condition
"do not produce too many false positives"
see in particular
https://github.com/rear/rear/issues/1040#issuecomment-1034576705
and
https://github.com/rear/rear/issues/1040#issuecomment-1034978519

Efficiency is what actually matters.

How to get efficiency see
https://github.com/rear/rear/issues/1040#issuecomment-1062945160

# 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 ;

...

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

...

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.

So what I want mandatory is that one can see
the severity ('error' 'warning' 'info' 'style')
of each reported ShellCheck issue.

E.g. we may initially limit reported ShellCheck issues
only to what ShellCheck considers to be an "error"
(if this is possible with Differential-ShellCheck)
and watch how things behave with that for some time.

pcahyna commented at 2022-08-08 12:15:

I propose to leave this PR open and let @antonvoznia experiment with it and report results (it is one of the topics of his thesis).

jsmeix commented at 2022-08-08 13:00:

@pcahyna
I had no plans to close this issue.

In general I appreciate automated ShellCheck tests, cf.
https://github.com/rear/rear/issues/1040#issuecomment-1062932894
provided it is carefully implemented so it provides
real net value to ReaR developers and ReaR users.

I dared to assign this issue to you
because @jamacku @lzaoral and @antonvoznia
belong to your organization.

pcahyna commented at 2022-08-08 13:16:

I had no plans to close this issue.

I meant that the PR should not be merged yet, not that I expected it to be closed without merging :-)

jamacku commented at 2022-08-09 07:50:

So what I want mandatory is that one can see the severity ('error' 'warning' 'info' 'style') of each reported ShellCheck issue.

E.g. we may initially limit reported ShellCheck issues only to what ShellCheck considers to be an "error" (if this is possible with Differential-ShellCheck) and watch how things behave with that for some time.

differential-shellcheck currently doesn't support severity option, but it is planned to implement it and it should be really easy - https://github.com/redhat-plumbers-in-action/differential-shellcheck/issues/75

jsmeix commented at 2022-08-09 10:08:

@jamacku
wow! - thank you for
https://github.com/redhat-plumbers-in-action/differential-shellcheck/issues/75

jamacku commented at 2022-08-16 13:15:

Differential ShellCheck now allows to define minimal defect severity to be reported - https://github.com/redhat-plumbers-in-action/differential-shellcheck#severity

antonvoznia commented at 2022-10-02 13:01:

Hi @jamacku ,
Thank you for doing it.
I've tried this configuration, and I've got the message:
Warning: Unexpected input(s) 'severity', valid inputs are ['entryPoint', 'args', 'base', 'head', 'ignored-codes', 'shell-scripts', 'token']

Do you know why it prints such warnings?

jamacku commented at 2022-10-03 07:35:

Hi @jamacku , Thank you for doing it. I've tried this configuration, and I've got the message: Warning: Unexpected input(s) 'severity', valid inputs are ['entryPoint', 'args', 'base', 'head', 'ignored-codes', 'shell-scripts', 'token']

The severity option was introduced in differential-shellcheck@v2.5.0 but the workflow was still using @v2.4.0. I have updated the SHA. Now it should work correctly.

github-actions commented at 2022-12-03 02:25:

Stale pull request message

jamacku commented at 2022-12-04 11:41:

@antonvoznia, Any updates?

thomasmerz commented at 2022-12-13 15:51:

@jamacku , what's the difference or what is better than my PR https://github.com/rear/rear/pull/2754 ?

jamacku commented at 2022-12-14 08:32:

@jamacku , what's the difference or what is better than my PR #2754 ?

Hi @thomasmerz, looking at your PR, I see the following differences:

  • In your workflow, you are manually downloading ShellCheck sources, unpacking them, and executing them. This isn't the best practice, in my opinion. ShellCheck provides a container with everything you need, or you can use ShellCheck supplied by your distro. After that, you manually look for shell scripts in the repo and execute ShellCheck. All these manual steps could require future maintainers attention if they stop working or they want to take advantage of some new ShellCheck features. On the other hand, the ShellCheck workflow proposed in this PR uses dedicated GitHub Action, and maintainers of the rear don't have to maintain the code and logic behind it. It is constantly developed and updated, and new features are added.

  • I have also noticed that your ShellCheck workflow performs full scans. That is why you need to fix all "defects" to make it pass. This is usually quite challenging for most bigger projects since ShellCheck can produce a lot of "defects". Also, fixing existing defects in some cases could result in regressions. Meanwhile, ShellCheck from this PR performs differential scans and reports only newly added defects. This makes it easy for maintainers to adopt ShellCheck early and start linting incoming changes. It doesn't force them to fix or mute all "defects" reported by ShellCheck if they have existed in some cases for years, and they can focus on fixing real issues and defects.

  • From what I can tell, your workflow produces only text output into the runner's console. But this ShellCheck workflow can produce SARIF format and upload it directly into GitHub. You can see an example in the first comment of this PR.

thomasmerz commented at 2022-12-14 20:57:

@jamacku , now you really got me (plus after having a look at your GH profile). You @jamacku are a full-time-developer at Red Hat, I'm just a linux sysadm (@thomas-merz is my business account) and no developer. BTW: I've heard of your co-workers Lukรกลก and Kamil from @timogoebel at work ๐Ÿ˜ƒ

Therefore I totally agree that your solution and maintenance will be much better for the ReaR project over here ๐Ÿ‘๐Ÿผ
I really like "my" workflow for my personal and much smaller projects very well. But for this enterprise solution we really need a more professional and better supported workflow as you can and will provide.
I'm really glad that I started some discussions about assuring code quality by shellcheck and I really want to hand over to your solution. So I will close my PR #2754 to honor your PR. Please respond/give feedback ( @gdha @jsmeix @schlomo ) if you also prefer this PR over mine?

Just some final question:
Will your workflow use latest shellcheck version shortly after it has been released? My workflow always use latest (which some people like and others might dislike) because I don't want to care about updating versions.

I will have a look if your workflow can also replace mine in my personal (and other contributed) projects ๐Ÿคท๐Ÿผโ€โ™‚๏ธ

antonvoznia commented at 2022-12-14 21:04:

Hi @jamacku,
I'm still working with it and testing the Differential Shellcheck.
The Differential Shellcheck looks good. We want to test it more carefully for the ReaR project.

@pcahyna sent me some interesting PRs that had been merged.
I executed them with enabled Differential SHellcheck. So I verified if Differential Shellcheck find fixed errors.
And the same but in reverse way, I added the errors back and verify if Differential Shellcheck recognize it.
Also, I tested it with different severeties.
There some examples for original PR https://github.com/rear/rear/pull/2831:

https://github.com/antonvoznia/rear/pull/336
https://github.com/antonvoznia/rear/pull/333

I sent more examples to @pcahyna and @jsmeix.

Also, I added exceptions of rules into .shllcheckrc:
https://github.com/antonvoznia/rear/pull/71
Please, ignore the fact that I disabled Packit for the testing.

Thank you,
Anton

pcahyna commented at 2022-12-16 10:37:

Hi @jamacku , thank you for taking the time to explain the advantages of your solution better than I would. From my point of view, this is the key point:

  • I have also noticed that your ShellCheck workflow performs full scans. That is why you need to fix all "defects" to make it pass. This is usually quite challenging for most bigger projects since ShellCheck can produce a lot of "defects". Also, fixing existing defects in some cases could result in regressions. Meanwhile, ShellCheck from this PR performs differential scans and reports only newly added defects. This makes it easy for maintainers to adopt ShellCheck early and start linting incoming changes. It doesn't force them to fix or mute all "defects" reported by ShellCheck if they have existed in some cases for years, and they can focus on fixing real issues and defects.

The solution of examining all existing defects or "defects" is really not feasible. I recall that the original issue #1040 triggered cleanups of RAID code by @jsmeix and it took me lots of work to test just this chnage (PR #2768) because indeed, when fixing linter warnings en masse, one risks introducing regressions, often more serious than the original defects.

schlomo commented at 2022-12-16 11:00:

I really like this approach of checking only new stuff, thanks a lot @jamacku for this PR.

Can you maybe also add a dependabot entry to update the pinned GitHub actions versions? That way we won't "forget" to update.

Besides that I'd like to encourage everybody to accept this PR and to see how it works out in "real life". I think that the risk is very low as we can always change the configuration if there is too much pain in future PRs. I think it wouldn't hurt ReaR as a project to be a little bit more daring with such changes, as reverting them or modifying them to be less problematic is not much work. And maybe it just works out well and then everybody will be happy.

jsmeix commented at 2022-12-16 11:30:

@schlomo
when I visited Prague (triggered by Ubuntu Summit 7-9 Nov.)
I talked in person with @pcahyna and @antonvoznia
in particular about the "ShellCheck for ReaR" topic.

We all agree here (as far as I see).

Because "ShellCheck for ReaR" is implemented by Red Hat people
and @pcahyna is a ReaR maintainer, he leads that whole thing.
As far as I know he is currently rather busy with other things
that have higher priority so I ask for patience until he gets time
(after all "ShellCheck for ReaR" is not an urgent issue).

jamacku commented at 2022-12-16 11:33:

Hi @thomasmerz, Thanks a lot for your constructive approach and discussion. It's nice to see such a great community of developers.

Just some final question: Will your workflow use latest shellcheck version shortly after it has been released? My workflow always use latest (which some people like and others might dislike) because I don't want to care about updating versions.

In the current solution, I use the latest stable Fedora container as the base image for my Action. I'm using Fedora since there are a couple more packages that I need and are currently packaged only for Fedora. So I use the ShellCheck version that comes with Fedora. Now, my Action uses ShellCheck 0.8.0. But I have already created request to update to ShellCheck 0.9.0 once it's available in Fedora 37.

Regarding updates, I usually release a new minor release (e.g. v3.x.0) when I update to a new ShellCheck version. So it depends on how you are using my Action. If you pin to a major version (e.g. v3), you should always use "latest ShellCheck". If you are pinning to SHA or the exact version, then you probably would need some dependency manager to take care of updating.

I will have a look if your workflow can also replace mine in my personal (and other contributed) projects ๐Ÿคท๐Ÿผโ€โ™‚๏ธ

Feel free to reach out to me in case of any questions or requests. :slightly_smiling_face:

jamacku commented at 2022-12-16 11:36:

Can you maybe also add a dependabot entry to update the pinned GitHub actions versions? That way we won't "forget" to update.

Sure, I'll add the Dependabot config.

jsmeix commented at 2022-12-16 11:46:

FYI:

https://github.com/rear/rear/pull/2768
is a very good example for what I wrote in
https://github.com/rear/rear/issues/1040#issuecomment-1062932894

See in particular my comment on Mar 18
https://github.com/rear/rear/pull/2768#issuecomment-1072387116

... now I do at least understand what the code should do

which was 10 days after my initial comment there on Mar 8.
Before Mar 18 my attempts to fix it made things worse
(i.e. I introduced regressions) because I did not
understand what the code actually does.

So what is more important in practice
than a coding style checking tool is
https://github.com/rear/rear/wiki/Coding-Style

Make yourself understood
...
Code must be easy to read
...
Code must be easy to understand (answer the WHY)
...
Provide comprehensive comments that
tell what the computer should do and
also explain why it should do it
so others understand the intent behind
so that they can properly fix issues
or adapt and enhance things as needed
at any time later.

jsmeix commented at 2022-12-16 12:18:

Regarding "mute defects reported by ShellCheck":

In general I am against muting what ShellCheck reports.

My reasoning:
With muting ShellCheck the issue gets only concealed
which makes things worse because then the real issue
will never again be reported so it will never get fixed.
Furthermore those shellcheck disable=SC... comments
encourage others to "just do the same" to deal with ShellCheck.
In the end bad code becomes even more bad with tons of
ugly shellcheck disable=SC... comments all over the place
instead of actually fixing the root causes of issues.

Only when something reported by ShellCheck is a false positive,
muting ShellCheck is OK but then only with an explanatory
comment that tells WHY the particular case is as false positive.

Unfortunately it seems muting things reported by ShellCheck
is often the immediate way how ShellCheck issues are handled
which makes in the end all only worse (in practice nobody
will later care about muted ShellCheck issues).

pcahyna commented at 2022-12-16 12:31:

@antonvoznia is now producing examples of how will ShellCheck work in practice (by redoing old PRs with ShellCheck turned on). See selected examples in his reply. My plan is to take those examples and identify which real defects have been found by ShellCheck and which "defects" are uninteresting, or just false positives. Then he will produce statistics of the ratio between useful and useless warnings depending on the severity level of the tool.
In addition, I would like to have also statistics for more PRs about the amount of warnings and the frequency of every warning (without knowing whether they are valid or not). This will get us some idea about the amount of warnings we should expect from future changes.
According to the findings we will then decide what severity level and perhaps other settings of ShellCheck are the most appropriate, and merge the PR. My ETA for that is half of January.

The first task (identifying which warnings are real defects and which ones are useless) will need manual work and I expect to do that myself (it needs someone who is familiar with the code base, so @antonvoznia ca'nt do it by himself). (That's why the plan is to do it only for a subset of selected PRs.) If someone else wants to contribute to this effort, help is welcome!

pcahyna commented at 2022-12-16 13:00:

In general I am against muting what ShellCheck reports.

Me too, and that's another good aspect of having differential checks: you can simply ignore the results if they are not relevant and ShellCheck won't bother you again (but in case someone decides to do a scan in non-differential mode, ShellCheck will report the problems again and one can reexamine them - they will not get concealed).

Only when something reported by ShellCheck is a false positive,
muting ShellCheck is OK but then only with an explanatory
comment that tells WHY the particular case is as false positive.

We will eventually need to decide what to do in these cases. A typical example will be the unquoted expansion $v (v is a variable that is either -v or nothing). If ShellCheck warns about unquoted expansion, it is a false positive. But annotating every occurrence of this with a "shellcheck disable" comment and an explanation will lead to lots of such annotations in the code.

jsmeix commented at 2022-12-16 13:43:

Regarding unquoted variables:

According to
https://github.com/rear/rear/issues/1040#issuecomment-1034870262
we had on Feb 10

3717 SC2086

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

According to
https://github.com/rear/rear/issues/1040#issuecomment-1062945160

shellcheck.info.out
...
3717 SC2086

SC2086 is only of info severity for ShellCheck.

I think at least for now we don't need to care about it
because I think we should start with what ShellCheck
considers to be an "error" and afterwards
step by step as time permits we can proceed
with "warning" and later perhaps "info" and
finally perhaps even "style" but "style" sounds
questionable (if not even "calling for dispute").

In particular regarding

command $v ...

https://github.com/koalaman/shellcheck/wiki/SC2086
shows how one could make safe code for it via

command "${v[@]}" ...

or

command ${v:+"-v"} ...

but now the code looks ugly which contradicts
"Code must be easy to read" in
https://github.com/rear/rear/wiki/Coding-Style
and
it is likely even inexplicable for unexperienced users

What the heck does that "${v[@]}" or ${v:+"-v"} thingy do
and why the hell do they need to use that stuff here?

which contadicts
"Code must be easy to understand (answer the WHY)" in
https://github.com/rear/rear/wiki/Coding-Style
and I don't want to have a lengthy explanation comment
at each code place where e.g. '$v' is used unquoted.

github-actions commented at 2023-02-15 02:39:

Stale pull request message

pcahyna commented at 2023-02-15 14:54:

My plan for integrating this is as follows:
@antonvoznia did some research on configuration adjustements needed. I will let him clean up and rebase his branch ( https://github.com/antonvoznia/rear/tree/differential-shellcheck ). Once this is done, I will merge this PR ( if it is ready ) and let @antonvoznia open another PR with his changes on top of the merged master branch.

schlomo commented at 2023-02-22 21:43:

I have three main questions:

  1. where to we maintain and discuss our shellcheck rules?
  2. how do we teach shellcheck to consider Bash 4.1 as "reference" as we will support those old versions for quite some time?
  3. we use a large amount of Bashism that shellcheck complains about, and where I really don't see a benefit to follow the recommendation. Do we plan to exclude all those rules, or will we plaster ReaR code with shellcheck overrides? Or adapt ReaR code to match shellchecks coding guidelines?

I was quiet so far on this topic due to lack of practical experience, but recently I wrote some more ReaR code in VS Code with shellcheck active and I found it more annoying then helpful.

Some examples for that:

  • nearly every variable is marked as "unused" because it is used in another file that shellcheck of course can't know about
  • every unquoted variable is an error, but in many cases we actually know that we don't need to quote it because it is a system path or has other content where we created it and know that it won't contain spaces. Unquoted variables however are still easier to read and faster to type
  • every use of local is an error because shellcheck can't know that all our scripts are run inside a function
  • every beginning of a script file is an error because there is no shebang
  • we use var=( $(something)) in many - many - places and the word splitting happening there is often quite intentional. I personally don't see the value of alternatives like read -a var < <(something). Quite the opposite, our syntax preserves potential errors while the read syntax doesn't:
$ var=($(uname -a ; false)); echo $PIPESTATUS
1
$ read -a var < <(uname -a ; false); echo $PIPESTATUS
0

There is probably more, my point is that as much as I love code quality, I really don't know how much useful stuff will be left if we globally disable all the rules that I touched in this short list of things which annoyed me recently.

What I would like to see for this PR is therefore also the part of the global shellcheck rules, or at least deciding where we maintain those.

Also a big shout out of thank you for those who spent already so much time fixing shellcheck issues, as marked in our release notes

jsmeix commented at 2023-02-27 10:03:

From my experience, cf.
https://github.com/rear/rear/issues/1040#issuecomment-1062932894
I think when we globally disable all ShellCheck codes
where ShellCheck reports too many false positives,
a lot will still be left where things that ShellCheck reports
point to real issues (usually "point to" but not "are").

I think automated usage of ShellCheck does not require
that ShellCheck must detect very most of real issues.
I think it is already helpful when automated ShellCheck
detects "some" issues (for some reasonable amount of "some").
I.e. better some automated help than none at all.

jsmeix commented at 2023-02-27 12:35:

The most often reported ShellCheck codes
in our current GitHub master *.sh files:

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

# grep -o "SC.... (.*): " shellcheck.out | sort | uniq -c | sort -n -r

   3700 SC2086 (info): 
    593 SC2168 (error): 
    313 SC2034 (warning): 
    240 SC2155 (warning): 
    207 SC2162 (info): 
    180 SC2154 (warning): 
    146 SC2046 (warning): 
    130 SC2207 (warning): 
    119 SC2206 (warning): 
     75 SC2164 (warning): 
     69 SC2254 (warning): 
     69 SC2015 (info): 
     63 SC2219 (style): 
     51 SC2128 (warning): 
     47 SC2145 (error): 
     35 SC2231 (info): 
     35 SC2196 (info): 
     34 SC2166 (warning): 
     33 SC2004 (style): 
     32 SC2006 (style): 
     30 SC2181 (style): 
     27 SC2001 (style): 
     21 SC2174 (warning): 
     18 SC2236 (style): 
     18 SC1091 (info): 
     18 SC1090 (warning): 
     16 SC2295 (info): 
     16 SC2116 (style): 
     15 SC2153 (info): 
      9 SC2188 (warning): 
      9 SC2013 (info): 
      9 SC2012 (info): 
      8 SC2294 (warning): 
      8 SC2005 (style): 
      8 SC2002 (style): 
      7 SC2223 (info): 
      7 SC2044 (warning): 
      7 SC2043 (warning): 
      7 SC2009 (info): 
      6 SC2129 (style): 
      6 SC2124 (warning): 
      6 SC2094 (info): 
      6 SC2027 (warning): 
      6 SC2021 (info): 
      5 SC2090 (warning): 
      5 SC2089 (warning): 
      5 SC2029 (info): 
      4 SC2125 (warning): 
      4 SC2115 (warning): 
      4 SC2053 (warning): 
      4 SC2016 (info): 
      4 SC2010 (warning): 
      3 SC2140 (warning): 
      3 SC2064 (warning): 
      3 SC2059 (info): 
      2 SC2268 (style): 
      2 SC2234 (style): 
      2 SC2229 (warning): 
      2 SC2143 (style): 
      2 SC2126 (style): 
      2 SC2063 (warning): 
      2 SC2045 (warning): 
      2 SC2038 (warning): 
      2 SC2003 (style): 
      2 SC1007 (warning): 
      2 SC1001 (info): 
      1 SC2048 (warning): 
      1 SC2031 (info): 
      1 SC2030 (info):

The most often reported ShellCheck code

SC2086 (info): Double quote to prevent globbing and word splitting.

is in very most cases a false positive
(I did not check which ones are not false positives).
Because it is only 'info' we may care about it only
after we first and foremost cared about 'error'
and then as time permits about 'warning',
cf. at the bottom of
https://github.com/rear/rear/issues/1040#issuecomment-1062945160
see also what I wrote about 'SC2086' here in the above
https://github.com/rear/rear/pull/2847#issuecomment-1354798686

The most often reported ShellCheck 'error' codes are

    593 SC2168 (error): 
     47 SC2145 (error):

which are

SC2168 (error): 'local' is only valid in functions.
SC2145 (error): Argument mixes string and array. Use * or separate argument.

where SC2168 is almost always a false positive
(I did not check if there one that is not a false positive)
and regarding SC2145 see
https://github.com/rear/rear/issues/1040#issuecomment-1109782792
(back then I had no longer time to fix all of those).

The most often reported ShellCheck 'warning' code

SC2034 (warning): VARIABLE appears unused. Verify use (or export if used externally).

is in very most cases a false positive
(I did not check which ones are not false positives).

Regarding the second most often reported ShellCheck 'warning' code

VARIABLE=$( COMMAND )
^------^ SC2155 (warning): Declare and assign separately to avoid masking return values.

see
https://github.com/rear/rear/wiki/Coding-Style#variables

Avoid
local bar="$( COMMAND1 )"
because 'set -e' will not exit if COMMAND1 exits with a non-zero status.
Also
local baz="$( COMMAND2 )" || Error "COMMAND2 failed"
will not error out if COMMAND2 failed.
See https://github.com/koalaman/shellcheck/wiki/SC2155

jsmeix commented at 2023-02-27 12:56:

By the way via
https://github.com/rear/rear/commit/1db54d1b95b44698d7aaf37d2ca0d1c8808e8418
I fixed right now

In usr/sbin/rear line 232:
            echo "$PROGNAME: unrecognized option '$option'"
                                                  ^-----^ SC2154 (warning): option is referenced but not assigned.

so some ShellCheck 'warning' reports are actually bugs
which proves
https://schlomo.schapiro.org/2015/04/warning-is-waste-of-my-time.html

As a programmer I might not want to decide
for the user if some problem is an ERROR or not.
The obvious solution is to issue a WARNING in an attempt
to shed the responsibility of making a decision.
...
But in an automated world ... WARNINGS for most cases only create
extra manual work because somebody needs to go and check

I don't know if theoretically a perfect ShellCheck could be made
that can decide if a referenced but not assigned variable is an error
or if this is impossible for the bash scripting language.

thomasmerz commented at 2023-02-27 13:17:

@jsmeix , you should concentrate on "important" severity of errors as an top-down-approach: error, warning; but not info, style for the beginning.

jsmeix commented at 2023-02-27 14:12:

@thomasmerz
I think you misunderstood what I meant.

I meant that we can neither rely on that
a ShellCheck 'error' is actually an error
(because SC2168 is almost always a false positive)
nor that a ShellCheck 'warning' is not an error but only a warning.

Even a ShellCheck 'info' can be an actual error, see
https://github.com/rear/rear/issues/2927#issuecomment-1446322275

Regardless that we cannot rely on ShellCheck's severity rating
in practice we can only do what is possible with reasonable effort
which is what I explained in
https://github.com/rear/rear/issues/1040
therein in particular
https://github.com/rear/rear/issues/1040#issuecomment-1062932894
and
https://github.com/rear/rear/issues/1040#issuecomment-1060704029
and what I wrote above here

Because it is only 'info' we may care about it only
after we first and foremost cared about 'error'
and then as time permits about 'warning',
cf. at the bottom of
https://github.com/rear/rear/issues/1040#issuecomment-1062945160

schlomo commented at 2023-02-28 08:41:

Huge thanks @jsmeix for this detailed analysis!

What I take away from it is the following:

  1. we need to create a .shellcheckrc file (or maybe shellcheckrc to support running it as snap) with our specific configuration of
    • shell=bash
    • disabling checks we don't want to get alerts for, e.g. those that are mostly false positives
  2. it will take a bit of fine-tuning the rules as more commits/PRs come in and we shouldn't be worried about it but simply do it

I'd like to propose merging this PR as soon as the RC file is added so that we get started. It won't prevent anything but add valuable visibility, improving the quality of ReaR.

schlomo commented at 2023-04-23 09:10:

Any progress on this? I'm leaning towards being bold and moving forward vs. waiting for a miracle to happen. @jamacku could you please be so kind to also add the Shellcheck RC file with meaningful defaults?

lzaoral commented at 2023-04-23 09:37:

@schlomo The creation of meaningful ShellCheck defaults has nothing to do with @jamacku who is just the author of Differential ShellCheck. AFAIK, the defaults should be/have been created by @antonvoznia as part of his master thesis which @pcahyna supervised [1]. So we should wait on their input instead.

[1] See chapter 6: https://dspace.cvut.cz/bitstream/handle/10467/107238/F3-DP-2023-Voznia-Anton-dip_thesis-2.pdf

gdha commented at 2023-04-24 07:17:

Nice to have link of a thesis - thanks for that and keep up with the good (of students)

pcahyna commented at 2023-04-24 08:52:

Hi, I will meet with @antonvoznia this week to discuss the merging of his work and if there is no progress, I will take care of merging it myself.
(Sorry @schlomo , I agree that it is rather overdue.)

pcahyna commented at 2023-04-27 16:27:

merging - @antonvoznia please prepare a PR with your shellcheck configuration additions.

pcahyna commented at 2023-04-28 23:12:

@schlomo the ShellCheck RC files are in #2976. I will merge it without much delay, because I don't want to keep ShellCheck enabled without configuration for too long - it would lead to annoying noise in any future PRs. We can discuss further adjustments separately.

schlomo commented at 2023-04-29 19:36:

Thanks, fully agree with your approach

[Export of Github issue for rear/rear.]