-
Notifications
You must be signed in to change notification settings - Fork 178
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
Apply all current shellcheck suggestions to install.sh #1175
Conversation
@@ -336,7 +336,7 @@ parse_flags () | |||
while :; do | |||
case $1 in | |||
--develop) | |||
develop_build='1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this variable needed? It wasn't referenced anywhere.
Is the --develop
flag needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it isn't anymore. Command line flag still exists for backwards compatibility.
export PKG_CONFIG_PATH="${jm_root}/lib/pkgconfig:${PKG_CONFIG_PATH}" | ||
export LD_LIBRARY_PATH="${jm_root}/lib:${LD_LIBRARY_PATH}" | ||
export C_INCLUDE_PATH="${jm_root}/include:${C_INCLUDE_PATH}" | ||
export MAKEFLAGS='-j' | ||
|
||
# flags | ||
develop_build='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't tell if this variable was needed. Shellcheck didn't detect it was used anywhere.
e3cf26f
to
36fa761
Compare
install.sh
Outdated
@@ -434,20 +434,18 @@ main () | |||
{ | |||
jm_source="$PWD" | |||
jm_root="${jm_source}/jmvenv" | |||
jm_deps="${jm_source}/deps" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What jm_deps
used at all outside the install script? Shellcheck didn't detect it being used.
36fa761
to
d5f4a42
Compare
If we are doing this, might as well fix these things for all other shell scripts too.
And also add |
I think shellcheck is a good idea because shell scripts have many pitfalls which are sometimes hard to spot. Shellcheck provides good suggestions for a linter IMHO. I don't mind taking a look at automating the checks and making PRs to clean up the scripts, but I'm not sure when I'll get around to it. |
Superseded by #1414. |
…hell.sh` 22c13b0 Remove unused code instead of commenting out (Kristaps Kaupe) b928713 Add ShellCheck linter script (Kristaps Kaupe) 4f0eebc Apply all current shellcheck suggestions to rest of the scripts (Kristaps Kaupe) f0b9872 Apply all current shellcheck suggestions to install.sh (Kristaps Kaupe) Pull request description: Based on #1175, but also covers `install.sh` changes since then, all other shell scripts and adds ShellCheck linter script. Not adding this to CI for now. I'm thinking we could split off all the linting to separate job, don't see a point in running them under all OS / Python version combinations. Top commit has no ACKs. Tree-SHA512: 936320280f1ec9d8aa315c70be4d33fcb06d324531a17b30987c0be308d5351aa45dfac05cecadca8e9977a8f797b62ccd91d5d5fe92c5d0eacc6ebdf4dcd1c6
I noticed several shellcheck suggestions which I wrapped up into this PR. There could be issues with some of these changes. See comments.