-
Notifications
You must be signed in to change notification settings - Fork 19
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
add control of libusb DLL on Windows #48
add control of libusb DLL on Windows #48
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.
Thanks.
Informatively, what is the main difference between this approach and approach before? Both cases require the DLL to reside along the target exe/dll, correct?
Also, will then the final behavior differ between MinGW & MSVC compilers in terms of how DLLs will have to be applied (copied over to target dirs?, ...), etc...?
Discussed #37 (comment) Users may not have permissions in which to save the depthai "app", depthai-core.dll, and libusb.dll into those protected folders. The Windows OS and its Win32 api tightly controls the search path for DLLs; both historical and security reasons. With this PR there is no need to copy of libusb.dll to special folders or hack It is my understanding that MinGW is hacky Unix. It uses unix apis, unix behaviors, unix directories, etc. This PR does not change anything about the loading path of Unix-like systems. I am not aware of *nix loading issue with libusb. The issue #37 this helps to resolve is not a *nix bug. |
Here is Windows dll loading sequence https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications and a convo on MinGW and delayload https://stackoverflow.com/questions/1851267/mingw-gcc-delay-loaded-dll-equivalent |
pushed changed w/ feedback. Caution there was a change to code within |
...and putting all that code in |
Please do - its quite specific code, probably best to be kept more on the side. WRT: luxonis/depthai-core#473 (comment) Can we do the same without If not, what are the issues with leaving EDIT - If I'm understanding
|
When For example, if depthai-core is configured to be a shared library, and we are cmake building, and That means, if For example, my app with a shared depthai-core: if I use today's v2.15.3 and its PUBLIC linked XLink, then cmake propogates the link library and options downstream to my app. During the link of my app, it uses the Libusb is a dependency of xlink (not depthai-core, not downstream apps). And XLink is a dependency of depthai-core (except for that one xlink header...this feels like a special case). depthai-core.dll has already been compiled and it already has the delayload stub for libusb within it; because xlink was static and therefore xlink itself is within depthai-core.dll. Even if we use something like RUNTIME so downstream apps can find libusb.dll at the moment of linking, it is logically wrong. Downstream apps don't need to delayload libusb.dll. That's the job of Xlink. We don't need that library to be linked twice, or that DLL to be delayloaded twice. Yet, that is what is happening today with v2.15.3; xlink is being double linked. 🙈 The PUBLIC linking of Xlink is already imperfect/errant. A beneficial sideaffect was getting the include path for that one xlink header (the special case). Double linking of XLinkBelow are two pieces of This is depthai-core
Notice above the link libraries It is linking in Compare that same configuration with my depthai-core PR
The include directory for that one xlink header, linker option
...and to handle that special case of the one header file |
I think the only special case is the Regarding duplicate linking of XLink - not sure why there are 2 entries, but I don't think this is a problem - rechecking what build command outputs CMake on Unix /w Ninja & clang with same scenario. Either case, there should be at least 1 entry for XLink - because one "COULD" call eg:
I think this should be the case already - if its shared, XLink code will reside in core, and static link will be added downstream. But that code will only be used IFF some functions are used AFAIK. Compilation on Linux /w Ninja&Clang with core SHARED & XLink as PUBLIC dependency - for rgb_preview example: - Note linking libXLink.a as rgb_preview could call some functions of XLink
Compilation on Linux /w Ninja&Clang with core shared & XLink as PRIVATE dependency - for rgb_preview example: - Note no linking with XLink - but also doesn't propagate anything else (headers, defines, ...)
|
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.
Haven't tested yet, not on wins currently
@diablodale do you mind checking this change out? (stock depthai-core, with PUBLIC linking of XLink)
CMakeLists.txt
Outdated
# BUGBUG private fails...it does not put /delayload on tests or examples | ||
# interface and public both cascade up and put it on things "higher" than xlink and depthai-core | ||
# I think this is a bug in depthai-core cmake | ||
target_link_options(${TARGET_NAME} PUBLIC "/DELAYLOAD:libusb-1.0$<$<CONFIG:Debug>:d>.dll") |
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.
Okay, it seems we have this as generator expression: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html?highlight=link_only#genex:LINK_ONLY
I think this is actually the "solve it all" for the discussed issues at hand
target_link_options(${TARGET_NAME} PUBLIC "/DELAYLOAD:libusb-1.0$<$<CONFIG:Debug>:d>.dll") | |
target_link_options(${TARGET_NAME} PUBLIC "$<LINK_ONLY:/DELAYLOAD:libusb-1.0$<$<CONFIG:Debug>:d>.dll>") |
The single header The depthai-core project provides apis to use Luxonis sensors. It is not a general library that exposes XLink and libusb apis. My "vote" is that such is not allowed, and therefore linking should be PRIVATE. The executable code of XLink is linked directly into depthai-core.dll. When depthai-core.dll is loaded, that specific copy of XLink is loaded into memory including all its globals, TLS, etc. There's more to discuss, but I think this needs to be resolved first. |
Hmm, then its INTERFACE in case of dll and PUBLIC in case of static Also, this could be done for all dependencies in that case. Not sure what will happen to transitive dependency libusb in this case though... |
hi, I will be a delayed a few days. Will get back to this no later than Monday 16th, probably sooner. |
I read some related info on this, and I keep coming back to that single public xlink header is the special case. [EDIT] Maybe projects make separate folders for their private headers and public headers. Those projects do not have the issues we are encountering. cmake operates on whole include directories, not individual files. So we don't have a way to tell cmake all the includes are private...except this one which is public. Some of the discussions on this I read related to cmake What if we explored....
Or we could move that one include file to a different directory. And go from there. |
This ^ - it seems that linking only to "headers/defines" isn't possible. I (wrongfully) thought that this is what If we want to go with this method, then best case option is creating an INTERFACE target, doing just that. I've added this and pushed. Other options would cascade things too much (eg, adding just include dir of XLink to depthai-core include dirs, etc...) With above, I don't think that LINK_ONLY additions are needed, but for sake of completeness I'd still add them. Some discussion /w my thoughts on this here: https://discourse.cmake.org/t/add-only-library-headers-during-target-link-libraries/2973 |
Pushed to both XLink and core. @diablodale feel free to checkout |
I think we are closer :-) I had a cmake config fail when your changes to xlink+depthai-core were used on my app. |
With core CMake issues now resolved, do device search improvements + these changes pass as expected? |
I'm working on this PR now; separating to a file and removing debug. Quick tests worked. I used this PR when I tested the depthai-core find(xlink) |
197b2fc
to
bbf2c7b
Compare
Force pushed... What do you think of commit 2? The approach could be done many ways, if you prefer another just ask me. FYI: I think mingw might have a similar solution if needed, but requires using the mingw external tool |
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 - LGTM
+1 on MinGW. Its mostly supported on per need basis.
Will you add in the LINK_ONLY option for the DELAYLOAD as well? I think its fits, even if we don't require it per se now.
Our approach went a different direction with xlink PRIVATE and xlinkpublic INTERFACE in your two PRs. I haven't...yet...seen DELAYLOAD on any compile. And I only see DELAYLOAD on the single correct link. Just now, I also search through the ninja.build for depthai-core and my app. Do you have a scenario/repro that causes DELAYLOAD to appear in the wrong location? Haha, I retested what I already tested. I'll squash the two commits, remove a BUGBUG, and force push. |
bbf2c7b
to
8181f6f
Compare
Ah, https://gitlab.kitware.com/cmake/cmake/-/issues/20022 and cmake policy https://cmake.org/cmake/help/latest/policy/CMP0099.html is the issue+fix why I have to make it PUBLIC with |
* load libusb from same dir as depthai-core on Windows * add xlink init error handling * add return success to XLinkPlatformInit() * xlink init unlocks mutex on errors + allows retry
8181f6f
to
489b2bf
Compare
added LINK_ONLY as requested. I did not see any change in behavior, the same DELAYLOAD params were the same everywhere I could see. You want it, I see no harm, and it is related to a bugfix of above. force-pushed :-) |
Thanks! Still, main focus is playing nice with core so as long as everything remains functioning as expected I think its good to go ahead. Merging, and starting CI over at core - thanks again for you contribution! |
NOT READY FOR MERGE
This PR controls the location in which Windows searches+loads the libusb DLL.
The libusb DLL should be in the same folder as the binary code that is/contains XLink. The primary config of depthai-core is to have a static XLink that is linked into depthai-core. Therefore...
Approach