-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
dilanbhalla
commented
Dec 14, 2022
I haven't tested this logic, as I am not sure how this is done with GitHub CLI extensions. Just wanted to offer some assistance with this feature. If one of the maintainers can help with running a test, it would be much appreciated. Also, not sure why it is failing the status check, it seems to be downloading some really old 2.6.1 database and throwing some internal CodeQL CLI failure. Would love to get some feedback on these points, thanks in advance!
aeisenberg
commented
Dec 14, 2022
Thanks for the contribution!
Looks like we're hard-coding a bunch of version numbers in the test and the older version being used is no longer able to run modern queries. I could just change the versions to something more recent, but better would be to get the latest and second latest versions for these tests.
@aeisenberg
aeisenberg
left a comment
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.
This looks reasonable to me, but I have no way of testing this. Have you tried this out? On MacOS, the extension live in $HOME/.local/share/gh/extensions/gh-codeql. I expect it's similar on windows. I think the best thing to do is manually edit the gh-codeql file to see if it works.
And if you rebase this PR on top of mine, it will be passing.
@redsun82
redsun82
left a comment
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.
@aeisenberg sorry for necroreviewing 😆 maybe we can pick this up? I was just trying to understand if install_stub works on Windows and I stumbled on this. It might be nice to pick this up at a certain point. Maybe I'll give it a spin for a test.
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.
we should add an exit /b %ERRORLEVEL% line, otherwise a command failure gets lost
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 don't think this is required for Windows batch files
aeisenberg
commented
Nov 21, 2024
@redsun82, that would be great. I have no way to test this easily. If you want to take this over and merge when you're comfortable, I would 100% support that.
rvermeulen
commented
Jan 17, 2025
We should be able to test this in an Action workflow using a Windows runner.
On Windows, gh codeql install-stub creates a bash script (codeql) which
is not discoverable by Node.js child_process.spawn(). This causes
'spawn codeql ENOENT' errors in integration tests.
Add a step that creates a codeql.cmd wrapper delegating to 'gh codeql'
so that spawn('codeql', ...) resolves correctly on Windows.
Aligns with github/gh-codeql#21 which adds native Windows support to
install-stub. This workaround can be removed once that PR is merged.
...untu-latest` and `windows-latest` (#22) * Initial plan * fix: cross-platform support for Windows compatibility - Use pathToFileURL() instead of file:// string concatenation in: - server/src/ql-mcp-server.ts (entrypoint check) - server/src/tools/codeql/language-server-eval.ts (workspace URI) - client/src/ql-mcp-client.js (entrypoint check) - Fix path.includes() to normalize separators in cli-tool-registry.ts - Replace .split('/').pop() with path.basename() for cross-platform path handling - Update client-integration-tests.yml to run on matrix of ubuntu-latest and windows-latest - Add platform-specific CI steps for process cleanup and OS dependencies - Update setup-codeql-environment action with Windows cache paths Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * refactor: improve path normalization performance using replace() instead of split/join Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * Bash on Windows as an actions workflow fix... * Initial plan * Fix Windows integration test: use bash shell for CodeQL CLI check Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * fix: create Windows-compatible codeql.cmd wrapper On Windows, gh codeql install-stub creates a bash script (codeql) which is not discoverable by Node.js child_process.spawn(). This causes 'spawn codeql ENOENT' errors in integration tests. Add a step that creates a codeql.cmd wrapper delegating to 'gh codeql' so that spawn('codeql', ...) resolves correctly on Windows. Aligns with github/gh-codeql#21 which adds native Windows support to install-stub. This workaround can be removed once that PR is merged. * Another attempted fix for client-integration-tests on Windows * Yet another attempted fix on Windows * Cleanup OS tmp dir client integration tests * Fixes for PR review comments * More fixes for latest PR review comments --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Adding option to install stub for windows (codeql.cmd) so that windows users can use this GitHub managed CLI directly within the terminal or VSCode.