Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support loading dynamic libraries from multiple paths and try more paths in default loader #1864

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ewencp
Copy link

@ewencp ewencp commented Mar 23, 2022

Change the DynamicLibraryLoader::new interface to accept a slice of paths
instead of a single path. It tries to load them in order, using the first one it is
able to load. Update the auto_loader implementation to use the updated
version, including a larger set of paths for the Vulkan library.

 - `**Breaking** DynamicLibraryLoader::new now accepts a slice of paths.`
  1. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

  2. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    files(CHANGELOG_VULKANO.md and CHANGELOG_VK_SYS.md)
    by maintainers right after the Pull Request merge.

    • Entries for Vulkano changelog:

      • **Breaking** Breaking entry description.
      • Non-breaking entry description....
    • Entries for VkSys changelog:

      • Entry 1.
      • Entry 2....
  3. Run cargo fmt on the changes.

  4. Make sure that the changes are covered by unit-tests.

  5. Update documentation to reflect any user-facing changes - in this repository.

@ewencp
Copy link
Author

ewencp commented Mar 23, 2022

For reference, the list of paths is the same as used by SDL. This makes development on macos using the home-brew molten-vk package simpler since with this update the dylib can be found automatically (assuming paths are configured correctly). Otherwise you are forced to load the FunctionPointers yourself.

Copy link
Member

@AustinJ235 AustinJ235 left a comment

Choose a reason for hiding this comment

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

Overall this is probably a good change for MacOS. Would other platforms also have multiple paths?

i.e. sdl's wayland code has this:

#if defined(__OpenBSD__)
#define DEFAULT_VULKAN  "libvulkan.so"
#else
#define DEFAULT_VULKAN  "libvulkan.so.1"
#endif

Comment on lines +105 to +116
let get_proc_addr = {
let ptr: *mut c_void = dl
.symbol("vkGetInstanceProcAddr")
// Arguably we could continue trying subsequent paths here, but we don't
// expect to find a matching filename without the right contents, so better
// to fail here, especially since an unknown library is now loaded into the
// process.
.map_err(|_| {
LoadingError::MissingEntryPoint("vkGetInstanceProcAddr".to_owned())
})?;
mem::transmute(ptr)
};
Copy link
Member

Choose a reason for hiding this comment

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

DynamicLibrary seems to implement Drop, so would it be safe to continue attempting paths?
https://docs.rs/shared_library/0.1.9/shared_library/dynamic_library/struct.DynamicLibrary.html#impl-Drop

I don't know much about loading library, so I don't know what is safe and what isn't.

get_proc_addr,
})
}
None => Err(LoadingError::LibraryLoadFailure(errors.join(" "))),
Copy link
Member

Choose a reason for hiding this comment

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

I not sure I am a huge fan of the space separated errors. shared_library isn't very helpful with just returning a String. Perhaps the errors should be a Vec<LoadingError { path: Path, error: String }>?

@Rua
Copy link
Contributor

Rua commented Mar 23, 2022

Vulkano uses Ash, and Ash has its own loading implementation. Perhaps these changes would be useful for them as well? I've been considering making Vulkano use Ash's loading code so then Vulkano would also benefit.

@Eliah-Lakhin Eliah-Lakhin added the PR: In review. Comments to address. Maintainer started review process, and put additional questions and comments to be addressed. label Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: In review. Comments to address. Maintainer started review process, and put additional questions and comments to be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants