Generate missing messages at runtime#1257
Conversation
minggangw
force-pushed
the
fix-1254
branch
3 times, most recently
from
eb7763e to
cccca5b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements runtime message generation capabilities for rclnodejs, allowing custom ROS2 message types to be dynamically discovered, built, and loaded during execution rather than requiring pre-generation. This addresses issue #1257 by adding automatic discovery and synchronous generation of missing message interfaces.
- Adds runtime message discovery and generation functionality to the interface loader
- Implements synchronous message generation using worker processes to avoid blocking the main thread
- Includes comprehensive test infrastructure with a custom message package for validation
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/test-rosidl-message-generator.js |
Adds test case for runtime message generation with custom test package setup |
test/custom_msg_test/ |
Complete ROS2 package with custom message definitions for testing runtime generation |
rosidl_gen/index.js |
Adds synchronous worker-based message generation function and exports helper utilities |
rosidl_gen/generate_worker.js |
New worker script for performing message generation in separate process |
lib/interface_loader.js |
Implements runtime message discovery and generation when interfaces are not found |
README.md |
Updates documentation to indicate manual message generation is no longer required |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const idlPath = path.join(generatedRoot, 'share'); | ||
| const useIDL = !!process.argv.find((arg) => arg === '--idl'); | ||
|
|
||
| // Get target path from environment variable instead of workerData |
There was a problem hiding this comment.
This comment references 'workerData' but the code uses child_process.spawnSync, not worker_threads. The comment should be updated to reflect the actual implementation using environment variables and spawn processes.
| // Get target path from environment variable instead of workerData | |
| // Get target path from the WORKER_TARGET_PATH environment variable |
| // We are going to ignore the path ROS2 installed. | ||
| if (pkgPath.includes('ros2-linux')) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The hardcoded check for 'ros2-linux' is platform-specific and may not work on other operating systems or ROS2 installation methods. Consider using a more robust method to identify system ROS2 installations or make this configurable.
| } | ||
| } catch (err) { | ||
| // Skip directories we can't read | ||
| console.error('Error reading directory:', dir, err.message); |
There was a problem hiding this comment.
This error message is logged to console.error but doesn't provide enough context about what operation was being performed. Consider using a more descriptive message like 'Failed to read directory during message discovery:' or use a proper logging mechanism.
| console.error('Error reading directory:', dir, err.message); | |
| console.error( | |
| `Failed to read directory during message discovery for package "${packageName}", type "${type}", message "${messageName}". Directory: ${dir}. Error: ${err.message}` | |
| ); |
This PR implements runtime message generation capabilities for rclnodejs, allowing custom ROS2 message types to be dynamically discovered, built, and loaded during execution rather than requiring pre-generation. - Adds runtime message discovery and generation functionality to the interface loader - Implements synchronous message generation using worker processes to avoid blocking the main thread - Includes comprehensive test infrastructure with a custom message package for validation Fix: #1257
This PR implements runtime message generation capabilities for rclnodejs, allowing custom ROS2 message types to be dynamically discovered, built, and loaded during execution rather than requiring pre-generation.
Fix: #1257