GROOVY-9018: fix shellcheck warnings for bin scripts#896
Closed
calid wants to merge 1 commit intoapache:masterfrom
Closed
GROOVY-9018: fix shellcheck warnings for bin scripts#896calid wants to merge 1 commit intoapache:masterfrom
calid wants to merge 1 commit intoapache:masterfrom
Conversation
calid
force-pushed
the
fix-shellcheck-warnings
branch
from
a2340ba to
753eb40
Compare
Contributor
|
I changed the .gitignore separately, so you can remove that commit. Also, the completion script are generated by picocli, so we wouldn't apply that here since it would be overwritten the next time they are generated. Instead, if you have the time, it would be great if you approached that project and had them update their templates/code which are used when generating those files. But the bootstrap changes look good to me. I'll see if anyone else comments. |
most are common gotchas like lack of variable quoting, using deprecated
constructs like '``' instead of '$()', not exporting env vars used in
child scripts, and using non-portable operators like '-a' and '-o' in
conditionals
some specific items to note in startGroovy:
* ulimit -H -n is technically not portable as POSIX ulimit only
supports -f. however there is no clean POSIX-y alternative and after
researching pretty much every shell on every *nix implements these
options, so I've simply added a comment that suppresses this warning
* the logic to munge arguments on cygwin was unecessarily commplex
(and not portable). i've replaced this with a far simpler solution
that builds up the munged arguments string dynamically then uses eval
to invoke set and override the positional parameters
* the final java invocation did 'eval exec' which is unecessary and a
little nonsensical. directly using exec along with standard variable
interpolation should suffice. however since we _do_ want word
splitting on the JAVA_OPTS argument we disable the double quote
warning here
a complete dump of all the warnings shellcheck reported can be found
here: https://gist.github.com/calid/634258c25f9d4aa0c1327eff1c5b16e1
calid
force-pushed
the
fix-shellcheck-warnings
branch
from
753eb40 to
b00dc95
Compare
Contributor
Author
|
ok sounds good, updated the PR accordingly |
asfgit
pushed a commit
that referenced
this pull request
Mar 18, 2019
most are common gotchas like lack of variable quoting, using deprecated
constructs like '``' instead of '$()', not exporting env vars used in
child scripts, and using non-portable operators like '-a' and '-o' in
conditionals
some specific items to note in startGroovy:
* ulimit -H -n is technically not portable as POSIX ulimit only
supports -f. however there is no clean POSIX-y alternative and after
researching pretty much every shell on every *nix implements these
options, so I've simply added a comment that suppresses this warning
* the logic to munge arguments on cygwin was unecessarily commplex
(and not portable). i've replaced this with a far simpler solution
that builds up the munged arguments string dynamically then uses eval
to invoke set and override the positional parameters
* the final java invocation did 'eval exec' which is unecessary and a
little nonsensical. directly using exec along with standard variable
interpolation should suffice. however since we _do_ want word
splitting on the JAVA_OPTS argument we disable the double quote
warning here
a complete dump of all the warnings shellcheck reported can be found
here: https://gist.github.com/calid/634258c25f9d4aa0c1327eff1c5b16e1
Contributor
|
Merged, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.