- Notifications
You must be signed in to change notification settings - Fork2k
Update and rename README to README.md#1591
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Youw commentedDec 21, 2024
@AndrewBloom have you considered usinghttps://github.com/libusb/libusb-cmake instead of sample you gave? |
AndrewBloom commentedDec 21, 2024
@Youw, thanks for your comment. I didn't know the existence of that project. I looked at it, and there is a difference on the approaches to the problem. The project you mentioned, tries toredo the build process using Cmake. This can lead to misalignments betweenlibusb and their cmakelists file. For example, if a file is added tolibusb source, the same has to be added on their cmakelists. The approach I documented, instead, simplydelegates the build to the official build system. It's the equivalent of, in plain english, "please build it and give me the result". The risk of misalignment this way is eliminated, as long as the top part of this readme is valid (instructions on how to build usingndk-build). |
Youw commentedDec 21, 2024
I'm maintaining that project, and making sure that won't happen. CI helps me a lot with that. |
AndrewBloom commentedDec 21, 2024
@Youw, I'm glad to have the chance to speak with someone that maintains that project. I appreciate your effort in giving to the community aCMakeLists to compilelibusb using only thecmake build system. I can indeed imagine some scenarios wherecmake is available on thehost system, but the build system supported bylibusb is not, and suchCMakeLists is necessary. But in the general case, your approach adds complexity, and so potential failures. It's a build system supporting multiple architectures, and you need to duplicate everything on theCMakelists. As a user, I want to be sure that every compilation or optimisation flag or quirkiness is used when I compilelibusb, and that guarantee is given only using the official build system. The simplest and easiest solution, IMO, is todelegate the build process to the official supported build system. Thankfully, usingExternalProject functionality incmake, this is possible. Actually, the same approach could be used for other OSes, and I leave it to you to evaluate the pros and cons in that scenarios. For Android, I can attest first hand that it delivers its benefits in a concise and elegant way. |
Youw commentedDec 22, 2024
@AndrewBloom I get your points. |
Youw left a comment
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.
@tormodvolden,@hjelmn as much as it goes regarding the content of suggested CMake sample - I don't find any issues with it.
seanm commentedDec 22, 2024
github doesn't make it easy to diff this. It just shows one file deleted, and one new file with all new contents. Can you please make a separate PR that only renames the file? That should be easier to get approved and merged. Then you can rebase this one, and reviewers can see what you actually changed. |
Added a very useful section with an example of CMakeLists, so to make easier a compilation with the android default ide and processes.
AndrewBloom commentedDec 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@seanm I admit that renaming and modifying the file in the same commit was terrible and I had too to double check the changes line by line myself. Sorry it's my first pull request to an open source project :) @Youw I'll try thelibusb-cmake project tomorrow and let you know the results. |
added the include directive. added void return type. corrected first parameter libusb_set_option, from &ctx that was wrong type(** vs * accepted by the func) to NULL as perunrooted_android.c example.
AndrewBloom commentedDec 23, 2024
@Youw I switched tolibusb_cmake on my project and it runs. Consider that my project currently is nothing more than a sketch, that takes the fd from android and passes it to the lib. I added the logging fromunrooted_android.c just to push it a bit further and they are printed. It's definitely far from being a thorough test, sorry I can't be more helpful. @seanm I added a 3rd commit, because testing the sample code there was a wrong type and compiler error. I guess that the right parameter is NULL for that function as it is used inunrooted_android.c, but I didn't track down the call on the source code to be 100% sure. |
| ####About `LIBUSB_OPTION_NO_DEVICE_DISCOVERY` | ||
| The method `libusb_set_option(&ctx,LIBUSB_OPTION_NO_DEVICE_DISCOVERY,NULL)` does not affect the `ctx`.It allows initializing libusb on unrootedAndroid devices by skipping device enumeration. | ||
| The method `libusb_set_option(NULL,LIBUSB_OPTION_NO_DEVICE_DISCOVERY,NULL)` does not affect the `ctx`.It allows initializing libusb on unrootedAndroid devices by skipping device enumeration. |
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.
This looks inconsistent and confusing. You have to explain better why it should be like this.
libusb_set_option(NULL, ...) sets defaults for new contexts created later. I does indeed affect ctx. However it is cleaner to specify this option when calling libusb_init_context(), instead of doing it separately.
Used markdown, now the standard on github for readme. Added a very useful section with an example of CMakeLists, so to make easier a compilation with the android default ide and processes.