Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shellcheck check #55

Open
wants to merge 10 commits into
base: master
from

Conversation

@Mickael-Martin
Copy link

commented Feb 25, 2019

To refer on #24
Maybe, some modifications will be done on other checks to obtain a good compatibility (I try it this my package)

package_linter.py Outdated Show resolved Hide resolved

Mickael and others added some commits Feb 25, 2019

Mickael
prefer launch sheelcheck only if present, no install
Example :

! Binary shellcheck not found in $PATH, try to install it
! Cannot parse the script with shellcheck
Mickael
add exclusion for source cmd
files called by source can be not present on a dev environment
@alexAubin

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

So I tested it on my side and it seem to spit out a lot of messages per script ... namely the double-quoting of variable which is not exactly the standard among apps :/ I doubt that the app folks will be happy about seeing so many messages so I disabled the corresponding error category

Testing on various apps, I see it still shows a lot of various error and many of them might not be so relevant, idk ...

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

So I tested it on my side and it seem to spit out a lot of messages per script ... namely the double-quoting of variable which is not exactly the standard among apps :/ I doubt that the app folks will be happy about seeing so many messages so I disabled the corresponding error category

Does that still cover if [ $stuff == 'other_sutff' ]? Because that's a super common error that will fail if there is a space in $stuff and that should REALLY be quoted.

@maniackcrudelis

This comment has been minimized.

Copy link

commented Mar 2, 2019

double-quoting of variable which is not exactly the standard among apps :/ I doubt that the app folks will be happy about seeing so many messages

Especially for that "error" which is not one...
Still, the case shown by Bram is indeed an error.
About double quotes globally, as I said many times, one should know why and when using double quotes instead of stuffing scripts of double quotes everywhere.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Does that still cover if [ $stuff == 'other_sutff' ] ?

It probably doesn't, but I doubt one can simply enable a check for this case while disabling this check for all the other case :s

@Mickael-Martin

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

So I tested it on my side and it seem to spit out a lot of messages per script ... namely the double-quoting of variable which is not exactly the standard among apps :/ I doubt that the app folks will be happy about seeing so many messages so I disabled the corresponding error category

Does that still cover if [ $stuff == 'other_sutff' ]? Because that's a super common error that will fail if there is a space in $stuff and that should REALLY be quoted.

Yes :

in /tmp/test.sh line 2:
if [ $1 == "test" ];then
     ^-- SC2086: Double quote to prevent globbing and word splitting.

For me, https://github.com/koalaman/shellcheck/wiki/SC2086 is a common mistake and bash devs must to check their code to fix it. This error causes several broken code, make an exception is not a good choice in my opinion.

@maniackcrudelis

This comment has been minimized.

Copy link

commented Mar 4, 2019

What about if [ $val -eq 0 ] or if [ $val == stuff ]?
Those are not errors.

@Mickael-Martin

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

What about if [ $val -eq 0 ] or if [ $val == stuff ]?
Those are not errors.

The first example is a comparison of integers, so you don't need double quotes, for the second, if you have space in val var, your condition cannot works.

@maniackcrudelis

This comment has been minimized.

Copy link

commented Mar 4, 2019

In both cases you don't need double quotes.
If the comparison is between $val and stuff, then you don't expect any space.
If I use such a comparison, then I know that $val will not contain any space.

@maniackcrudelis

This comment has been minimized.

Copy link

commented Mar 4, 2019

I already said that many times.
An if condition is a simple case, but there many others cases not as simple as an if.
One should now how, when and why using double quotes instead of putting it everywhere that's easy to know you would maybe need some.

@Mickael-Martin

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

But you have an error if your $val contains space(s) or contain nothing, if you not test for this $val before.
For comparison string, double quotes are a best practice, you have many example of security issues and bypassing check without var non-double quoted (like https://wiki.bash-hackers.org/syntax/quoting, or https://unix.stackexchange.com/questions/171346/security-implications-of-forgetting-to-quote-a-variable-in-bash-posix-shells )

@maniackcrudelis

This comment has been minimized.

Copy link

commented Mar 4, 2019

if $val contains a space or is empty.
In my example, I didn't quote either $val or stuff. That was on purpose, because I know neither one or the other will contain a space or be empty.
For sure in a if there's no risk, but there's many others cases you have to know what you're doing.
Packagers should learn how to deal with double quotes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.