Closed ankurvdev closed 1 year ago
And thanks for doing this in a first place. Since I've volountired to support this, I might as well apply some of the fixes/suggestions I've suggested myself. At least at some point.
Do you think the project name should be libusb-1.0
instead of usb-1.0
?
I don't think so. At least for one reason - there will be lots of CMake "workarounds" to make the build binary-compatible with current Autotools (and yes, I think we should make it as compatible as possible).
So if the resulting binary is libusb-1.0.so
- the library/project name should be usb-1.0
. That is a logical assumption in CMake ecosystem.
There's quite a bit of comments in here :) and i'll have limited bandwidth until next week. I do agree with most of them and it's probably good to accommodate all of them before a merge.
I'd really appreciate any contributions to the branch if anyone else has bandwidth and would like to accelerate the merge (Seems like there's a few followup that might be queued behind it)
a general comment:
- I suggest we follow some general CMake guidelines/code-style, e.g.:
UPPER_CAMEL_CASE
for all variables, including user-defined;
At the end, I'd like us to some practices we've umplemented for HIDAPI (documented here), e.g.
add_subdirectory(libusb)
should be easy to do for any project (be configurable)
It's worth adding a cmake style guide here since I have my own preferences but I'll gladly follow what you'd like them to be
I tend to follow lower_case for local scoped variables and function names and UPPER_CASE for global scope variables / cached variables (I probably have violations for my own policy in this pr) but a few lines to standardize styling might help everyone.
We can probably add a comment at the top here for styling if you don't want an extra file
I probably have violations for my own policy
That's probably what triggered my attention the most.
We can probably add a comment at the top here for styling if you don't want an extra file
Ideally would be to introduce something like cmake-format. But for now, jsut keeping the implementation clean/consistent - worth more as far as efforts spending is conserned.
The most important idea is to make the usage of CMakeLists.txt as much flexible/comfortable as possible for all of: this repo developers, package managers, submodule users, etc. That's why I referenced the HIDAPI document where many such ideas already implemented and I'd like to use those here too.
As much as I can tell based on comments in this thread - we're generally on the same page.
@ankurvdev
I'd really appreciate any contributions to the branch if anyone else has bandwidth and would like to accelerate the merge (Seems like there's a few followup that might be queued behind it)
It's complicated to contribute on a PR because I can't push code on it. Should I create a PR on our repo and you merge it ?
I'm pretty sure you can push directly into ankurvdev/libusb-cmake:master If not - you may ask @ankurvdev to add you directly as a contributor.
Preferably to keep the entire conversation in this thread. Unless @ankurvdev is using his master branch elsewhere, in thich case messing around with it might be inconvenient for him/her.
Let me know if there are any permission issues. The branch should be open for any contributions.
Hi I might have gotten a little carried away. A Thursday night little project went south because libusbs lack of cmake support and I pulled this repo to find the "finish me" message ;) Anyway while doing other things I've updated the CMakefile per the comments. There were also a few things wrong or missing so I've sorted them out. @ankurvdev you should have a pull request on your repo if you want to take it forward.
Thanks a lot @madwax. This looks a lot clean I think you've addressed most of the comments already but I added a few touch ups.
@Youw I'm wondering what your thoughts are on using subtree (https://www.atlassian.com/git/tutorials/git-subtree) for managing the repository rather than a submodule.
I don't have too much experience with subtree but I used it in a project and liked it. It does require a little discipline to keep things simple (for eg: just based on my limited experience)
But in the end the experience for users and downstream projects would be quite neat.
we might even petition vcpkg (and rtlsdr) to switch to this repo for expanding support for libusb to more platforms
Thanks a lot @madwax. This looks a lot clean I think you've addressed most of the comments already but I added a few touch ups.
Nice one. The PR is now working pretty well.
One minor issue that the static lib name is not so correct for MSVC build. It should be libusb-1.0.lib
and not usb-1.0.lib
. MinGW build is correct -- the name of the static lib is libusb-1.0.a
.
The other issue is that the test program is named stress
and not really testlib
.
The following run log is under Developer PowerShell for VS 2022
.
PS C:\work\libusb\libusb_cmake_pr1> cmake -B build_vs2022
-- Building for: Ninja
-- The C compiler identification is MSVC 19.35.32216.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.35.32215/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for clock_gettime
-- Looking for clock_gettime - not found
-- Looking for pthread_condattr_setclock
-- Looking for pthread_condattr_setclock - not found
-- Looking for pthread_setname_np
-- Looking for pthread_setname_np - not found
-- Looking for pthread_threadid_np
-- Looking for pthread_threadid_np - not found
-- Looking for eventfd
-- Looking for eventfd - not found
-- Looking for pipe2
-- Looking for pipe2 - not found
-- Looking for syslog
-- Looking for syslog - not found
-- Looking for include file asm/types.h
-- Looking for include file asm/types.h - not found
-- Looking for include file string.h
-- Looking for include file string.h - found
-- Looking for include file sys/time.h
-- Looking for include file sys/time.h - not found
-- Looking for timerfd_create
-- Looking for timerfd_create - not found
-- Looking for nfds_t
-- Looking for nfds_t - not found
-- Performing Test HAVE_STRUCT_TIMESPEC
-- Performing Test HAVE_STRUCT_TIMESPEC - Success
-- Configuring done
-- Generating done
-- Build files have been written to: C:/work/libusb/libusb_cmake_pr1/build_vs2022
PS C:\work\libusb\libusb_cmake_pr1> cmake --build build_vs2022
[34/34] Linking C executable xusb.exe
PS C:\work\libusb\libusb_cmake_pr1> ls .\build_vs2022\
Directory: C:\work\libusb\libusb_cmake_pr1\build_vs2022
Mode LastWriteTime Length Name
---- ------------- ------ ----
d----- 9/4/2023 6:30 pm CMakeFiles
d----- 9/4/2023 6:30 pm gen_include
-a---- 9/4/2023 6:30 pm 4312 .ninja_deps
-a---- 9/4/2023 6:30 pm 2905 .ninja_log
-a---- 9/4/2023 6:30 pm 46140 build.ninja
-a---- 9/4/2023 6:30 pm 14856 CMakeCache.txt
-a---- 9/4/2023 6:30 pm 1930 cmake_install.cmake
-a---- 9/4/2023 6:30 pm 341504 dpfp.exe
-a---- 9/4/2023 6:30 pm 992312 dpfp.ilk
-a---- 9/4/2023 6:30 pm 1708032 dpfp.pdb
-a---- 9/4/2023 6:30 pm 356864 fxload.exe
-a---- 9/4/2023 6:30 pm 1015824 fxload.ilk
-a---- 9/4/2023 6:30 pm 1748992 fxload.pdb
-a---- 9/4/2023 6:30 pm 28952 getoptMSVC.lib
-a---- 9/4/2023 6:30 pm 327168 hotplugtest.exe
-a---- 9/4/2023 6:30 pm 968696 hotplugtest.ilk
-a---- 9/4/2023 6:30 pm 1683456 hotplugtest.pdb
-a---- 9/4/2023 6:30 pm 326656 listdevs.exe
-a---- 9/4/2023 6:30 pm 965016 listdevs.ilk
-a---- 9/4/2023 6:30 pm 1683456 listdevs.pdb
-a---- 9/4/2023 6:30 pm 328704 sam3u_benchmark.exe
-a---- 9/4/2023 6:30 pm 972584 sam3u_benchmark.ilk
-a---- 9/4/2023 6:30 pm 1683456 sam3u_benchmark.pdb
-a---- 9/4/2023 6:30 pm 330752 testlib.exe
-a---- 9/4/2023 6:30 pm 981912 testlib.ilk
-a---- 9/4/2023 6:30 pm 1691648 testlib.pdb
-a---- 9/4/2023 6:30 pm 332288 testlibusb.exe
-a---- 9/4/2023 6:30 pm 966848 testlibusb.ilk
-a---- 9/4/2023 6:30 pm 1683456 testlibusb.pdb
-a---- 9/4/2023 6:30 pm 1052556 usb-1.0.lib
-a---- 9/4/2023 6:30 pm 366592 xusb.exe
-a---- 9/4/2023 6:30 pm 997736 xusb.ilk
-a---- 9/4/2023 6:30 pm 1724416 xusb.pdb
The following is with MSYS2 MinGW64.
PS C:\work\libusb\libusb_cmake_pr1> cmake -B build
-- Building for: Ninja
-- The C compiler identification is GNU 12.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/msys64/mingw64/bin/cc.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for clock_gettime
-- Looking for clock_gettime - found
-- Looking for pthread_condattr_setclock
-- Looking for pthread_condattr_setclock - found
-- Looking for pthread_setname_np
-- Looking for pthread_setname_np - found
-- Looking for pthread_threadid_np
-- Looking for pthread_threadid_np - not found
-- Looking for eventfd
-- Looking for eventfd - not found
-- Looking for pipe2
-- Looking for pipe2 - not found
-- Looking for syslog
-- Looking for syslog - not found
-- Looking for include file asm/types.h
-- Looking for include file asm/types.h - not found
-- Looking for include file string.h
-- Looking for include file string.h - found
-- Looking for include file sys/time.h
-- Looking for include file sys/time.h - found
-- Looking for timerfd_create
-- Looking for timerfd_create - not found
-- Looking for nfds_t
-- Looking for nfds_t - not found
-- Performing Test HAVE_STRUCT_TIMESPEC
-- Performing Test HAVE_STRUCT_TIMESPEC - Success
-- Performing Test HAVE_VISIBILITY
-- Performing Test HAVE_VISIBILITY - Success
-- Configuring done
-- Generating done
-- Build files have been written to: C:/work/libusb/libusb_cmake_pr1/build
PS C:\work\libusb\libusb_cmake_pr1> cmake --build .\build\
[30/30] Linking C executable xusb.exe
PS C:\work\libusb\libusb_cmake_pr1> ls .\build\
Directory: C:\work\libusb\libusb_cmake_pr1\build
Mode LastWriteTime Length Name
---- ------------- ------ ----
d---- 9/4/2023 6:28 pm CMakeFiles
d---- 9/4/2023 6:28 pm gen_include
-a--- 9/4/2023 6:29 pm 42172 .ninja_deps
-a--- 9/4/2023 6:29 pm 2573 .ninja_log
-a--- 9/4/2023 6:28 pm 41183 build.ninja
-a--- 9/4/2023 6:28 pm 2058 cmake_install.cmake
-a--- 9/4/2023 6:28 pm 16060 CMakeCache.txt
-a--- 9/4/2023 6:29 pm 639470 dpfp.exe
-a--- 9/4/2023 6:29 pm 663046 fxload.exe
-a--- 9/4/2023 6:29 pm 628165 hotplugtest.exe
-a--- 9/4/2023 6:29 pm 272070 libusb-1.0.a
-a--- 9/4/2023 6:29 pm 627057 listdevs.exe
-a--- 9/4/2023 6:29 pm 631054 sam3u_benchmark.exe
-a--- 9/4/2023 6:29 pm 631616 testlib.exe
-a--- 9/4/2023 6:29 pm 632512 testlibusb.exe
-a--- 9/4/2023 6:29 pm 656117 xusb.exe
Same for shared build with -D LIBUSB_BUILD_SHARED_LIBS=ON
.
MinGW build is correct to produce libusb-1.0.dll
and libusb-1.0.dll.a
.
MSVC build is incorrect to produce usb-1.0.dll
and usb-1.0.lib
.
Other than the above mentioned issues, the PR already works pretty well. I tested under Windows using the following test enviroment.
1) Developer PowerShell for VS 2022, using Ninja or Visual Studio 17 2022
generator
2) Using VS2022 to open the folder, which can create configurations for different platforms like ARM64, x86, x64, etc
3) Using MinGW64 without MSYS2, using Ninja or MinGW Makefiles
generator
4) Using MinGW64 with MSYS2, using Ninja or MSYS Makefiles
generator
@mcuee About the lib name, I would say it's in fact correct, in over +20 year of coding I've never seen a Windows static lib prefixed with lib unless there is a Linux coder around ;) The lib prefix is very much a POSIX think which is why MinGW64 does it. However naming the bin is easy.
Also good spot on the test being stress by bad.
@ankurvdev Just a quick question, why did you put back the "" around all the paths? Did you run into a problem or was that just a style thing?
Also thanks for the if(LIBUSB_TARGETS_INCLUDE_USING_SYSTEM) It got lost
Quotes around paths in general are needed to handle paths with spaces.
Most of source paths here don't need spaces anymore since I yanked out the SOURCE_DIR from LIBSUSB_ROOT which wasn't really needed and libusb paths themselves dont have spaces, but I also now simply consider it a style+good-practice to have all paths and single path variables be enclosed in Quotes for uniformity. That way, variables that can have multiple paths (lists) also stand out.
Good point on the spaces and as its a standards thing then cool. tbh I'm working with about 4 different styles of CMake currently and was a little bit in auto mode at the time. Will keep with the ""'s
The CMAKE_CURRENT_SORUCE_DIR Is something I do because of problems with nesting CMakefiles so its a defensive thing, in this case it might be overkill.
I'm wondering when that's useful.
I've never had to, the only place I remember using it was an add_custom_commands that did some sort of code generation etc and wanted to deal exclusively in absolute paths with Binary dir as my working dir.
I tried an uber project with libusb add_subdirectory, with both , it being an actual subdirectory and with absolute path outside the outer project (with binary_dir) and it worked.
So I figured I'll remove it and let you holler back :)
lol problem. The issue is when you want to do any processing of a targets source files. For example I have functions that prettify a targets folder structure in VS and XCode, when you get the source files and they are relative then SOURCE_DIR is used to build the abs path which is a bugger as that's set to the current CMakefile. I use similar functions to create pseudo-targets for static analysis, styling etc etc.
I'm wondering what your thoughts are on using subtree
Haven't tried it yet, but I believe this is a perfect opportunity to do so for me. Git submodules is the default option under the tips of my fingers, but I'm in constant search of the alternatives to it.
I would say it's in fact correct, in over +20 year of coding I've never seen a Windows static lib prefixed with lib unless there is a Linux coder around
I totally get you you on this one. In fact - I usually recommend using the "default" platform-specific behavior, which is familiar to the "natives" of the platform.
But in this particular case - we're trying to mimc the current Autotools build system of libusb, so that in cases like this there is no or minimum difference in the behavior.
TL;DR: Lets set the PREFIX
property for libusb target to lib
unconditionally for all platforms, same way as it is done in current build system(s) for libusb.
Thanks for all of the updates. I'll have a chance to take a look, hopefully, later this week.
@Youw I'm wondering what your thoughts are on using subtree
See my thoughts at: https://github.com/libusb/libusb-cmake/discussions/3
Ping ... Should we merge this now ?
I've applied all of the fixes from my comments myself, and I think we're in a good place to have it merged.
Same for shared build with
-D LIBUSB_BUILD_SHARED_LIBS=ON
. MinGW build is correct to producelibusb-1.0.dll
andlibusb-1.0.dll.a
. MSVC build is incorrect to produceusb-1.0.dll
andusb-1.0.lib
.
Fixed.
Ported from https://github.com/libusb/libusb/pull/1134