Only add typical CUDA paths to the linker search paths if no explicit path is supplied#475
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly modifies the build script to only use typical CUDA search paths when no explicit environment variables like CUDA_PATH are set. This helps prevent linker errors in environments like Conda. It also adds a helpful warning for Conda users. The changes are logical and address the described problem. I have one suggestion to improve the implementation's efficiency by using iterators instead of collecting into vectors, which will reduce memory allocations in the build process.
| let env_vars = TYPICAL_CUDA_PATH_ENV_VARS | ||
| .iter() | ||
| .map(std::env::var) | ||
| .filter_map(Result::ok); | ||
| .filter_map(Result::ok) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
Collecting the environment variables into a Vec here is inefficient as it forces an allocation. The subsequent logic then requires another allocation if no environment variables are found. This can be made more performant and idiomatic by using iterators.
Consider refactoring this part of link_searches to use a peekable() iterator and a trait object (Box<dyn Iterator<...>>) to avoid allocations:
// In link_searches function
let mut env_vars_iter = TYPICAL_CUDA_PATH_ENV_VARS
.iter()
.map(std::env::var)
.filter_map(Result::ok)
.peekable();
let no_env_vars = env_vars_iter.peek().is_none();
// ... conda warning logic using `no_env_vars` ...
let roots_iter: Box<dyn Iterator<Item = PathBuf>> = if no_env_vars {
Box::new(typical_locations.into_iter().map(PathBuf::from))
} else {
Box::new(env_vars_iter.map(PathBuf::from))
};
for root in roots_iter {
// ... existing loop body
}This approach avoids intermediate collections, reducing memory allocations and making the code cleaner.
|
Great improvement, ty! |
In the build script
fn link_searches(...)figures out where CUDA libraries may be.There are two ways of generating linker search paths
CUDA_PATH/usr.Currently, the way the linker search paths are generated is that any valid paths generated from 2 are always appended to the linker search paths.
This is a problem when building in Conda environments, because this will cause the linker to see all system-installed libraries. This will cause obscure linker errors as shown below, because Conda environments may use a different
libcetc. than the system-installed one:Therefore, this PR does two things:
CUDA_PATH-like environment variable is defined during building, it will no longer add any "standard locations" to the linker search paths in order to prevent system libraries from being exposed to the linker.CUDA_PATH-like environment variables, just to help out folks running into this scenario.