-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix Node.js binding path to match node-pre-gyp versioning #7277
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
Fix Node.js binding path to match node-pre-gyp versioning #7277
Conversation
|
Hi @DennisOSRM and @SiarheiFedartsou please take a look at my fix |
|
Thanks for this @afarber. I wasted so much time trying to understand what was going on! |
43baee5 to
05a1f30
Compare
|
Hi @DennisOSRM please consider merging this fix and then releasing the 6.0.1 version |
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 fixes the Node.js binding path to align with node-pre-gyp versioning conventions by updating the binding directory from lib/binding to lib/binding_napi_v8.
Key changes:
- Updated BINDING_DIR in CMakeLists.txt to use
lib/binding_napi_v8and added directory creation during CMake configuration - Removed the
lib/binding_napi_v8symlink that previously pointed tolib/binding - Updated all references across code, documentation, CI scripts, and configuration files to use the new path
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/nodejs/CMakeLists.txt | Updates BINDING_DIR variable to new path and adds file(MAKE_DIRECTORY) command to ensure directory exists |
| lib/index.js | Updates require path to load native binding from new location |
| src/nodejs/node_osrm.cpp | Updates documentation comments with corrected binary paths |
| docs/nodejs/api.md | Updates API documentation examples with new binding path |
| package.json | Updates nodejs-tests script to reference new binary location |
| scripts/ci/run_benchmarks.sh | Updates benchmark script to use new node_osrm.node path |
| scripts/ci/node_package.sh | Updates packaging script to check binary in new location |
| .github/workflows/osrm-backend.yml | Updates commented-out test commands with new binary paths |
| .gitignore | Updates ignored directory from lib/binding to lib/binding_napi_v8 |
| lib/binding_napi_v8 | Removes symlink that previously pointed to binding directory |
| CHANGELOG.md | Adds entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DennisOSRM
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.
Good sleuthing finding this fix. 😉
Fix #7272 by:
lib/bindingtolib/binding_napi_v8to align with node-pre-gyp's versioningconventions
file(MAKE_DIRECTORY)to create the binding directory during CMake configurationTasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?