Leverage prebuildify to provide prebuilt addon for npm package by minggangw · Pull Request #1287 · RobotWebTools/rclnodejs

minggangw · 2025-10-11T07:51:25Z

This PR introduces prebuildify support to provide prebuilt addon binaries for the npm package, eliminating the need to compile from source in many cases. The changes implement a smart native addon loader that prioritizes prebuilt binaries while maintaining fallback compilation capabilities.

Key changes:

Replaced direct bindings() calls with a custom native_loader.js module across all files
Added prebuild infrastructure with scripts for installation, tagging, and CI workflows
Updated package.json to include prebuildify tooling and modified installation flow

Fix: #1286

Copilot

Pull Request Overview

This PR introduces prebuildify support to provide prebuilt addon binaries for the npm package, eliminating the need to compile from source in many cases. The changes implement a smart native addon loader that prioritizes prebuilt binaries while maintaining fallback compilation capabilities.

Key changes:

Replaced direct bindings() calls with a custom native_loader.js module across all files
Added prebuild infrastructure with scripts for installation, tagging, and CI workflows
Updated package.json to include prebuildify tooling and modified installation flow

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
lib/native_loader.js	New centralized loader that handles prebuilt binary detection and fallback compilation
scripts/install.js	Installation script that checks for compatible prebuilt binaries before building from source
scripts/tag_prebuilds.js	Utility to tag prebuilt binaries with ROS/Ubuntu version information
package.json	Updated dependencies and scripts to support prebuildify workflow
Multiple lib/*.js files	Replaced `bindings('rclnodejs')` calls with `require('./native_loader.js')`
.github/workflows/*.yml	CI workflows for building prebuilt binaries on Linux x64 and ARM64
binding.gyp	Updated C++ standard to C++20 across platforms

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-11T07:51:58Z

+function detectUbuntuCodename() {
+  if (process.platform !== 'linux') {
+    return null;
+  }
+
+  try {
+    const osRelease = fs.readFileSync('/etc/os-release', 'utf8');
+    const match = osRelease.match(/^VERSION_CODENAME=(.*)$/m);
+    return match ? match[1].trim() : null;
+  } catch {
+    return null;
+  }
+}


The detectUbuntuCodename function is duplicated. This function already exists at line 36 within the customFallbackLoader function. Extract this into a shared utility function to eliminate code duplication.

coveralls · 2025-10-11T08:08:54Z

coverage: 83.268% (-1.0%) from 84.306%
when pulling 07a4058 on minggangw:fix-1286
into d4d2d7c on RobotWebTools:develop.

Copilot

Pull Request Overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot

Pull Request Overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T02:58:52Z

+**Supported Platforms:**
+
+- **Ubuntu 22.04 (Jammy)** - ROS 2 Humble
+- **Ubuntu 24.04 (Noble)** - ROS 2 Jazzy, Kilted, Rolling


[nitpick] Corrected spelling of 'Kilted' to 'Kilted' - this appears to be the correct ROS 2 distribution name, but verify if this should be 'Kilted Kaiju' for clarity.

Copilot · 2025-10-13T02:58:53Z

+      debug('Running forced node-gyp rebuild...');
+      execSync('npm run rebuild', {
+        stdio: 'inherit',
+        cwd: __dirname + '/..',


Use path.join() instead of string concatenation for cross-platform compatibility. Replace __dirname + '/..' with path.join(__dirname, '..').

Suggested change

cwd: __dirname + '/..',

cwd: path.join(__dirname, '..'),

Copilot · 2025-10-13T02:58:53Z

+      debug('Running node-gyp rebuild...');
+      execSync('npm run rebuild', {
+        stdio: 'inherit',
+        cwd: __dirname + '/..',


Use path.join() instead of string concatenation for cross-platform compatibility. Replace __dirname + '/..' with path.join(__dirname, '..').

Copilot · 2025-10-13T02:58:53Z

+    execSync('npm run rebuild', {
+      stdio: 'inherit',
+      cwd: path.join(__dirname, '..'),
+      timeout: 600000, // 10 minute timeout


The timeout value 600000 (10 minutes) is inconsistent with the 300000 (5 minutes) timeout used in native_loader.js. Consider using a consistent timeout value or defining it as a constant.

Copilot

Pull Request Overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 6 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T06:42:32Z

+    return;
+  }
+
+  const files = fs.readdirSync(prebuildDir).filter((f) => f.endsWith('.node'));


If fs.readdirSync(prebuildDir) throws an error (e.g., permission denied), the error message won't be helpful. Consider wrapping in try-catch with descriptive error handling.

Suggested change

const files = fs.readdirSync(prebuildDir).filter((f) => f.endsWith('.node'));

let files = [];

try {

files = fs.readdirSync(prebuildDir).filter((f) => f.endsWith('.node'));

} catch (err) {

console.error(`Failed to read prebuilds directory '${prebuildDir}': ${err.message}`);

return;

}

Copilot · 2025-10-13T06:42:33Z

+      });
+
+      // Load the newly built binary
+      nativeModule = require('bindings')('rclnodejs');


Missing import for 'bindings' module. The code uses require('bindings') but doesn't import it at the top of the file.

Copilot · 2025-10-13T06:42:33Z

+    // - build/Debug/rclnodejs.node
+    // - compiled/{node_version}/{platform}/{arch}/rclnodejs.node
+    // etc.
+    nativeModule = require('bindings')('rclnodejs');


Missing import for 'bindings' module. The code uses require('bindings') but doesn't import it at the top of the file.

Copilot · 2025-10-13T06:42:33Z

+      });
+
+      // Try to load the newly built binary
+      nativeModule = require('bindings')('rclnodejs');


Missing import for 'bindings' module. The code uses require('bindings') but doesn't import it at the top of the file.

Copilot · 2025-10-13T06:42:33Z

+    debug('Forcing build from source (RCLNODEJS_FORCE_BUILD=1)');
+
+    // Trigger actual compilation
+    const { execSync } = require('child_process');


The execSync import is duplicated and appears inside functions that may be called multiple times. Consider moving the import to the top of the file to avoid repeated require() calls.

Copilot · 2025-10-13T06:42:33Z

+    debug('No existing built binary found, triggering compilation...');
+
+    // Trigger actual compilation
+    const { execSync } = require('child_process');


The execSync import is duplicated and appears inside functions that may be called multiple times. Consider moving the import to the top of the file to avoid repeated require() calls.

This PR introduces prebuildify support to provide prebuilt addon binaries for the npm package, eliminating the need to compile from source in many cases. The changes implement a smart native addon loader that prioritizes prebuilt binaries while maintaining fallback compilation capabilities. Key changes: - Replaced direct `bindings()` calls with a custom `native_loader.js` module across all files - Added prebuild infrastructure with scripts for installation, tagging, and CI workflows - Updated package.json to include prebuildify tooling and modified installation flow Fix: #1286

Leverage prebuildify to provide prebuilt addon for npm package

7277443

Copilot AI review requested due to automatic review settings October 11, 2025 07:51

Copilot AI reviewed Oct 11, 2025

View reviewed changes

Address comments

3768545

minggangw requested a review from Copilot October 11, 2025 09:05

Copilot AI reviewed Oct 11, 2025

View reviewed changes

minggangw added 3 commits October 11, 2025 17:27

Address comments

d22c41b

Add prebuilt binaries

e872d09

Revise README

94e17e9

minggangw requested a review from Copilot October 13, 2025 02:57

Copilot AI reviewed Oct 13, 2025

View reviewed changes

minggangw added 4 commits October 13, 2025 11:03

Correct supported ROS2 releases

663de2b

Add prbuild section into npm README

c2b618c

Address comments

037e562

Abstract detectUbuntuCodename

8330f4a

minggangw requested a review from Copilot October 13, 2025 06:41

Copilot AI reviewed Oct 13, 2025

View reviewed changes

minggangw added 2 commits October 13, 2025 14:47

Address comments

72e5606

Address comments

07a4058

minggangw merged commit 3765085 into RobotWebTools:develop Oct 13, 2025
18 checks passed

minggangw mentioned this pull request Oct 13, 2025

Prebuildify #1283

Closed

-  const files = fs.readdirSync(prebuildDir).filter((f) => f.endsWith('.node'));
+  let files = [];
+  try {
+    files = fs.readdirSync(prebuildDir).filter((f) => f.endsWith('.node'));
+  } catch (err) {
+    console.error(`Failed to read prebuilds directory '${prebuildDir}': ${err.message}`);
+    return;
+  }

Uh oh!

Conversation

minggangw commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

minggangw commented Oct 11, 2025 •

edited

Loading

coveralls commented Oct 11, 2025 •

edited

Loading