Prebuildify#1283
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements prebuildify support for rclnodejs, enabling prebuilt native binaries for faster installation and deployment. The changes also vendor the ref-napi dependency and create a custom native module loader to handle prebuilt binaries with exact environment matching.
- Integrates prebuildify to generate prebuilt native binaries tagged with Ubuntu codename and ROS2 distribution
- Vendors ref-napi dependency to eliminate external dependency conflicts
- Implements custom native loader with exact environment matching for prebuilt binaries
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds prebuildify dependency and prebuild npm script |
| scripts/tag-prebuilds.js | Script to tag prebuilt binaries with Ubuntu/ROS2 specific information |
| lib/native_loader.js | Custom loader for prebuilt binaries with fallback to source compilation |
| third_party/ref-napi/ | Vendored copy of ref-napi dependency with integration bindings |
| binding.gyp | Updates build configuration for C++20 and ref-napi integration |
| .github/workflows/ | CI workflows for generating prebuilt binaries on Linux x64/ARM64 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| it('test generate-ros-messages script installation', function (done) { | ||
| // confirm script is installed at <pgk>/node_modules/.bin/<script> | ||
| let script = createScriptFolderPath(this.tmpPkg); | ||
| console.log(script); |
There was a problem hiding this comment.
Debug console.log statement should be removed from production test code.
| console.log(script); |
| // if (getNodeVersionInfo()[0] === 10) { | ||
| // rimraf.sync(this.tmpPkg); | ||
| // } else { | ||
| // fs.rmSync(this.tmpPkg, { recursive: true }); | ||
| // } |
There was a problem hiding this comment.
Commented out cleanup code should be removed or fixed. This will cause test artifacts to accumulate.
| // if (getNodeVersionInfo()[0] === 10) { | |
| // rimraf.sync(this.tmpPkg); | |
| // } else { | |
| // fs.rmSync(this.tmpPkg, { recursive: true }); | |
| // } | |
| if (getNodeVersionInfo()[0] === 10) { | |
| rimraf.sync(tmpPkg); | |
| } else { | |
| fs.rmSync(tmpPkg, { recursive: true }); | |
| } |
429bcad to
bfee50e
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Corrected grammar in error message from 'no digits we found' to 'no digits were found'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Corrected grammar in error message from 'no digits we found' to 'no digits were found'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Corrected grammar in error message from 'no digits we found' to 'no digits were found'.
// Copyright (c) 2020 The ref-napi Authors.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| char* ptr = AddressForArgs(args); | ||
|
|
||
| if (ptr == nullptr) { | ||
| throw Error::New(env, "readObject: Cannot write to nullptr pointer"); |
There was a problem hiding this comment.
Error message is misleading - this is in WriteObject function but says 'readObject'. Should be 'writeObject: Cannot write to nullptr pointer'.
| throw Error::New(env, "readObject: Cannot write to nullptr pointer"); | |
| throw Error::New(env, "writeObject: Cannot write to nullptr pointer"); |
| char* ptr = AddressForArgs(args); | ||
|
|
||
| if (ptr == nullptr) { | ||
| throw TypeError::New(env, "readInt64: Cannot read from nullptr pointer"); |
There was a problem hiding this comment.
Error message is inconsistent - this is in ReadInt32 function but says 'readInt64'. Should be 'readInt32: Cannot read from nullptr pointer'.
| throw TypeError::New(env, "readInt64: Cannot read from nullptr pointer"); | |
| throw TypeError::New(env, "readInt32: Cannot read from nullptr pointer"); |
0cd1d39 to
1c72186
Compare
1c72186 to
b442efd
Compare
No description provided.