Skip to content

Make the driver feature not depend on nvrtc#468

Merged
chelsea0x3b merged 1 commit into
chelsea0x3b:mainfrom
johanpel:nvrtc-optional
Oct 28, 2025
Merged

Make the driver feature not depend on nvrtc#468
chelsea0x3b merged 1 commit into
chelsea0x3b:mainfrom
johanpel:nvrtc-optional

Conversation

@johanpel

Copy link
Copy Markdown
Contributor

The driver feature depends on the nvrtc feature. Remove this dependency and feature-gate its usages (mostly in tests). This is useful when applications use the driver but don't perform runtime compilation of CUDA code.

@johanpel johanpel requested a review from chelsea0x3b as a code owner October 28, 2025 13:39

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly decouples the driver feature from nvrtc by feature-gating the parts of the code that depend on runtime compilation. The changes are logical and well-contained. I have one suggestion to improve the maintainability of the test code in src/driver/safe/launch.rs by applying the feature gate at the module level.

Comment thread src/driver/safe/launch.rs
Comment on lines 274 to 284
#[cfg(test)]
mod tests {
use crate::{
driver::{CudaContext, DriverError},
nvrtc::compile_ptx_with_opts,
};
use crate::driver::{CudaContext, DriverError};
#[cfg(feature = "nvrtc")]
use crate::nvrtc::compile_ptx_with_opts;

use super::*;

#[cfg(feature = "nvrtc")]
#[test]
fn test_launch_arrays() -> Result<(), DriverError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

For better maintainability, you could apply the nvrtc feature gate to the entire tests module instead of each individual test. This avoids repetition and ensures future tests in this file that rely on nvrtc are correctly gated. You've used a similar pattern in src/driver/safe/unified_memory.rs.

This change would make the code more concise and less error-prone.

Suggested change
#[cfg(test)]
mod tests {
use crate::{
driver::{CudaContext, DriverError},
nvrtc::compile_ptx_with_opts,
};
use crate::driver::{CudaContext, DriverError};
#[cfg(feature = "nvrtc")]
use crate::nvrtc::compile_ptx_with_opts;
use super::*;
#[cfg(feature = "nvrtc")]
#[test]
fn test_launch_arrays() -> Result<(), DriverError> {
#[cfg(all(test, feature = "nvrtc"))]
mod tests {
use crate::{
driver::{CudaContext, DriverError},
nvrtc::compile_ptx_with_opts,
};
use super::*;
#[test]
fn test_launch_arrays() -> Result<(), DriverError> {

@chelsea0x3b chelsea0x3b merged commit fe342da into chelsea0x3b:main Oct 28, 2025
18 checks passed
@chelsea0x3b

Copy link
Copy Markdown
Owner

Ty for the pr! easy review

@johanpel johanpel deleted the nvrtc-optional branch October 29, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants