-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[ci] enforce 'shellcheck' checks #6498
Comments
…icrosoft#6498) - Errors- SC2086 (info) : Double quote to prevent globbing and word splitting
…t#6498) - Fixed SC2086 (info): Double quote to prevent globbing and word splitting.
…icrosoft#6498) - fixed SC2086 (info): Double quote to prevent globbing and word splitting.
- Used double quotes around entire word as suggested - microsoft#6498
@jameslamb WDYT about using PSScriptAnalyzer for our PowerShell scripts? |
I'm willing to try it! I'm concerned that it'll be difficult to install, but maybe that's just because I don't know what "Powershell Gallery" is. I hope we won't have to build it from source on every CI run, and I hope we won't have to introduce a new |
Looks like that tool is written in C# programming language. We can use dotnet-sdk to build C# programs on Linux. Ref. |
GitHub Actions has preinstalled PowerShell. So installation is pretty simple actually! Just tried the following pwsh -v
pwsh -command "Install-Module -Name PSScriptAnalyzer -Scope AllUsers -Force -SkipPublisherCheck"
git clone https://github.com/microsoft/LightGBM
read -r -d '' analyzer_cmd << EOM
Invoke-ScriptAnalyzer -Path ./LightGBM/.ci -Severity Warning -Recurse -Outvariable issues
\$errors = \$issues.Where({\$_.Severity -eq 'Error'})
\$warnings = \$issues.Where({\$_.Severity -eq 'Warning'})
if (\$errors) {
Write-Error "There were \$(\$errors.Count) errors and \$(\$warnings.Count) warnings total." -ErrorAction Stop
} else {
Write-Output "There were \$(\$errors.Count) errors and \$(\$warnings.Count) warnings total."
}
EOM
pwsh -command "${analyzer_cmd}"
And got the following output:
|
Thanks for that! @StrikerRUS I'd support a PR that introduces this, if you'd like to try one. I hope we can add it to the existing linting job Line 101 in bbeecc0
pwsh on Linux.
I don't think this should be added to |
Yeah, I'd like to try.
Agree. |
Summary
This project has several shell scripts. Those should be tested with
shellcheck
.Motivation
Shell scripts are used in this project for 2 primary purposes:
build-python.sh
is a public, user-facing API into building the Python packageChecking those scripts with static analyzers would help to reduce the risk of mistakes being silently missed. It'd also give us a chance of catching issues in code paths that aren't covered by CI.
Description
This should be done with
pre-commit
, using the latest version of https://pypi.org/project/shellcheck-py/.It is not necessary to fix all of the problems in a single pull request.
To work on this, add the
shellcheck-py
config linked in the docs above, then run this:That'll generate a list of errors. Put up a pull request that fixes some of them, and explain in the PR description exactly which errors it fixes.
As of this writing, that check returned the following:
full list of errors (click me)
References
The text was updated successfully, but these errors were encountered: