-
Notifications
You must be signed in to change notification settings - Fork 10.5k
fix(scripts): Add Windows (win32/x64) support to lint.js #16193
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
base: main
Are you sure you want to change the base?
fix(scripts): Add Windows (win32/x64) support to lint.js #16193
Conversation
Summary of ChangesHello @ZafeerMahmood, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request aims to add Windows support to the lint.js script. While previously identified command injection vulnerabilities related to environment variables are acceptable for developer-facing build scripts in trusted environments, there are still critical functional issues with the shellcheck installer for Windows. These include an incorrect platform architecture identifier, a wrong download URL, and faulty extraction logic that doesn't correctly handle the zip archive's directory structure. Additionally, consider refactoring the use of execSync with shell: true for improved robustness, even if the immediate security risk from environment variables is mitigated.
|
/gemini review |
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.
Code Review
This pull request adds Windows support to the lint.js script, which is a great improvement for cross-platform development. However, a critical issue was found in the Windows installer logic for shellcheck that would prevent it from working, specifically regarding an incorrect download URL and executable renaming after extraction. Detailed comments and a code suggestion are provided for this fix.
| ? `powershell -Command "` + | ||
| `Invoke-WebRequest -Uri 'https://github.com/koalaman/shellcheck/releases/download/v${SHELLCHECK_VERSION}/shellcheck-v${SHELLCHECK_VERSION}.zip' -OutFile '${TEMP_DIR}/.shellcheck.zip'; ` + | ||
| `Add-Type -AssemblyName System.IO.Compression.FileSystem; ` + | ||
| `[System.IO.Compression.ZipFile]::ExtractToDirectory('${TEMP_DIR}/.shellcheck.zip', '${TEMP_DIR}/shellcheck')"` |
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.
The Windows installer for shellcheck has a couple of issues that will prevent it from working correctly:
- Incorrect URL: The download URL is hardcoded to
.../shellcheck-v${SHELLCHECK_VERSION}.zip, which will result in a 404 error. The URL needs to include the platform and architecture, similar to the other installers. The correct asset for Windows is named likeshellcheck-vX.Y.Z.win.x86_64.zip. - Incorrect Executable Name: The zip file contains a versioned executable (e.g.,
shellcheck-v0.11.0.exe). After extraction, the script will look forshellcheck.exein thePATHand fail to find it. The installer needs to rename the executable toshellcheck.exeafter extraction.
I've included a suggestion that also adds a New-Item command for consistency with the actionlint installer and fixes both issues.
? `powershell -Command "` +
`New-Item -ItemType Directory -Force -Path '${TEMP_DIR}/shellcheck' | Out-Null; ` +
`Invoke-WebRequest -Uri 'https://github.com/koalaman/shellcheck/releases/download/v${SHELLCHECK_VERSION}/shellcheck-v${SHELLCHECK_VERSION}.${platformArch.shellcheck}.zip' -OutFile '${TEMP_DIR}/.shellcheck.zip'; ` +
`Add-Type -AssemblyName System.IO.Compression.FileSystem; ` +
`[System.IO.Compression.ZipFile]::ExtractToDirectory('${TEMP_DIR}/.shellcheck.zip', '${TEMP_DIR}/shellcheck'); ` +
`Move-Item -Path '${TEMP_DIR}/shellcheck/shellcheck-v${SHELLCHECK_VERSION}.exe' -Destination '${TEMP_DIR}/shellcheck/shellcheck.exe'"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.


Summary
Add Windows (win32/x64) support to scripts/lint.js to fix the "Unsupported platform/architecture" error when running npm run preflight on Windows.
Details
Related Issues
Fixes #15074
How to Validate
node scripts/lint.js --setup
node scripts/lint.js --eslint
- On macOS/Linux, run node scripts/lint.js --setup - should work as before
Pre-Merge Checklist