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

libusb-cmake as libusb provider and added MSVC Support #1440

Open
wants to merge 8 commits into
base: testing
Choose a base branch
from

Conversation

a-michelis
Copy link

@a-michelis a-michelis commented Nov 21, 2024

Overview

This pull request alters Findlibusb.cmake in order to add proper Windows MSVC handling. It discards the old MINGW approach and uses the libusb-cmake repository as the main provider instead for both Windows and Linux. As it stands now, if libusb is not installed for these two OSes, the libusb-cmake repository is being cloned and incorporated into the build. This way,stlink has a freshly built libusb, tailored to the system's configuration and needs, installed into the same INSTALL_PREFIX as stlink.

Additionally, this pull request adds Windows/MSVC support into the c/c++ github workflow.

Potential issues

  1. In Windows, there is a possibility that the installation path for all LIBRARY items must be the same as the one of RUNTIME items, as opposed to the Linux paradime, where LIBRARY and ARCHIVE items are the ones that share location. This needs further investigation and potentialy a complementary fix, as I currently have no equipment and a clean environment to test it. I'll get back to it once I manage to prepare the proper testing ground.

  2. One very important note on this PR is that it probably drops support for MINGW. If mingw is still needed I'm going to need assistance, Since I've never worked with it in the past and i do not know how to properly validate such builds.

- Changed Legacy MINGW Checkers for MSVC
- If not available, now `Findlibusb.cmake` fetches `libusb-cmake` repo and includes it at config time
- For MSVC, Added fix for ill-defined `ssize-t`
- For linux build-along, added check whether LIBUDEV is installed/enabled
if (NOT LIBUSB_FOUND)
message(FATAL_ERROR "libusb library not found on your system! Install libusb 1.0.x from your package repository.")
message(STATUS "No libusb-1.0 not installed into your system. Downloading and building it from source")
Copy link
Member

Choose a reason for hiding this comment

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

This is not a good practise from my point of view.
All unix-based systems should continue to install and use libusb provided from their respective package repositories. It must be considered a very bad attempt to mix locally installed newer library versions with probably already existing dependencies used by other (system) packages originating from the package system, as this opens the door for incompatibilities and functional failures.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem, Will revert to the old practice!

As for windows, Will do some checks for dll locations compatibility later today or tmr, so we can be sure for this as well

Copy link
Member

@Nightwalker-87 Nightwalker-87 left a comment

Choose a reason for hiding this comment

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

Please solely introduce changes affecting the windows operating system due to the package-based nature of unix-based systems to ensure the verification of system integrity.

@a-michelis
Copy link
Author

a-michelis commented Nov 25, 2024

Tested the MSVC Build and it needs some additional root cmake modifications to work, but totally easy to do. Will come back with them sometime later today.

As for the library part, This is something that cannot be brought to this PR, since it requres restructuring in both code and CMAKE:

  • Since windows have their own usb.h, it conflicts with stlink's header and, thus, it's required to put the headers within a prefixed subfolder and use them as such (ie #include <stlink/usb.h>). This requires code modifications, as all headers that have #include <someStlinkHeader.h> must be converted to #include <stlink/someStlinkHeader.h>.

  • Windows require __declspec(dllexport/dllimport) to each desired function, in order to properly export functionality. This is solvable by creating an stlink/api.h header with the well-known macro definition (let's call it STLINK_API), that prepends dllexport when building and dllimport when using (via compiler definition). That's easy to address- essentially a templated copy-paste-edit snippet.

# Link shared library
if (WIN32)
target_link_libraries(${STLINK_LIB_SHARED} ${LIBUSB_LIBRARY} ${SSP_LIB} wsock32 ws2_32)
else ()
target_link_libraries(${STLINK_LIB_SHARED} ${LIBUSB_LIBRARY} ${SSP_LIB})
endif()

install(TARGETS ${STLINK_LIB_SHARED} ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})

# For windows, shared libraries go to `bin` and their implibs go to `lib`
Copy link
Member

Choose a reason for hiding this comment

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

Documentaton: Replace "implibs" with "imported libraries (implibs)" in comment for clarification.

@@ -51,75 +52,42 @@ elseif (CMAKE_SYSTEM_NAME STREQUAL "OpenBSD") # OpenBSD; libus
message(FATAL_ERROR "No libusb-1.0 library found on your system! Install libusb-1.0 from ports or packages.")
endif()

elseif (WIN32 OR (MINGW AND EXISTS "/etc/debian_version")) # Windows OR cross-build with MinGW-toolchain on Debian
Copy link
Member

@Nightwalker-87 Nightwalker-87 Nov 30, 2024

Choose a reason for hiding this comment

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

Would it be possible to leave
elseif (MINGW AND EXISTS "/etc/debian_version") # Cross-build with MinGW-toolchain on Debian

including the following associated code block for now, until cross building via MVSC on Linux has also been tested and verified? The idea is to drop this part as well later on, if everything works well. I'd just prefer to keep at least one working approach for now ...

Copy link
Member

@Nightwalker-87 Nightwalker-87 left a comment

Choose a reason for hiding this comment

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

I consider this a good and very useful contribution, with only a few remaining FMPOV accountable remarks that should be easily addressable. Thank you very much. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants