-
Notifications
You must be signed in to change notification settings - Fork 23
Add option to use Windows default DLL search strategy (for Conda) #972
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine if this "just works" for conda, I don't really see a better way.
Make sure to merge #973 into this first if you agree with that change
crates/harp/src/library.rs
Outdated
| #[cfg(windows)] | ||
| use std::sync::atomic::AtomicBool; | ||
| #[cfg(windows)] | ||
| use std::sync::atomic::Ordering; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've tried hard to contain OS specific stuff in sys/windows/ or sys/unix/ so we can avoid the cfgs everywhere
See #973 for an alternate approach that you can merge into this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, that's better!
crates/ark/src/main.rs
Outdated
|
|
||
| // Windows-specific options | ||
| #[cfg(target_os = "windows")] | ||
| print!(r#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I save in this file, rust-analyzer reformats this a little. Can you please check the formatting first before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like rustfmt was failing silently on my Windows machine. 😭 Fixed.
|
Adding some extra context to this for future us R's DLL is located in In particular, To work around this, we instead use:
This was enough for regular R, but conda R is different. In Conda R, the Conda will put this location on the Conda will also put these locations on the These are DLLs that Conda's version of R specially links to, note that these are not found by But note how these are found by Window's standard dll search ordering. So, here's the summary: Regular R:
Conda R:
Since there is no easy way to detect when we need to switch between these approaches, adding a CLI option seemed like the best option |
Co-authored-by: Davis Vaughan <davis@rstudio.com>
Ark's default DLL search strategy doesn't work for Conda R since it depends on libraries shipped with Conda that are not placed in either of the locations Ark currently looks (alongside the R binary and in the same path as
R.dllitself).This change adds a
--dll-search-pathoption (only for Windows) that searchesPATHfor DLLs, or (more accurately) it enables Windows' default DLL search behavior, which includes checkingPATH.The change shouldn't affect DLL resolution for other R installations; for those, we continue to use the same strategy we do today.
Part of posit-dev/positron#4398.