luxonis / XLink

A cross-platform library for communicating with devices over various physical links.
Apache License 2.0
12 stars 18 forks source link

add control of libusb DLL on Windows #48

Closed diablodale closed 2 years ago

diablodale commented 2 years ago

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...

  1. If you have shared lib (aka DLL) of depthai-core, then libusb DLL should be in the same folder as depthai-core.dll
  2. If you have static lib depthai-core, then your main app "contains" depthai-core. Then, libusb DLL should be in the same folder as your main app.
  3. If you customed and compiled your own depthai-core so that XLink is a separate shared lib (aka DLL), then libusb DLL should be in the same folder as XLink.dll. This is probably a rare scenario.

Approach

diablodale commented 2 years ago

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 https://github.com/luxonis/XLink/issues/37#issuecomment-1119015060 Main difference is that the existing code does not work when the main depthai-core "app" is not the top-level process EXE. Existing code will fail in scenarios like unreal, unity, cycling74 max, probably python, "plugins", etc. For all of them, the main process EXE is python.exe, unreal.exe, unity.exe, max.exe. Those top-level EXEs are usually in permissions restricted locations like c:\program files\cycling74\max 7\max.exe or c:\Program files\python37\python.exe.

Users may not have permissions in which to save the depthai "app", depthai-core.dll, and libusb.dll into those protected folders. And users typically don't want to do that. And app writers like me don't want to build install processes to do that. We all want folders separate from c:\program files\unreal\ to contain our "app" and its supporting DLLs.

The Windows OS and its Win32 api tightly controls the search path for DLLs; both historical and security reasons. This PR uses the Win32 api to temporarily add the OP described directory to the DLL load sequence so that libusb.dll can be found.

With this PR there is no need to copy of libusb.dll to special folders or hack PATH. Instead, just copy depthai-core.dll and libusb.dll to the same folder. And if it is all static, then copy libusb.dll to the statically linked EXE.

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.

diablodale commented 2 years ago

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 Seems delayload is primarily a Microsoft Linker feature. So it is up to MinGW toolchain+WinOS to control library loading

diablodale commented 2 years ago

pushed changed w/ feedback. Caution there was a change to code within #ifdef __DEVICE__ so I have no way to test it.

diablodale commented 2 years ago

...and putting all that code in usbInitialize() with a #ifdef around it and needed includes feels a bit weird. I would be ok moving it to a separate c/h and calling it. Its more of a maintenance thing than code accuracy. Your choice. :-)

themarpe commented 2 years ago

I would be ok moving it to a separate c/h and calling it. Its more of a maintenance thing than code accuracy.

Please do - its quite specific code, probably best to be kept more on the side.

WRT: https://github.com/luxonis/depthai-core/pull/473#issuecomment-1122209633

Can we do the same without /DELAYLOAD (or have that as PRIVATE)? I see this causing some additional pain for other integration options (https://github.com/luxonis/depthai-core/tree/xlink_device_search_improvements)

If not, what are the issues with leaving /DELAYLOAD as public and XLink as public eg in core for further downstream apps?

EDIT - If I'm understanding /DELAYLOAD correctly, it only functions as means of not loading the DLL immediately, but same behavior could be achieved by manually using LoadLibrary & GetProcAddress:

The MSVC linker supports the delayed loading of DLLs. This feature relieves you of the need to use the Windows SDK functions LoadLibrary and GetProcAddress to implement DLL delayed loading.

But we do this already, so not sure the play between /DELAYLOAD /w combination with LoadLibrary Reread the code, I see we rely on it to not have to use LoadLibrary explicitly, but we only set correct paths for the DLL to be found when we eventually do call a function from the DLL eg: libusb_init

diablodale commented 2 years ago

When "/DELAYLOAD:libusb-1.0$<$<CONFIG:Debug>:d>.dll" is given to the linker, the linker silently load/scans/processes that exact DLL for a list of functions within that DLL and how that DLL is structured. The linker then creates a new block of binary code, "a delayload stub", and links that stub into the thing being linked. This is all transparently and silently done.

For example, if depthai-core is configured to be a shared library, and we are cmake building, and depthai-core.dll is in the process of being linked, then the linker opens/scans libusb-1.0.dll and creates a delayload stub for it and links that stub into depthai-core.dll.

That means, if PUBLIC is used for xlink, then the link library and link options are propagated downstream. This is undesired and will throw warnings or fail. Why? Because the downstream main app needs depthai-core. It does not need XLink (except for that one header).

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 "/DELAYLOAD:libusb-1.0$<$<CONFIG:Debug>:d>.dll" on my app itself and fails because it can not find libusb.dll at the moment of linking. Earlier, when depthai-core itself was cmake built, the linker throws warnings of the use of DELAYLOAD on a library which has no needed imports.

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 XLink

Below are two pieces of build.ninja from the depthai-core project. The specific piece is the linking of the test filesystem_text.exe.

This is depthai-core xlink_device_search_improvements, therefore it is PUBLIC Xlink linking It is configured for shared depthai-core, build all test and examples, and local static xlink fix37-libusb-win-loadpath.

#############################################
# Link the executable tests\filesystem_test.exe

build tests\filesystem_test.exe tests\filesystem_test.lib: CXX_EXECUTABLE_LINKER__filesystem_test_Debug tests\CMakeFiles\filesystem_test.dir\src\filesystem_test.cpp.obj | depthai-cored.lib XLink\XLinkd.lib C$:\.hunter\_Base\cb0ea1f\c4a44b7\01437af\Install\lib\usb-1.0d.lib || XLink\XLinkd.lib depthai-cored.dll
  FLAGS = -DWINVER=0x0A00 -D_WIN32_WINNT=0x0A00 -DNTDDI_VERSION=0x0A000005 /arch:AVX /Gw /QIntel-jcc-erratum /guard:cf /GS  -D_CRT_SECURE_NO_WARNINGS /wd5030 /w15240 -D_CRT_SECURE_NO_WARNINGS /wd5030 /w15240   /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1
  LINK_FLAGS = /machine:x64 /ignore:4099 /debug /INCREMENTAL /subsystem:console    /DELAYLOAD:libusb-1.0d.dll /DEF:tests\CMakeFiles\filesystem_test.dir\.\exports.def
  LINK_LIBRARIES = depthai-cored.lib  XLink\XLinkd.lib  C:\.hunter\_Base\cb0ea1f\c4a44b7\01437af\Install\lib\usb-1.0d.lib  delayimp.lib  Pathcch.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib
  LINK_PATH = -LIBPATH:C:\PROGRA~1\NVIDIA~2\CUDA\v10.1\lib\x64
  OBJECT_DIR = tests\CMakeFiles\filesystem_test.dir
  POST_BUILD = cmd.exe /C "cd /D C:\njs\depthai-core\build\tests && C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe -noprofile -executionpolicy Bypass -file C:/njs/vcpkg/scripts/buildsystems/msbuild/applocal.ps1 -targetBinary C:/njs/depthai-core/build/tests/filesystem_test.exe -installedDir C:/njs/depthai-core/build/vcpkg_installed/x64-windows-static-md-v142-sdk10/debug/bin -OutVariable out"
  PRE_LINK = cmd.exe /C ""C:\Program Files\CMake\bin\cmake.exe" -E __create_def C:\njs\depthai-core\build\tests\CMakeFiles\filesystem_test.dir\.\exports.def C:\njs\depthai-core\build\tests\CMakeFiles\filesystem_test.dir\.\exports.def.objs && cd C:\njs\depthai-core\build"
  RESTAT = 1
  TARGET_COMPILE_PDB = tests\CMakeFiles\filesystem_test.dir\
  TARGET_FILE = tests\filesystem_test.exe
  TARGET_IMPLIB = tests\filesystem_test.lib
  TARGET_PDB = tests\filesystem_test.pdb

Notice above the link libraries It is linking in XLink\XLinkd.lib and hunter\...\usb-1.0d.lib which are both errant. Notice above the link flags. It has DELAYLOAD. This is errant. Xlink and libusb were linked into depthai-core.dll. Therefore, they should not be double-linked into filesytem_test.exe. And it should not have DELAYLOAD.

Compare that same configuration with my depthai-core PR make-xlink-linking-private Therefore it is: XLink PRIVATE linking and the include directory change. Notice the link libraries and flags. It correctly does not link XLink.lib and libusb.lib, and correctly does not have DELAYLOAD.

#############################################
# Link the executable tests\filesystem_test.exe

build tests\filesystem_test.exe tests\filesystem_test.lib: CXX_EXECUTABLE_LINKER__filesystem_test_Debug tests\CMakeFiles\filesystem_test.dir\src\filesystem_test.cpp.obj | depthai-cored.lib || depthai-cored.dll
  FLAGS = -DWINVER=0x0A00 -D_WIN32_WINNT=0x0A00 -DNTDDI_VERSION=0x0A000005 /arch:AVX /Gw /QIntel-jcc-erratum /guard:cf /GS  -D_CRT_SECURE_NO_WARNINGS /wd5030 /w15240 -D_CRT_SECURE_NO_WARNINGS /wd5030 /w15240   /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1
  LINK_FLAGS = /machine:x64 /ignore:4099 /debug /INCREMENTAL /subsystem:console    /DEF:tests\CMakeFiles\filesystem_test.dir\.\exports.def
  LINK_LIBRARIES = depthai-cored.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib
  LINK_PATH = -LIBPATH:C:\PROGRA~1\NVIDIA~2\CUDA\v10.1\lib\x64
  OBJECT_DIR = tests\CMakeFiles\filesystem_test.dir
  POST_BUILD = cmd.exe /C "cd /D C:\njs\depthai-core\build\tests && C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe -noprofile -executionpolicy Bypass -file C:/njs/vcpkg/scripts/buildsystems/msbuild/applocal.ps1 -targetBinary C:/njs/depthai-core/build/tests/filesystem_test.exe -installedDir C:/njs/depthai-core/build/vcpkg_installed/x64-windows-static-md-v142-sdk10/debug/bin -OutVariable out"
  PRE_LINK = cmd.exe /C ""C:\Program Files\CMake\bin\cmake.exe" -E __create_def C:\njs\depthai-core\build\tests\CMakeFiles\filesystem_test.dir\.\exports.def C:\njs\depthai-core\build\tests\CMakeFiles\filesystem_test.dir\.\exports.def.objs && cd C:\njs\depthai-core\build"
  RESTAT = 1
  TARGET_COMPILE_PDB = tests\CMakeFiles\filesystem_test.dir\
  TARGET_FILE = tests\filesystem_test.exe
  TARGET_IMPLIB = tests\filesystem_test.lib
  TARGET_PDB = tests\filesystem_test.pdb

The include directory for that one xlink header, linker option delayload, and the link of Xlink.lib itself...we need a coordinated improvement. What is needed is to have XLink privately link into the thing that is depthai-core binary code...

...and to handle that special case of the one header file

themarpe commented 2 years ago

I think the only special case is the /DELAYLOAD... It has to be set only at the final place needed (eg app or dll that links in XLink). If XLink was DLL it should stop there, if core was dll, should stop there (be applied there), etc... I'll check if there exist any property like that for CMake. (I guess it could, as there is LINK_ONLY,... actually thinking about it now, I guess this is actually the property which we are after....)

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: XLinkInitialize from their own app. Why would they want that aside, but they may. If we want to take this option away, then we should link to XLink as INTERFACE in core. That should be the mix of "PRIVATE" but expose headers and defines (but don't link, etc...) - (have to check if this behavior does indeed match the PUBLIC in this case, or if properties of XLink would have to be set in such way beforehand...) EDIT: if depthai-core would be static, the proposed wouldn't work - as in that case we would HAVE to link to XLink as well in the downstream target

depthai-core=shared, then the linker should link XLink only into depthai-core.dll depthai-core=static, then the linker should link XLink only into the downstream binary

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. If its static, then it has to "link" in both core case & downstream (symbols have to be available I guess).


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

[1/4] /usr/bin/clang++-11 -DBACKWARD_HAS_BACKTRACE=0 -DBACKWARD_HAS_BACKTRACE_SYMBOL=0 -DBACKWARD_HAS_BFD=0 -DBACKWARD_HAS_DW=0 -DBACKWARD_HAS_DWARF=0 -DBACKWARD_HAS_LIBUNWIND=0 -DBACKWARD_HAS_UNWIND=1 -DDEPTHAI_BOOTLOADER_VERSION=\"0.0.17\" -DDEPTHAI_DEVICE_VERSION=\"7f7e0e7aa2d705d8e618605800fd3c2447a866dd\" -DDEPTHAI_ENABLE_BACKWARD -DDEPTHAI_PATCH_ONLY_MODE -DDEPTHAI_RESOURCE_COMPILED_BINARIES -DDEPTHAI_TARGET_CORE -DJSON_USE_IMPLICIT_CONVERSIONS=1 -DLIBARCHIVE_STATIC -DSPDLOG_COMPILED_LIB -D__PC__ -Ddepthai_core_EXPORTS -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/include -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-shared/include -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-bootloader-shared/include -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/include/depthai -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/src -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-shared/src -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-bootloader-shared/src -isystem /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-shared/3rdparty -isystem /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/_cmrc/include -isystem /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/.hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/include -isystem /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/.hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/backward -g -fPIC -Wall -Wextra -Woverloaded-virtual -Wformat=2 -Wmisleading-indentation -Wnull-dereference -Wdouble-promotion -Wsign-compare -Wtype-limits -Werror=self-assign-field -Werror=unused-lambda-capture -Werror=return-type -Werror=non-virtual-dtor -Werror=sign-compare -Werror=reorder -Werror=switch-enum -std=c++14 -MD -MT CMakeFiles/depthai-core.dir/src/utility/Initialization.cpp.o -MF CMakeFiles/depthai-core.dir/src/utility/Initialization.cpp.o.d -o CMakeFiles/depthai-core.dir/src/utility/Initialization.cpp.o -c /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/src/utility/Initialization.cpp
[2/4] : && /usr/bin/clang++-11 -fPIC -g   -shared -Wl,-soname,libdepthai-core.so -o libdepthai-core.so CMakeFiles/depthai-core.dir/shared/depthai-shared/src/datatype/DatatypeEnum.cpp.o CMakeFiles/depthai-core.dir/shared/depthai-shared/src/utility/Checksum.cpp.o CMakeFiles/depthai-core.dir/shared/depthai-bootloader-shared/src/SBR.c.o CMakeFiles/depthai-core.dir/shared/depthai-bootloader-shared/src/Bootloader.cpp.o CMakeFiles/depthai-core.dir/src/device/Device.cpp.o CMakeFiles/depthai-core.dir/src/device/DeviceBase.cpp.o CMakeFiles/depthai-core.dir/src/device/DeviceBootloader.cpp.o CMakeFiles/depthai-core.dir/src/device/DataQueue.cpp.o CMakeFiles/depthai-core.dir/src/device/CallbackHandler.cpp.o CMakeFiles/depthai-core.dir/src/device/CalibrationHandler.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/Pipeline.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/AssetManager.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/Node.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/XLinkIn.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/XLinkOut.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/ColorCamera.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/MonoCamera.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/StereoDepth.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/NeuralNetwork.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/ImageManip.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/VideoEncoder.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/DetectionNetwork.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/Script.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/SpatialDetectionNetwork.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/SystemLogger.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/SpatialLocationCalculator.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/AprilTag.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/ObjectTracker.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/IMU.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/EdgeDetector.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/SPIIn.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/node/FeatureTracker.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/Buffer.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/ImgFrame.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/ImageManipConfig.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/CameraControl.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/NNData.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/ImgDetections.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/SpatialImgDetections.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/SystemInformation.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/StreamMessageParser.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/SpatialLocationCalculatorData.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/SpatialLocationCalculatorConfig.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/AprilTags.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/AprilTagConfig.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/Tracklets.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/IMUData.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/StereoDepthConfig.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/EdgeDetectorConfig.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/TrackedFeatures.cpp.o CMakeFiles/depthai-core.dir/src/pipeline/datatype/FeatureTrackerConfig.cpp.o CMakeFiles/depthai-core.dir/src/utility/Initialization.cpp.o CMakeFiles/depthai-core.dir/src/utility/Resources.cpp.o CMakeFiles/depthai-core.dir/src/utility/Path.cpp.o CMakeFiles/depthai-core.dir/src/utility/Platform.cpp.o CMakeFiles/depthai-core.dir/src/utility/Environment.cpp.o CMakeFiles/depthai-core.dir/src/xlink/XLinkConnection.cpp.o CMakeFiles/depthai-core.dir/src/xlink/XLinkStream.cpp.o CMakeFiles/depthai-core.dir/src/openvino/OpenVINO.cpp.o CMakeFiles/depthai-core.dir/src/openvino/BlobReader.cpp.o CMakeFiles/depthai-core.dir/src/bspatch/bspatch.c.o  -Wl,-rpath,/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/.hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/cmake/XLink/dependencies/lib:  libdepthai-resources.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libXLink.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libbz2.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libarchive_static.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libspdlog.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libz.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/cmake/XLink/dependencies/lib/libusb-1.0.so  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/liblzma.a  -lpthread && :
[3/4] : && /usr/bin/clang++-11 -fPIC -g   -shared -Wl,-soname,libdepthai-opencv.so -o libdepthai-opencv.so CMakeFiles/depthai-opencv.dir/src/opencv/ImgFrame.cpp.o  -Wl,-rpath,/opt/intel/openvino_2021.4.689/opencv/lib:/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build:/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/.hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/cmake/XLink/dependencies/lib:  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_imgproc.so.4.5.3  libdepthai-core.so  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_core.so.4.5.3  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libXLink.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/cmake/XLink/dependencies/lib/libusb-1.0.so  -lpthread && :
[4/4] : && /usr/bin/clang++-11 -g -Wl,--export-dynamic  -rdynamic examples/CMakeFiles/rgb_preview.dir/ColorCamera/rgb_preview.cpp.o -o examples/rgb_preview  -Wl,-rpath,/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/examples:/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build:/opt/intel/openvino_2021.4.689/opencv/lib:/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/.hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/cmake/XLink/dependencies/lib  examples/libutility.so  libdepthai-opencv.so  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_gapi.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_highgui.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_ml.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_objdetect.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_photo.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_stitching.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_video.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_videoio.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_calib3d.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_dnn.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_features2d.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_flann.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_imgcodecs.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_imgproc.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_core.so.4.5.3  libdepthai-core.so  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/libXLink.a  .hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/lib/cmake/XLink/dependencies/lib/libusb-1.0.so  -lpthread && :

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, ...)

/usr/bin/clang++-11 -fPIC -g   -shared -Wl,-soname,libdepthai-opencv.so -o libdepthai-opencv.so CMakeFiles/depthai-opencv.dir/src/opencv/ImgFrame.cpp.o  -Wl,-rpath,/opt/intel/openvino_2021.4.689/opencv/lib:/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build:  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_imgproc.so.4.5.3  libdepthai-core.so  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_core.so.4.5.3 && :
[74/75] /usr/bin/clang++-11 -DDEPTHAI_TARGET_CORE -DDEPTHAI_TARGET_OPENCV -DJSON_USE_IMPLICIT_CONVERSIONS=1 -Drgb_preview_EXPORTS -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/examples/utility -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/include -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-shared/include -I/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-bootloader-shared/include -isystem /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/.hntr/_Base/cb0ea1f/84bd56f/c4ef5cf/Install/include -isystem /opt/intel/openvino_2021.4.689/opencv/include -isystem /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/shared/depthai-shared/3rdparty -g -fPIE -MD -MT examples/CMakeFiles/rgb_preview.dir/ColorCamera/rgb_preview.cpp.o -MF examples/CMakeFiles/rgb_preview.dir/ColorCamera/rgb_preview.cpp.o.d -o examples/CMakeFiles/rgb_preview.dir/ColorCamera/rgb_preview.cpp.o -c /home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/examples/ColorCamera/rgb_preview.cpp
[75/75] : && /usr/bin/clang++-11 -g -Wl,--export-dynamic  -rdynamic examples/CMakeFiles/rgb_preview.dir/ColorCamera/rgb_preview.cpp.o -o examples/rgb_preview  -Wl,-rpath,/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build/examples:/home/themarpe/Documents/luxonis/depthai/gen2/depthai-python/depthai-core/build:/opt/intel/openvino_2021.4.689/opencv/lib  examples/libutility.so  libdepthai-opencv.so  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_gapi.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_highgui.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_ml.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_objdetect.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_photo.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_stitching.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_video.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_videoio.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_calib3d.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_dnn.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_features2d.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_flann.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_imgcodecs.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_imgproc.so.4.5.3  /opt/intel/openvino_2021.4.689/opencv/lib/libopencv_core.so.4.5.3  libdepthai-core.so && 
diablodale commented 2 years ago

Either case, there should be at least 1 entry for XLink - because one "COULD" call eg: XLinkInitialize from their own app.

The single header XLink\XLinkPublicDefines.h does not include any declarations of functions; no XLinkInitialize(); This makes it far outside reasonable use for a dev using depthai-core app to be able to call XLinkInitialize() or any Xlink or libusb API. The app's developer would need to rwrite their own extern declaration, perhaps GetProcAddr for Windows, a lot of work. Very untested and unpredictable how depthai-core might respond of the main app starts manipulating XLink or libusb.

The depthai-core project provides apis to use Luxonis sensors. It is not a general library that exposes XLink and libusb apis. I do not believe there should be any accommodation for an app developer using depthai-core to call directly into xlink or libusb.

My "vote" is that such is not allowed, and therefore linking should be PRIVATE. I am also concerned with having two XLinks in memory. Yes... 2.

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. If you PUBLIC link XLink, and a dev of a main app writes their own XLinkInitialize extern declaration and then calls that API...the linker will link a copy of the executable code for XLink from XLib.lib into the main app. This is a second exact copy of XLink binary code. When the main app runs, it loads itself which includes its copy of XLink and therefore allocates all its globals, TLS, etc. Then...depthai-core.dll is loaded as a DLL into the main app. The DLL, as above, has its own copy of the XLink binary code and it allocates its own globals, TLS, mutex, etc within the memory context of the DLL...this is separate from the main app. Now two XLinks are running at the same time and they are not sharing globals, TLS, etc. When the main app calls XLinkInitialize() it will do all that work (init mutex, onetime=1, etc.) with its copy. And when the main app calls an depthai-core API, then depthai-core calls XLinkInitialize() and it will initialize its separate copy of XLink. This is ripe for many many problems. 💥🙈

There's more to discuss, but I think this needs to be resolved first. This is a Luxonis choice to make. We can go PUBLIC but you create the failure scenario above.

themarpe commented 2 years ago

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. If you PUBLIC link XLink, and a dev of a main app writes their own XLinkInitialize extern declaration and then calls that API...the linker will link a copy of the executable code for XLink from XLib.lib into the main app. This is a second exact copy of XLink binary code. When the main app runs, it loads itself which includes its copy of XLink and therefore allocates all its globals, TLS, etc. Then...depthai-core.dll is loaded as a DLL into the main app. The DLL, as above, has its own copy of the XLink binary code and it allocates its own globals, TLS, mutex, etc within the memory context of the DLL...this is separate from the main app. Now two XLinks are running at the same time and they are not sharing globals, TLS, etc. When the main app calls XLinkInitialize() it will do all that work (init mutex, onetime=1, etc.) with its copy. And when the main app calls an depthai-core API, then depthai-core calls XLinkInitialize() and it will initialize its separate copy of XLink. This is ripe for many many problems.

Hmm, then its INTERFACE in case of dll and PUBLIC in case of static depthai-core? We still need to expose include headers as well as any potential defines that would be set.

Also, this could be done for all dependencies in that case.

Not sure what will happen to transitive dependency libusb in this case though...

diablodale commented 2 years ago

hi, I will be a delayed a few days. Will get back to this no later than Monday 16th, probably sooner.

diablodale commented 2 years ago

I read some related info on this, and I keep coming back to that single public xlink header is the special case. Linking Xlink PRIVATE to depthai-core works for everything else (so far for all combinations of static/shared tests).

[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 FRAMEWORK and PUBLIC_HEADER feature pair, and the idea of an IMPORTED INTERFACE library.

What if we explored....

Or we could move that one include file to a different directory. And go from there.

themarpe commented 2 years ago

What if we explored.... keeping depthai-core cmake as linking Xlink PRIVATE. This passes all static and shared combinations of tests...except for that one header. And to me, it logically makes sense...the only thing that needs XLink code is the "private" part of depthai-core. create a IMPORTED INTERFACE library (or something similar) for that one header file. And then link that into depthai-core as INTERFACE. It could be PUBLIC if no compiler/tool sees the two copies of that header and complains -- but I don't see any value in that since INTERFACE avoids that issue.

This ^ - it seems that linking only to "headers/defines" isn't possible. I (wrongfully) thought that this is what INTERFACE does.

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

themarpe commented 2 years ago

Pushed to both XLink and core. @diablodale feel free to checkout

diablodale commented 2 years ago

I think we are closer :-) I had a cmake config fail when your changes to xlink+depthai-core were used on my app. https://github.com/luxonis/depthai-core/pull/410#issuecomment-1126937995

themarpe commented 2 years ago

With core CMake issues now resolved, do device search improvements + these changes pass as expected? Let me know once you are up for merging

diablodale commented 2 years ago

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) .. change with XLINK_LOCAL

