Add shellcheck action

[benjaminfrank]

the action ran properly in my forks PR: benjaminfrank#1
there are some findings though.

main question: is this desirable? if yes i could work on getting rid of the findings and update this PR accordingly over the weekend

[lynxthecat]

Ah interesting. Some GitHub wizardry.

[benjaminfrank]

I think you will need to enable/allow GH actions first and then all PRs will use shellcheck .
you can make passing mandatory if you want but currently there are some findings

[friendly-bits]

Thing is, shellcheck is not very flexible. adblock-lean currently has 66 warnings and 33 infos from shellcheck. I am fairly certain that all of them are either irrelevant or completely benign. We disabled some shellcheck warnings globally via directives, but that's not enough to make it completely warning-free. I wouldn't want to disable every shellcheck warning currently showing up because in some cases same shellcheck warnings are actually helpful and relevant. For example, it complains about the EXTRA_COMMANDS variable (SC2034 - EXTRA_COMMANDS appears unused) because it doesn't know that EXTRA_COMMANDS is actually used by /etc/rc.common. At the same time, there are cases where I mistype a name of a variable and that warning actually helps me to spot the mistake.

The "proper" way to deal with this (AFAIK) is adding a specific shellcheck directive before the line where a warning should be ignored. However this solution has the downside of increasing the script size and lines count, while adding nothing to the functionality.

Ideas are welcome.

[lynxthecat]

Hmm yes in cake-autorate we just added this:

https://github.com/lynxthecat/cake-autorate/blob/master/.shellcheckrc

So really only for the benefit of those with a working directory and making commits. I found shellcheck helpful but end up ignoring a lot of the warnings.

[friendly-bits]

I'm thinking that perhaps a good middle-ground would be calling shellcheck --severity error, which would only check for errors. At my experience, shellcheck errors are actual errors. So this way it's useful.

[friendly-bits]

Also the PR should target a new branch, rather than master. I'll make a development branch.
Edit: ignore the above, as this only adds a .yaml file. So can go into master.

[benjaminfrank]

another idea i had: if the size of the script is critical and the amount of shellcheck pragmas is an issue:
the script could strip those out. basically a grep -v '# shellcheck disable=' in the auto update and some "build" process that does the same when creating a release artifact

[justops1337]

@friendly-bits Is it normal that all files are reported with multiple errors SC1017 when the repo is cloned locally?
It looks like the files are created in Windows and have additional \r aka ^M carriage return characters.

[friendly-bits]

Is it normal that all files are reported with multiple errors SC1017 when the repo is cloned locally?

This is definitely not normal. Unix scripts should have Unix-style newlines. adblock-lean even checks the newline style of the config file because at some point we've had users copy-pasting config from Windows with Windows-style newlines (CR) which would cause config load failure.

While I am working on adblock-lean on Windows (in VS Code), the editor is configured to use Unix-style newlines (LF). Moreover, I just verified (on my Linux machine) that the files do not have any CR newlines:

$ git clone https://github.com/lynxthecat/adblock-lean
$ cd adblock-lean
$ cr="$(printf '\r')"
$ grep "$cr" adblock-lean abl-install.sh usr/lib/adblock-lean/*; echo $?
1
$ grep "$cr" cr-test.sh; echo $? # test against a file containing CR newlines

0
$ shellcheck adblock-lean
[ no SC1017, just some harmless info's ]
$ shellcheck cr-test.sh

In cr-test.sh line 1:
#!/bin/sh
         ^-- SC1017 (error): Literal carriage return. Run script through tr -d '\r' .

So I believe it's something in your setup. I can try help with troubleshooting this if you like but perhaps it's better done with PM's in the forum.

[friendly-bits]

P.s. I aplogize to @benjaminfrank for not merging this PR. The reason is that currently I don't feel like this is needed. Personally, I'm using shellcheck plugin in VS Code which provides real-time feedback, so I already know everything shellcheck has to say about the scripts. Perhaps if more people get involved in active development, merging this PR will start to make sense.

[justops1337]

Is it normal that all files are reported with multiple errors SC1017 when the repo is cloned locally?

This is definitely not normal. Unix scripts should have Unix-style newlines. adblock-lean even checks the newline style of the config file because at some point we've had users copy-pasting config from Windows with Windows-style newlines (CR) which would cause config load failure.

While I am working on adblock-lean on Windows (in VS Code), the editor is configured to use Unix-style newlines (LF). Moreover, I just verified (on my Linux machine) that the files do not have any CR newlines:

$ git clone https://github.com/lynxthecat/adblock-lean
$ cd adblock-lean
$ cr="$(printf '\r')"
$ grep "$cr" adblock-lean abl-install.sh usr/lib/adblock-lean/*; echo $?
1
$ grep "$cr" cr-test.sh; echo $? # test against a file containing CR newlines

0
$ shellcheck adblock-lean
[ no SC1017, just some harmless info's ]
$ shellcheck cr-test.sh

In cr-test.sh line 1:
#!/bin/sh
         ^-- SC1017 (error): Literal carriage return. Run script through tr -d '\r' .

So I believe it's something in your setup. I can try help with troubleshooting this if you like but perhaps it's better done with PM's in the forum.

You're right. The issue was on my end and for some reason, the git config --global core.autocrlf was with an empty value. Just switched it to git config --global core.autocrlf input and cloned again the repo. Thanks

[friendly-bits]

Perhaps we should add .gitattributes file in order to explicitly tell git to use LF newlines, like so. Probably this should prevent such issues.

[justops1337]

Maybe it'll be a better way to set it as auto in .gitattributes and to define different file types. This will give you a more granular solution.

*               text=auto
*.gitattributes text
.gitignore      text
*.md            text diff=markdown
*.sh            text eol=lf

You can find here also a few examples: gitattributes

[friendly-bits]

I really don't know much about gitattributes so I'm not sure what all this does and why it's needed. For instance, what's wrong with having eol=lf apply to all files (including md), rather than only to .sh files? What does this do:

*               text=auto

And this:

*.md            text diff=markdown

[justops1337]

* text=auto - Auto detect text files and perform LF normalization

The second one was just an example if you want to apply different rules for different file types.