Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@AndrewBloom
Copy link

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.

@Youw
Copy link
Member

Youw commented Dec 21, 2024

@AndrewBloom have you considered using https://github.com/libusb/libusb-cmake instead of sample you gave?

@AndrewBloom
Copy link
Author

@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 to redo the build process using Cmake. This can lead to misalignments between libusb and their cmakelists file. For example, if a file is added to libusb source, the same has to be added on their cmakelists. The approach I documented, instead, simply delegates 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 using ndk-build).
IMO, given the fact that Android native development relies now on cmake, and 99.9% of android developers use Android Studio, the added information is extremely valuable in reducing the integration time between a new project and libusb, minimising the risk of unforeseen side effects. Moreover, the code includes some quirkiness that took me days to discover (the use of BUILD_BYPRODUCTS needed to make it work with ninja is far from obvious).
I thought to share my findings so to save some other people's time when facing the same issues, but if you think it is out of scope feel free to discard the pull request.

@Youw
Copy link
Member

Youw commented Dec 21, 2024

This can lead to misalignments between libusb and their cmakelists file. For example, if a file is added to libusb source, the same has to be added on their cmakelists.

I'm maintaining that project, and making sure that won't happen. CI helps me a lot with that.

@AndrewBloom
Copy link
Author

@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 a CMakeLists to compile libusb using only the cmake build system. I can indeed imagine some scenarios where cmake is available on the host system, but the build system supported by libusb is not, and such CMakeLists 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 the CMakelists. As a user, I want to be sure that every compilation or optimisation flag or quirkiness is used when I compile libusb, and that guarantee is given only using the official build system. The simplest and easiest solution, IMO, is to delegate the build process to the official supported build system. Thankfully, using ExternalProject functionality in cmake, 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
Copy link
Member

Youw commented Dec 22, 2024

@AndrewBloom I get your points.
Still, if you don't mind me asking, I'd like to hear if as simple as add_subdirectory(libusb-cmake) worked in your environment at all. A small feedback to help improve the project if anything is needed.

Copy link
Member

@Youw Youw left a 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
Copy link
Contributor

seanm commented Dec 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
Copy link
Author

AndrewBloom commented Dec 23, 2024

@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 :)
I did a new branch on my fork and only renamed the file there, then I rebased the patch-1 branch to that branch on my local machine and pushed forced the changes on my forked repository, and it appears that the pull request is now updated to reflect that, without me explicitly asking for it.
Unfortunately the diff is still not super useful.
Consider that I used ChatGPT to change the file to md format (basically adding the formatting for headers and such) and despite I asked explicitly to not change the contents, it took some liberties and reworded a couple of phrases. These AIs are becoming more and more rebellious.
I checked the content and everything should be there, hopefully you'll confirm that there is no significant changes too.
Ps. at least looking at the second commit you'll see the diff side by side, but it still gives all lines changed.
here: https://github.com/AndrewBloom/libusb/tree/patch-1/android you can see how the file is displayed as markdown, I think it looks clearer but I agree it's quite subjective.

@Youw I'll try the libusb-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
Copy link
Author

@Youw I switched to libusb_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 from unrooted_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 in unrooted_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 unrooted Android 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 unrooted Android devices by skipping device enumeration.
Copy link
Contributor

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.