diablodale commented 2 years ago

Force pushed... Commit 1 is a squash of the previous 4 commits Commit 2 is moving the Windows-specific init code into a single file.

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 dlltool to create a stub lib for the delayload. I suggest not doing that work until someone escalates they need need it.

diablodale commented 2 years ago

Our approach went a different direction with xlink PRIVATE and xlinkpublic INTERFACE in your two PRs. The command target_link_options(XLink PUBLIC "/DELAYLOAD) is restricted to only add that option at the link step of Xlink. This is desired. Why is there a need to put <LINK_ONLY> on a command that is already link-only? In addition, the cmake docs write <LINK_ONLY> is "intended for use only in an INTERFACE_LINK_LIBRARIES target property".

I haven't...yet...seen DELAYLOAD on any compile. And I only see DELAYLOAD on the single correct link. This is from my testing of this PR + your pending PRs.

Just now, I also search through the ninja.build for depthai-core and my app. My app has no DELAYLOAD. This is correct. depthai-core has one DELAYLOAD...on the link for depthai-core.dll. This is correct.

Do you have a scenario/repro that causes DELAYLOAD to appear in the wrong location?

Haha, I retested what I already tested. target_link_options(XLink PRIVATE "/DELAYLOAD) will not work. It does not propagate that option downstream. This PR + your two PRs together will propagate the DELAYLOAD option downstream until the point that XLink (or its downstream project) is PRIVATE linked and then it will stop there. That's what I've seen so far.

I'll squash the two commits, remove a BUGBUG, and force push.

diablodale commented 2 years ago

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 target_link_options(XLink PUBLIC "/DELAYLOAD instead of PRIVATE.

diablodale commented 2 years ago

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 :-)

themarpe commented 2 years ago

Thanks! Yeah, I guess the point of having it is not from standpoint of depthai-core, but if someone consumes XLink as PUBLIC in their own project.

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!