-
-
Notifications
You must be signed in to change notification settings - Fork 35
Add pre-built keysender binary to avoid build issues #200
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
- Add pre-built keysender.node binary for v2.2.0 - Add postinstall script to copy pre-built binary - Configure npm to avoid rebuilding native modules - Downgrade keysender to v2.2.0 (last known working version) This change ensures that users don't need compatible Visual Studio versions to install the package, as the pre-built binary is included and automatically installed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
MCPClaude
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.
PR Review: Add pre-built keysender binary
Thank you for this PR that addresses build issues with the keysender module. I've reviewed the changes and have the following feedback:
Overall Assessment
The approach of including a pre-built binary to solve compilation issues is pragmatic, especially for Windows users struggling with newer VS versions. The implementation is straightforward and should work as intended. I have some concerns about cross-platform compatibility and security that should be addressed.
.npmrc
- The settings look appropriate for preventing automatic rebuilds
prefer-offline=trueis a good addition to improve installation speed
package.json
- The postinstall script with
|| trueensures installation continues even if binary copying fails - Good decision to downgrade to a known stable version (2.2.0)
scripts/postinstall.js
- The script is well-structured and includes proper error handling
- It uses modern ES module syntax, which requires Node.js ≥14
Issues and Suggestions:
-
Cross-platform compatibility: The PR mentions this is a Windows x64 binary, but there's no handling for other platforms (macOS, Linux). Consider:
// Add platform detection import os from 'os'; const platform = os.platform(); const arch = os.arch(); if (platform !== 'win32' || arch !== 'x64') { console.log(`[postinstall] Skipping binary copy for ${platform}-${arch}`); process.exit(0); // Exit successfully }
-
Binary validation: There's no verification that the binary is intact or hasn't been tampered with. Consider adding a checksum validation:
import crypto from 'crypto'; // Add checksum verification function verifyChecksum(filePath, expectedHash) { const fileBuffer = fs.readFileSync(filePath); const hashSum = crypto.createHash('sha256'); hashSum.update(fileBuffer); const hex = hashSum.digest('hex'); return hex === expectedHash; } // Use before copying if (!verifyChecksum(sourceFile, 'expected-hash-here')) { console.error('[postinstall] Binary validation failed, checksum mismatch'); process.exit(1); }
-
Documentation: The PR would benefit from a README update explaining:
- Why the pre-built binary is included
- Which platforms/architectures are supported
- How to compile manually if needed
- How to update the binary when needed
-
Security considerations: Pre-built binaries can pose security risks. Consider:
- Documenting the build environment used to create the binary
- Including source code version information
- Adding instructions for users who prefer to build from source
Binary file
- Unable to review the binary file directly, but ensure it was built in a secure environment
- Consider documenting which version of Visual Studio and build tools were used
Future Maintenance Considerations
- When keysender is updated, the pre-built binary will need to be rebuilt
- Consider adding automated build process to generate binaries for multiple platforms
Overall, this is a practical solution to the build issues, and with the suggested improvements, it will be more robust and maintainable.
MCPClaude
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.
While the PR provides a practical solution to the build issues with the keysender module, there are some important aspects that should be addressed before merging:
-
Platform detection: The current implementation doesn't account for non-Windows platforms. Add platform detection to handle different operating systems appropriately.
-
Binary verification: Without checksum validation, there's no way to verify the integrity of the binary. Add a verification mechanism to ensure security.
-
Documentation: Please add documentation explaining:
- The purpose of the pre-built binary
- Supported platforms/architectures
- Build environment details
- Instructions for users who want to build from source
-
Future maintenance plan: Outline a plan for maintaining these binaries when the keysender package is updated.
Once these issues are addressed, the PR will be much more robust and ready for approval. The core implementation is sound, but these security and maintenance considerations are important for a production-ready solution.
|
@MCPClaude are you there? |
Summary
Problem
Many users are experiencing build failures with the keysender module due to:
illegal qualified name in member declarationin virtual.hppSolution
This PR includes a pre-built binary (compiled with older VS version) that works correctly and is automatically installed via a postinstall script. This ensures users can install the package without needing compatible build tools.
Changes
/prebuilt/keysender/key_sender.node- pre-built Windows x64 binaryscripts/postinstall.js- copies pre-built binary to node_modulespackage.json- added postinstall script.npmrc- configures npm to avoid rebuildingTest plan
npm install🤖 Generated with Claude Code