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

Add support for setting the log callback with libusb_set_option/libusb_init_context #1265

Conversation

hjelmn
Copy link
Member

@hjelmn hjelmn commented Apr 3, 2023

This commit effectively deprecates libusb_set_log_cb by adding support for setting the callback in either libusb_set_option or libusb_init_context. Since the libusb_set_log_cb is already in use we can not easily deprecate it without first incrementing the major version. We may do that in the future.

@hjelmn hjelmn force-pushed the libusb_add_support_for_setting_callback_in_options branch 2 times, most recently from eb496ea to 777e008 Compare April 3, 2023 16:10
@mcuee mcuee added the core Related to common codes label Apr 4, 2023
@mcuee
Copy link
Member

mcuee commented Apr 4, 2023

There are CI build issues to be sorted out, for MSYS2 Clang and Appveyor (MSVC build).

@hjelmn hjelmn force-pushed the libusb_add_support_for_setting_callback_in_options branch from 777e008 to 3d8b856 Compare April 4, 2023 14:25
@hjelmn
Copy link
Member Author

hjelmn commented Apr 4, 2023

Ok, that should fix it. Figured that most compilers had implemented the typeof extension but Microsoft did not bother with it.

@hjelmn hjelmn force-pushed the libusb_add_support_for_setting_callback_in_options branch from 3d8b856 to f4618e1 Compare April 4, 2023 15:02
@mcuee
Copy link
Member

mcuee commented Apr 6, 2023

Test under Windows are good, for both MinGW64 build MSVC x64 build.

PS C:\work\libusb\libusb_pr1265> .\tests\set_option.exe (MinGW64 build)
Starting test run: test_set_log_level_basic...
Success (0)
Starting test run: test_set_log_level_env...
Success (0)
Starting test run: test_no_discovery...
Skip (3)
Starting test run: test_set_log_level_default...
Success (0)
Starting test run: test_set_log_cb...
Success (0)
---
Ran 5 tests
Passed 4 tests
Failed 0 tests
Error in 0 tests
Skipped 1 tests

PS C:\work\libusb\libusb_pr1265> .\build\v143\x64\Release\set_option.exe (MSVC build)
Starting test run: test_set_log_level_basic...
Success (0)
Starting test run: test_set_log_level_env...
Success (0)
Starting test run: test_no_discovery...
Skip (3)
Starting test run: test_set_log_level_default...
Success (0)
Starting test run: test_set_log_cb...
Success (0)
---
Ran 5 tests
Passed 4 tests
Failed 0 tests
Error in 0 tests
Skipped 1 tests

@hjelmn
Copy link
Member Author

hjelmn commented Apr 7, 2023

Alright. Looks good. Thanks @mcuee! Now to try to get one or two more reviews.

libusb/libusb.h Outdated Show resolved Hide resolved
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.

Don't see any obviouse issues with the implementation.
Even the test coverage is great.

libusb/core.c Outdated Show resolved Hide resolved
…b_init_context

This commit effectively deprecates libusb_set_log_cb by adding support for
setting the callback in either libusb_set_option or libusb_init_context. Since
the libusb_set_log_cb is already in use we can not easily deprecate it without
first incrementing the major version. We may do that in the future.

Signed-off-by: Nathan Hjelm <hjelmn@google.com>
@hjelmn hjelmn force-pushed the libusb_add_support_for_setting_callback_in_options branch from f4618e1 to d96a230 Compare April 11, 2023 16:12
libusb/core.c Show resolved Hide resolved
@Youw
Copy link
Member

Youw commented Apr 11, 2023

Nothing else catching my sight.

@hjelmn hjelmn closed this in 84ac0b0 Apr 12, 2023
tormodvolden added a commit that referenced this pull request Jul 6, 2023
This could possibly scribble the arg union for other options
although it wouldn't happen with our current set of options.

Fixup of commit 84ac0b0

References #1265

Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to common codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants