Conversation
…soem-addon Replace cmake-js with node-gyp build flow
…zation, and update README and CHANGELOG for build system migration
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the build system from CMake/cmake-js to node-gyp, simplifying the build process and dependencies. The change removes the need for CMake while maintaining full compatibility with Node.js and Electron environments.
- Replaces CMake-based build with node-gyp and binding.gyp configuration
- Updates all build scripts and CI workflows to use node-gyp instead of cmake-js
- Modernizes test infrastructure with improved mock handling and stub mode support
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| binding.gyp | New node-gyp build configuration replacing CMakeLists.txt |
| package.json | Updates build scripts and dependencies from cmake-js to node-gyp |
| scripts/postinstall.js | Refactored to use node-gyp instead of cmake-js for native addon building |
| scripts/rebuild-electron.js | Complete rewrite to use node-gyp for Electron rebuilds |
| scripts/generate-ec-options.js | New utility script for generating SOEM configuration headers |
| test files | Enhanced test infrastructure with better mock handling and stub mode support |
| CI/build scripts | Updated to use node-gyp throughout the build pipeline |
| README.md | Updated documentation removing CMake references and simplifying setup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Enregistrer des placeholders test.todo pour indiquer comment activer ces tests | ||
| describe('Integration tests (disabled by default)', () => { | ||
| test.todo('Set RUN_INTEGRATION=true pour exécuter avec l’addon natif'); | ||
| test.todo('Set RUN_INTEGRATION_STUB=true pour exécuter avec le stub JS'); |
There was a problem hiding this comment.
[nitpick] Mix of French and English in comments and test descriptions. Consider using consistent language throughout the codebase.
| // Enregistrer des placeholders test.todo pour indiquer comment activer ces tests | |
| describe('Integration tests (disabled by default)', () => { | |
| test.todo('Set RUN_INTEGRATION=true pour exécuter avec l’addon natif'); | |
| test.todo('Set RUN_INTEGRATION_STUB=true pour exécuter avec le stub JS'); | |
| // Register test.todo placeholders to indicate how to enable these tests | |
| describe('Integration tests (disabled by default)', () => { | |
| test.todo('Set RUN_INTEGRATION=true to run with the native addon'); | |
| test.todo('Set RUN_INTEGRATION_STUB=true to run with the JS stub'); |
| const isNotVirtual = !/WAN Miniport|Loopback|Virtual/i.test(best.description); | ||
|
|
||
| if (interfaces.some(iface => /Intel.*Gigabit/i.test(iface.description))) { | ||
| if (interfaces.some((iface: any) => /Intel.*Gigabit/i.test(iface.description))) { |
There was a problem hiding this comment.
Using any type annotation defeats TypeScript's type safety. Consider defining a proper interface type for network interfaces or using the existing interface structure.
| expect(console.warn).toHaveBeenCalledWith( | ||
| 'Warning: Could not list network interfaces:', | ||
| 'Test error' | ||
| 'Warning: Could not list network interfaces:', 'Test error' |
There was a problem hiding this comment.
[nitpick] The error message assertion spans multiple arguments but could be more robust. Consider using expect.stringContaining() or a single string matcher to avoid brittleness from argument splitting.
| 'Warning: Could not list network interfaces:', 'Test error' | |
| expect.stringContaining('Warning: Could not list network interfaces: Test error') |
| else | ||
| print_warning "Fichier .node non trouvé" | ||
| fi | ||
| if npx node-gyp configure build; then |
There was a problem hiding this comment.
[nitpick] The script runs configure and build in a single command. Consider separating these steps for better error handling and debugging, as configure failures have different root causes than build failures.
| ], | ||
| 'include_dirs': [ | ||
| '<!(node -p "require(\'node-addon-api\').include")', | ||
| '<(module_root_dir)/node_modules/node-addon-api', |
There was a problem hiding this comment.
The include directory is specified twice - once dynamically via node-addon-api and once with a hardcoded path. The hardcoded path on line 19 is redundant and could cause conflicts.
| '<(module_root_dir)/node_modules/node-addon-api', |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📋 Description
Change N-api to node-gyp
🔄 Type of Change
🧪 How Has This Been Tested?
Test Configuration:
📋 Checklist