Skip to content

Conversation

@bruce87en
Copy link
Contributor

Fix compile issue when used as submodule:
1.Modify BUILD_SHARED_LIBS to BUILD_LIBUSBP_SHARED_LIBS to avoid potential conflicts.
2.Modify the header file include path relative to the current CMakeLists.txt to avoid compilation errors caused by the top-level project not being libusbp.

@DavidEGrayson
Copy link
Member

DavidEGrayson commented Aug 6, 2024

Change 2 seems OK, but for change 1, the intention is to just use this standard CMake variable which automatically controls what type of library gets built, instead of defining our own:

https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

So I think the BUILD_SHARED_LIBS code should stay is it is, but you could elaborate on what specific problem it's causing for you

@NuLL3rr0r
Copy link
Contributor

NuLL3rr0r commented Mar 4, 2025

Well, I personally am in fabor of @bruce87en approach, for example libusb-cmake have these option names, which is clean:

set(LIBUSB_BUILD_SHARED_LIBS            OFF             CACHE BOOL "Build shared libraries for libusb")
set(LIBUSB_BUILD_TESTING                ${BUILD_TESTS}  CACHE BOOL "Build testing for libusb")
set(LIBUSB_BUILD_EXAMPLES               OFF             CACHE BOOL "Build examples for libusb")
set(LIBUSB_INSTALL_TARGETS              OFF             CACHE BOOL "Disable installation targets for libusb")
set(LIBUSB_TARGETS_INCLUDE_USING_SYSTEM OFF             CACHE BOOL "Use system includes for libusb")
set(LIBUSB_ENABLE_LOGGING               ON              CACHE BOOL "Enable logging in libusb")
set(LIBUSB_ENABLE_DEBUG_LOGGING         OFF             CACHE BOOL "Enable debug logging in libusb")
set(LIBUSB_ENABLE_UDEV                  ON              CACHE BOOL "Enable udev support in libusb")

add_subdirectory(third-party/libusb-cmake)

Where as:

set(BUILD_SHARED_LIBS       OFF     CACHE   BOOL        "Build as shared library")
set(ENABLE_EXAMPLES         FALSE   CACHE   INTERNAL    "True if you want to build the examples.")
set(ENABLE_TESTS            ${BUILD_TESTS}    CACHE   INTERNAL    "True if you want to build the tests.")
set(LIBUSBP_LOG             FALSE   CACHE   INTERNAL    "Output log messages to stderr for debugging.")
set(VBOX_LINUX_ON_WINDOWS   FALSE   CACHE   INTERNAL    "Skip tests known to cause problems on a Linux VirtualBox guest on Windows.")
set(ENABLE_GCOV             FALSE   CACHE   INTERNAL    "Compile with special options needed for gcov.")

add_subdirectory(third-party/libusbp)

unset(BUILD_SHARED_LIBS)
unset(ENABLE_EXAMPLES)
unset(ENABLE_TESTS)
#unset(LIBUSBP_LOG)
unset(VBOX_LINUX_ON_WINDOWS)
unset(ENABLE_GCOV)

The reason is we want to build some things statically and some things dynamically. His approach which is used by many cmake-based projects is more flexible, because does not impose an unset.

@DavidEGrayson
Copy link
Member

DavidEGrayson commented Apr 28, 2025

I committed @bruce87en 's improvement 2 and then I made some more improvements to the CMake code; you all are welcome to check it out. There is no longer any explicit reference to BUILD_SHARED_LIBS. I think I could achieve the fine-grained control that you guys want simply by changing the add_library line in src/CMakeLists.txt to this:

add_library (usbp ${LIBUSBP_LIB_TYPE} ${sources})

So if it's not defined, you get the default type of library (determined by BUILD_SHARED_LIBS). If it's defined to "STATIC" or "SHARED" then you get that type of library. I wonder if that's really best way though.

@DavidEGrayson
Copy link
Member

DavidEGrayson commented May 12, 2025

I think that using libusbp's CMake files (instead of just installing it and using the pkg-config files) and also expecting its library to be a different type than the default type you have set using the BUILD_SHARED_LIBS variable is a niche-enough case that we don't need to worry about supporting it, especially since you do have the workaround that @NuLL3rr0r described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants