-
Notifications
You must be signed in to change notification settings - Fork 4
ci: run ci on macos and windows #127
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?
Conversation
696ff55
to
b3f8d42
Compare
Co-authored-by: Amour1688 <[email protected]>
…es (#130) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Amour1688 <[email protected]>
Co-authored-by: Amour1688 <[email protected]>
…es (#130) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Amour1688 <[email protected]>
…es (#130) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Amour1688 <[email protected]>
…es (#130) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Amour1688 <[email protected]>
b3f8d42
to
ec9ed12
Compare
|
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.
Pull Request Overview
This PR adds support for running CI on macOS and Windows platforms, in addition to the existing Linux support. The changes implement cross-platform build tooling and binary distribution for the rslint TypeScript/JavaScript linter.
- Cross-platform CI configuration for Linux, macOS, and Windows
- Multi-platform binary build system with platform-specific npm packages
- File casing fixes for cross-platform compatibility
- Platform-aware binary resolution in the VS Code extension and core package
Reviewed Changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
.github/workflows/ci.yml |
Added matrix strategy to run tests on Linux, macOS, and Windows |
scripts/build-npm.mjs |
Rewrote build script to support platform-specific binary generation |
packages/vscode-extension/src/main.ts |
Fixed import casing from Extension to extension |
packages/vscode-extension/src/extension.ts |
Fixed import casing from Rslint to rslint |
packages/vscode-extension/src/Rslint.ts |
Fixed import casing from Extension to extension |
packages/vscode-extension/scripts/build.js |
Updated to use platform-specific binary resolution |
packages/vscode-extension/package.json |
Added dev dependencies for all platform binary packages |
packages/rslint/src/service.ts |
Updated binary path resolution to use platform-specific packages |
packages/rslint/package.json |
Updated build script to use new platform-aware build tool |
npm/*/package.json |
Added exports field for binary path resolution |
internal/config/config.go |
Added pathMatch wrapper for cross-platform path handling |
.gitattributes |
Added file for consistent line endings across platforms |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
} | ||
|
||
main(); | ||
const args = process.argv.slice(2); | ||
const command = args[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.
The command is being extracted from args[1], but args is already process.argv.slice(2). This means the command would actually be at args[0], not args[1]. This will cause all commands to be undefined and fall through to the default case.
const command = args[1]; | |
const command = args[0]; |
Copilot uses AI. Check for mistakes.
@@ -1,5 +1,5 @@ | |||
import { spawn, ChildProcess } from 'child_process'; | |||
import path from 'path'; | |||
import { platform, arch } from 'os'; |
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 import should use 'node:os' instead of 'os' to be consistent with Node.js best practices for built-in modules, as seen in other files in this PR.
import { platform, arch } from 'os'; | |
import { platform, arch } from 'node:os'; |
Copilot uses AI. Check for mistakes.
return { | ||
GOOS, | ||
GOARCH, | ||
ext, |
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 getPlatformConfig function returns an object without the binPath property, but the JSDoc comment indicates it returns PlatformConfigWithBinPath which should include binPath. This will cause runtime errors when the binPath property is expected.
ext, | |
ext, | |
binPath: `npm/${platform}-${arch}/rslint${ext}`, |
Copilot uses AI. Check for mistakes.
No description provided.