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

How to deal with deprecated libusb_init() in other projects #1236

Closed
mcuee opened this issue Jan 25, 2023 · 45 comments
Closed

How to deal with deprecated libusb_init() in other projects #1236

mcuee opened this issue Jan 25, 2023 · 45 comments
Labels
core Related to common codes

Comments

@mcuee
Copy link
Member

mcuee commented Jan 25, 2023

PR #1026 has been merged.

Now I have a problem building OpenOCD.
https://github.com/openocd-org/openocd/blob/master/src/jtag/drivers/libusb_helper.c

src/jtag/drivers/libusb_helper.c: In function ‘jtag_libusb_open’:
src/jtag/drivers/libusb_helper.c:158:2: error: ‘libusb_init’ is deprecated: Use libusb_init_context instead [-Werror=deprecated-declarations]
  158 |  if (libusb_init(&jtag_libusb_context) < 0)
      |  ^~
In file included from src/jtag/drivers/libusb_helper.h:12,
                 from src/jtag/drivers/libusb_helper.c:17:
/usr/local/include/libusb-1.0/libusb.h:1568:17: note: declared here
 1568 | int LIBUSB_CALL libusb_init(libusb_context **ctx);
      |                 ^~~~~~~~~~~
cc1: all warnings being treated as errors

For now, I have to use ./configure --disable-werror. What will be a proper fix for OpenOCD? They may have to support older libusb version.

OpenOCD has dependancy to libjaylink which has the same issue.

core.c: In function ‘jaylink_init’:
core.c:108:2: error: ‘libusb_init’ is deprecated: Use libusb_init_context instead [-Werror=deprecated-declarations]
  108 |  if (libusb_init(&context->usb_ctx) != LIBUSB_SUCCESS) {
      |  ^~
In file included from core.c:30:
/usr/local/include/libusb-1.0/libusb.h:1568:17: note: declared here
 1568 | int LIBUSB_CALL libusb_init(libusb_context **ctx);
      |                 ^~~~~~~~~~~
cc1: all warnings being treated as errors
@mcuee mcuee added the question Technical support, will be closed if deemed not a libusb issue. Please use mailing list. label Jan 25, 2023
@mcuee
Copy link
Member Author

mcuee commented Jan 25, 2023

@Youw

It affects HIDAPI as well but it is just a warning here.

hid.c: In function ‘hid_init’:
hid.c:545:3: warning: ‘libusb_init’ is deprecated: Use libusb_init_context instead [-Wdeprecated-declarations]
  545 |   if (libusb_init(&usb_context))
      |   ^~
In file included from hid.c:44:
/usr/local/include/libusb-1.0/libusb.h:1568:17: note: declared here
 1568 | int LIBUSB_CALL libusb_init(libusb_context **ctx);
      |                 ^~~~~~~~~~~

@mcuee
Copy link
Member Author

mcuee commented Jan 25, 2023

@hjelmn and @tormodvolden

Shall we keep it for now (not deprecate libusb_init) and only deprecate it in the next release (libusb-1.0.28)?

Basically it affects many projects using libusb, wmost of them (ge: HIDAPI and libftdi and avrdude) will generate a warning which is good. But for some projects it will be treated as errors like OpenOCD and libjaylink.

@mcuee
Copy link
Member Author

mcuee commented Jan 25, 2023

For our own libusb-compat-0.1 project, I will fix it later.

mcuee@UbuntuSwift3:~/build/libusb-compat-0.1$ make
make  all-recursive
make[1]: Entering directory '/home/mcuee/build/libusb-compat-0.1'
Making all in libusb
make[2]: Entering directory '/home/mcuee/build/libusb-compat-0.1/libusb'
/bin/bash ../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I..    -fvisibility=hidden -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -I/usr/local/include/libusb-1.0 -g -O2 -MT libusb_la-core.lo -MD -MP -MF .deps/libusb_la-core.Tpo -c -o libusb_la-core.lo `test -f 'core.c' || echo './'`core.c
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -fvisibility=hidden -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -I/usr/local/include/libusb-1.0 -g -O2 -MT libusb_la-core.lo -MD -MP -MF .deps/libusb_la-core.Tpo -c core.c  -fPIC -DPIC -o .libs/libusb_la-core.o
core.c: In function ‘usb_init’:
core.c:178:3: warning: ‘libusb_init’ is deprecated: Use libusb_init_context instead [-Wdeprecated-declarations]
  178 |   r = libusb_init(&ctx);
      |   ^
In file included from core.c:29:
/usr/local/include/libusb-1.0/libusb.h:1568:17: note: declared here
 1568 | int LIBUSB_CALL libusb_init(libusb_context **ctx);
      |                 ^~~~~~~~~~~
core.c:186:4: warning: ‘libusb_set_debug’ is deprecated: Use libusb_set_option instead [-Wdeprecated-declarations]
  186 |    libusb_set_debug(ctx, 3);
      |    ^~~~~~~~~~~~~~~~
In file included from core.c:29:
/usr/local/include/libusb-1.0/libusb.h:1572:18: note: declared here
 1572 | void LIBUSB_CALL libusb_set_debug(libusb_context *ctx, int level);
      |                  ^~~~~~~~~~~~~~~~
core.c: In function ‘usb_set_debug’:
core.c:196:3: warning: ‘libusb_set_debug’ is deprecated: Use libusb_set_option instead [-Wdeprecated-declarations]
  196 |   libusb_set_debug(ctx, 3);
      |   ^~~~~~~~~~~~~~~~
In file included from core.c:29:
/usr/local/include/libusb-1.0/libusb.h:1572:18: note: declared here
 1572 | void LIBUSB_CALL libusb_set_debug(libusb_context *ctx, int level);
      |                  ^~~~~~~~~~~~~~~~
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I.. -fvisibility=hidden -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -I/usr/local/include/libusb-1.0 -g -O2 -MT libusb_la-core.lo -MD -MP -MF .deps/libusb_la-core.Tpo -c core.c -o libusb_la-core.o >/dev/null 2>&1
mv -f .deps/libusb_la-core.Tpo .deps/libusb_la-core.Plo
/bin/bash ../libtool  --tag=CC   --mode=link gcc -fvisibility=hidden -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -I/usr/local/include/libusb-1.0 -g -O2 -no-undefined -version-info 8:4:4 -release 0.1  -o libusb.la -rpath /usr/local/lib libusb_la-core.lo -L/usr/local/lib -lusb-1.0 
libtool: link: gcc -shared  -fPIC -DPIC  .libs/libusb_la-core.o   -L/usr/local/lib /usr/local/lib/libusb-1.0.so  -g -O2   -pthread -Wl,-soname -Wl,libusb-0.1.so.4 -o .libs/libusb-0.1.so.4.4.4
libtool: link: (cd ".libs" && rm -f "libusb-0.1.so.4" && ln -s "libusb-0.1.so.4.4.4" "libusb-0.1.so.4")
libtool: link: (cd ".libs" && rm -f "libusb.so" && ln -s "libusb-0.1.so.4.4.4" "libusb.so")
libtool: link: ar cr .libs/libusb.a  libusb_la-core.o
libtool: link: ranlib .libs/libusb.a
libtool: link: ( cd ".libs" && rm -f "libusb.la" && ln -s "../libusb.la" "libusb.la" )
make[2]: Leaving directory '/home/mcuee/build/libusb-compat-0.1/libusb'
Making all in examples
make[2]: Entering directory '/home/mcuee/build/libusb-compat-0.1/examples'
gcc -DHAVE_CONFIG_H -I. -I..  -I../libusb  -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -g -O2 -MT lsusb.o -MD -MP -MF .deps/lsusb.Tpo -c -o lsusb.o lsusb.c
mv -f .deps/lsusb.Tpo .deps/lsusb.Po
/bin/bash ../libtool  --tag=CC   --mode=link gcc -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -g -O2   -o lsusb lsusb.o ../libusb/libusb.la 
libtool: link: gcc -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -g -O2 -o .libs/lsusb lsusb.o  ../libusb/.libs/libusb.so -pthread
gcc -DHAVE_CONFIG_H -I. -I..  -I../libusb  -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -g -O2 -MT testlibusb.o -MD -MP -MF .deps/testlibusb.Tpo -c -o testlibusb.o testlibusb.c
testlibusb.c: In function ‘print_device’:
testlibusb.c:74:55: warning: ‘ - ’ directive output may be truncated writing 3 bytes into a region of size between 1 and 256 [-Wformat-truncation=]
   74 |         snprintf(description, sizeof(description), "%s - ", string);
      |                                                       ^~~
In file included from /usr/include/stdio.h:867,
                 from testlibusb.c:7:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:67:10: note: ‘__builtin___snprintf_chk’ output between 4 and 259 bytes into a destination of size 256
   67 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |        __bos (__s), __fmt, __va_arg_pack ());
      |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mv -f .deps/testlibusb.Tpo .deps/testlibusb.Po
/bin/bash ../libtool  --tag=CC   --mode=link gcc -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -g -O2   -o testlibusb testlibusb.o ../libusb/libusb.la 
libtool: link: gcc -std=gnu99 -fgnu89-inline -Wall -Wundef -Wunused -Wstrict-prototypes -Werror-implicit-function-declaration -Wno-pointer-sign -Wshadow -g -O2 -o .libs/testlibusb testlibusb.o  ../libusb/.libs/libusb.so -pthread
make[2]: Leaving directory '/home/mcuee/build/libusb-compat-0.1/examples'
make[2]: Entering directory '/home/mcuee/build/libusb-compat-0.1'
make[2]: Leaving directory '/home/mcuee/build/libusb-compat-0.1'
make[1]: Leaving directory '/home/mcuee/build/libusb-compat-0.1'

@tormodvolden
Copy link
Contributor

tormodvolden commented Jan 25, 2023

I would be fine with delaying the deprecation a bit, also to get some wider user experiences on the new one where it will be useful, before ditching the old which for many simpler use cases works exactly the same. I see no problem in keeping the old libusb_init in parallel for a good while, just that we want any users of LIBUSB_OPTION_NO_DEVICE_DISCOVERY (almost no-one since it is rather new and its applications are probably experimental) and LIBUSB_OPTION_USE_USBDK (maybe some, but the old libusb_open has no great disadvantage here) to use libusb_init_context. Also LIBUSB_OPTION_LOG_LEVEL users can profit from libusb_init_context and avoid using the shaky "set options for any future context" procedure that kind of works right now.

But in case libusb_init_context is the way to go and it will stay, it would only be harmful to drag out the transition. Those who choose to use -Werror likely do it also for this reason, to quickly react to e.g. deprecated code. An application supporting older libusb versions probably have a bunch of LIBUSB_API_VERSION checks and alternative code already. Another check for libusb_open will need to be added. The sooner they change it the better.

The problem would be if application developers don't test against libusb git but make their new release building with -Werror before 1.0.27 is out and picked up by them. A distribution then upgrades to libusb 1.0.27 and the application won't build there without distribution tweaking. But I guess adding -Wno-deprecated flags is part of the distribution game.

@Youw
Copy link
Member

Youw commented Jan 25, 2023

It affects HIDAPI as well but it is just a warning here.

No problem, I'll update HIDAPI once libusb 1.0.27 is released.
Thanks.

@mcuee
Copy link
Member Author

mcuee commented Jan 25, 2023

@tormodvolden

In htis case, I will just send an email to the OpenOCD mailing list to mention about this change. Let's see if we get some objections or not.

@mcuee mcuee added core Related to common codes and removed question Technical support, will be closed if deemed not a libusb issue. Please use mailing list. labels Jan 26, 2023
@mcuee
Copy link
Member Author

mcuee commented Jan 26, 2023

@tormodvolden

There is a strong objection from avrdude lead developer here. avrdudes/avrdude#1295 (comment)

In my opinion, it is a very bad decision to change the API of a library that has been in use for more than a decade.
Quite possible that the original implementation of libusb doesn't do anything there – other (re)implementations are doing something, like the FreeBSD one.
If we (or anyone else) now removes the call to libusb_init(), that application will break on such implementations.

@mcuee
Copy link
Member Author

mcuee commented Jan 26, 2023

@hselasky

Just wondering if you can implement the same API for FreeBSD libusb implementations. Thanks.

@mcuee
Copy link
Member Author

mcuee commented Jan 26, 2023

@tormodvolden

Feedbacks from OpenOCD developer.

On Wed, Jan 25, 2023 at 10:00 PM Antonio Borneo wrote:

Thanks for the alert.

I don't see LIBUSB_API_VERSION incremented.
Just forgot, or it's not supposed to change for this case?

Antonio

@hselasky
Copy link

@hselasky

Just wondering if you can implement the same API for FreeBSD libusb implementations. Thanks.

You mean libusb_init_context() ?

Yes, FreeBSD can do that. Can you send me a patch for /usr/src/lib/libusb to [email protected] ?

In worst case, we can also use symbol versioning, but it is good that new and different names are chosen when changing libusb APIs. That's the best way forward. Anyway. I see no problem supporting libusb 0.1 in the future aswell. Some people want it simple and some people need more complicated features as offered by libusb 1.0. I see no reason to support the one-shoe fits everyone thought, but other people may.

Again, send me a patch and I'll look into it.

@tormodvolden
Copy link
Contributor

@tormodvolden

There is a strong objection from avrdude lead developer here. avrdudes/avrdude#1295 (comment)

About time they rear they heads then, these API changes have been out for discussions for years here now.

In my opinion, it is a very bad decision to change the API of a library that has been in use for more than a decade.
Quite possible that the original implementation of libusb doesn't do anything there – other (re)implementations are doing something, like the FreeBSD one.

I don't understand exactly what he means by "there". Maybe he didn't get that libusb_init is to be replaced by libusb_init_context.

If we (or anyone else) now removes the call to libusb_init(), that application will break on such implementations.

@Youw
Copy link
Member

Youw commented Jan 26, 2023

What are the downside(s) of keeping the libusb_init(ctx) as a documented alias for libusb_initialize(ctx, NULL, 0)?
Keeping it that way would mean full backward compativility (including guarantees of the Version semantics) and the "old" API can alwasy be removed, if libusb ever lives up to v2.0.

@tormodvolden
Copy link
Contributor

tormodvolden commented Jan 26, 2023

On Wed, Jan 25, 2023 at 10:00 PM Antonio Borneo wrote:

I don't see LIBUSB_API_VERSION incremented.
Just forgot, or it's not supposed to change for this case?

We will of course bump LIBUSB_API_VERSION, just not happened in the git tree yet. It needs to be bumped for all the new API functions we just added.

EDIT: Done in commit 54350bd

@mcuee
Copy link
Member Author

mcuee commented Jan 26, 2023

Again, send me a patch and I'll look into it.

@hselasky

Not from me anyway. I am not so sure who would like to do it. I actually think you are the one to do it for FreeBSD. :-)

@hselasky
Copy link

@mcuee: May I of total lazyness ask for a link to the implementation of the needed function in Linux's LibUSB?

@mcuee
Copy link
Member Author

mcuee commented Jan 26, 2023

@mcuee: May I of total lazyness ask for a link to the implementation of the needed function in Linux's LibUSB?

I see.

This is the PR.

@tormodvolden
Copy link
Contributor

tormodvolden commented Jan 26, 2023

Why would something need to be changed for FreeBSD? EDIT: Got it, they have their own reimplementation, we only do NetBSD and OpenBSD.

@hselasky
Copy link

@tormodvolden : It's not really an own version, but more a USB "backend" redesign inside LibUSB, which optimizes and reduce error situations under FreeBSD. We try to keep up with new ideas and APIs in LibUSB, but at the same time appreciate that some things are written in stone, so to speak. Also due to Licensing issues, this was needed, because FreeBSD uses primarly a 2-clause-BSD license, and LibUSB is now part of the FreeBSD base system. I think NetBSD and OpenBSD only provide this library in their "ports", so the issue is not quite the same.

@hselasky
Copy link

To anyone interested, I've put up a review for FreeBSD:
https://reviews.freebsd.org/D38212

As a FreeBSD libusb maintainer I would be very happy if:

LIBUSB_DEPRECATED_FOR(libusb_init_context)

and

LIBUSB_DEPRECATED_FOR(libusb_set_option)

was removed from libusb.h to avoid unneeded churn in the code bases around the world.

The new functions are technically fine, but really have no value for FreeBSD except for the possibility to atomically supply the default libusb debug value during context init.

Also forcing users to set the debug value via libusb_set_option() is not really good for anything except breaking backwards compatibility and forcing users to upgrade.

@tormodvolden : If your intent with these changes is to force libusb users to upgrade, then I agree about the deprecated thing. Else not.

Technically, if libusb was a C++ library, then this would not be an issue at all I presume.

@tormodvolden
Copy link
Contributor

Also forcing users to set the debug value via libusb_set_option() is not really good for anything except breaking backwards compatibility and forcing users to upgrade.

I am also not so happy with this. I think the intention was to not have to support (and encourage) too many ways of doing things. In the larger picture I am not sure I like so much libusb_set_option and how we try to squeeze everything through it but that's a longer story. What exactly do you mean by "forcing users to upgrade"? Forcing application developers to update their code, inserting more LIBUSB_API_VERSION checks? Yes, but it wouldn't force end users and distributions to upgrade libusb at the same time, and the applications would still build against older libusb. Old applications would also link fine against newer libusb, the deprecation is only a compilation warning.

I would be happy keeping libusb_init not deprecated in libusb 1.0.x (also since its implementation costs us nothing), but rather find ways to hit down on applications attempting libusb_set_options with NO_DEVICE_DISCOVERY or USE_USBDK.

@hselasky
Copy link

@tormodvolden : Many people are pedandic about C-code and use -Werror and -Wall by default. When an upstream project sees a compiler error like "deprecated function", they will fix it and use the new function, but that also pulls in the requirement for upgrading the libusb package. Because the libusb in FreeBSD is in the base system, that requires a base system upgrade vs a package upgrade.

@Youw
Copy link
Member

Youw commented Jan 26, 2023

Old applications would also link fine against newer libusb, the deprecation is only a compilation warning.

That is not entirely true. So many projects are being compiled with -Werror (or /WX on MSVC). And by "simply upgrading the libusb version" those projects will fail to build.

And while for GCC/Clang there is a workaround (-Wno-error=deprecated-declarations), for MSVC it is impossible not to threat warnings as errors (with /WX flag), so the only workaround is /wd4996 - completely disable the warning, which beats its purpose in a first place.

@tormodvolden
Copy link
Contributor

I was talking about run-time linking, and in the context of end users and distributions. You can build an application against libusb 1.0.26, then upgrade libusb to 1.0.27 and the same application binary will run fine (maybe even better, since libusb has improved). There will be no deprecation warning.

@tormodvolden
Copy link
Contributor

When an upstream project sees a compiler error like "deprecated function", they will fix it and use the new function, but that also pulls in the requirement for upgrading the libusb package.

They would be doing it wrong. They should simply insert #if LIBUSB_API_VERSION >= 0x0100010A \n libusb_init_context(ctx, NULL, 0); \n #else \n libusb_init(ctx); \n #endif

Then the requirements won't need bumping and everybody is happy.

(Of course, as upstream developers we'd like everybody to use our latest version, so they can enjoy our improvements and not fight with resolved bugs - but within reason.)

@tormodvolden
Copy link
Contributor

for MSVC it is impossible not to threat warnings as errors (with /WX flag), so the only workaround is /wd4996 - completely disable the warning, which beats its purpose in a first place.

With this MSVC logic, what would be a valid case for using deprecation at all then? One could as well just rip out the old function. Only difference that the (error) message shows which new function to use. OTOH deprecated means it is available, but should be replaced.

@tormodvolden
Copy link
Contributor

So many projects are being compiled with -Werror (or /WX on MSVC). And by "simply upgrading the libusb version" those projects will fail to build.

If a project chooses to build with -Werror without -Wno-deprecated, this is exactly what they ask for. They want to be sure a deprecated function is identified and replaced rather than going unnoticed. If you want your project to build in as many situations as possible without having to maintain it, you shouldn't use -Werror without -Wno-deprecated.

@Youw
Copy link
Member

Youw commented Jan 26, 2023

this is exactly what they ask for

That is a fare point

what would be a valid case for using deprecation at all then?

I find it a "design bug" of MSVC. Don't think there is in such scenarios.

in the context of end users and distributions. You can build an application against libusb 1.0.26, then upgrade libusb to 1.0.27 and the same application binary will run fine

That is true in cases when all packages are distributed in a binary form, but there are things like Yocto/Gentoo/(I imagine maby others), when all packages are always built from sources.
I'm not saying the next statement is 100% true, but there likely be a project that will use -Werror.

And imagine an application that is linking against libusb1.0.24 and totally happy with its functionality. There is a choice - force to upgrade, or postpone upgrade of libusb for everyone (that's what most likely to happen).

what would be a valid case for using deprecation at all then?

Qt actually has an interesting mechanism for that: they use a macro QT_DEPRECATED_SINCE (sample), and when application is being compiled against Qt, it has to specify the target/expected API version (enforced via CMake/QMake scripts).

So ideally would be great to specify that the API is deprecated only for those users that are targeting the latest/specific version of libusb.
But I don't believe C (or any other tools) provide such functionality of-the-box.

So that's more an idea to think about rather then a real action item here.

One could as well just rip out the old function.

That would be no longer a libusb1.0 or even libusb1.x library at all, if any basic backward compatibility is to be preserved.

@mariusgreuel
Copy link

Shall we keep it for now (not deprecate libusb_init) and only deprecate it in the next release

Looking at the current implementation of libusb_init, I see no reason to deprecate this function at all.

IMHO, for a popular library such as libusb, there needs to be a strong technical reason (say 'security issue' or 'high maintenance') to deprecate a public function API.

@tormodvolden
Copy link
Contributor

I find it a "design bug" of MSVC. Don't think there is in such scenarios.

Anyway, MSVC is already special-cased in our definition of LIBUSB_DEPRECATED_FOR() so it is would be easy to just disable deprecation warnings there altogether if we want to keep these functions tagged as deprecated.

Interestingly, libusb_set_debug() was deprecated already in 1.0.20, but that has been going quiet without uproar as far as I know. Is it really that little used?

or postpone upgrade of libusb for everyone (that's what most likely to happen).

You mean a distribution will hold back libusb instead of patching the offending applications (or simply adding -Wno-deprecated)? I don't think so lowly of distribution packagers. Or you mean static linking? So not distributions. Some lazy application developer, basing their program on libusb, but not willing to make a small adaption to be able to include the newer libusb? I cannot really imagine this either. I guess we don't disagree that much, but it is good to get all perspectives on the table.

I reckon we want some kind of soft "deprecation" which makes application developers take notice, but doesn't break anything. That is what the GCC warnings are meant for, I believe. But I understand the common use of -Werror makes it different.

I would prefer we don't technically deprecate it with compiler attributes yet, but solely deprecate it in documentation and examples.

@Youw
Copy link
Member

Youw commented Jan 26, 2023

I would prefer we don't technically deprecate it with compiler attributes yet, but solely deprecate it in documentation and examples.

Sounds excellent.

it is good to get all perspectives on the table

My feeling about this conversation is mutual.

going quiet without uproar as far as I know. Is it really that little used?

Quick searching over Github shows quite a bit of usages (but I imagine lots of those are forks, etc., but still) and patches/fixes. Could be a number of reasons/coincidences why people didn't complain about it - mayby that really was a "natural" change.

I don't think so lowly of distribution packagers.

Lets assume that most/major distribution packagers really are disciplined-enough and will deal with such cases, i.e. will patch broken packages, etc.
But imagine not all of the projects that use libusb have used, for example, libusb_set_debug - fixing it simply less efforts, compared to libusb_init - it is used in a 100% of projects that use libusb (and no one can really know how many is that).

Secondly - there're distributions, most library developers wouldn't even know about. I mean closed/corporate solutions/etc., and of course they are still compliant with the library's license.

I had a case on my project about 3 years ago (yes, a corporate solution), when we couldn't upgrade a Yocto/BSP to a newer version for more than 8 months, because a change in one of the (non-direct) dependencies broke compilation of our Core module (and we couldn't "just fix it" - it was complicated at the moment).

@xloem
Copy link
Contributor

xloem commented Jan 27, 2023

Hey, I just found this thread.

Seeing the impact, in my opinion deprecating the function was a mistake. The role of a library is to make maintenance safe and easy for downstream users. We didn't hold this.

As it stands, this likely propagates breaking changes to unmaintained projects that used to build fine.

@xloem
Copy link
Contributor

xloem commented Jan 27, 2023

With all this attention, it seems important to mention that libusb is badly in need of maintainers. There was almost nobody available to review the change that preceded this issue.

@Youw
Copy link
Member

Youw commented Jan 27, 2023

Maybe not a full-time mantainer, but I'll find more time at least reviewing the PRs.

@tormodvolden
Copy link
Contributor

Seeing the impact, in my opinion deprecating the function was a mistake. The role of a library is to make maintenance safe and easy for downstream users. We didn't hold this.

Hey, hold on, we haven't released this yet :) We are free to test out things in our development git repository. This discussion is about what we will do in the release.

@tormodvolden
Copy link
Contributor

tormodvolden commented Jan 27, 2023

But imagine not all of the projects that use libusb have used, for example, libusb_set_debug - fixing it simply less efforts, compared to libusb_init - it is used in a 100% of projects that use libusb (and no one can really know how many is that).

This is true, many programs don't use libusb_set_debug. We can also safely say that many (simple or older) programs won't need the features of libusb_init_context and are not affected by the limitations of libusb_init.

@tormodvolden
Copy link
Contributor

tormodvolden commented Jan 27, 2023

With all this attention, it seems important to mention that libusb is badly in need of maintainers. There was almost nobody available to review the change that preceded this issue.

The suggested changes have also been brought up on the libusb mailing list many times. I am disappointed that projects that rely on libusb are not following the mailing list.

@mcuee
Copy link
Member Author

mcuee commented Jan 27, 2023

I am disappointed that projects that rely on libusb are not following the mailing list.

I think people expect basic libusb functions like libusb_init() should work no matter what is the change in libusb as long as the version number is 1.x.

@tormodvolden
Copy link
Contributor

I think people expect basic libusb functions like libusb_init() should work no matter what is the change in libusb as long as the version number is 1.x.

Yes, libusb_init() is guaranteed to continue to work in 1.0.x (we haven't adopted the full x.y.z semantic version convention yet, but that is another discussion, so maybe even 1.x.x). I think everybody has been agreeing to that from the start. Marking something as deprecated doesn't make it not work. The question is if we should do these -Werrors people a favour and not deprecate using compiler attributes.

@mcuee
Copy link
Member Author

mcuee commented Jan 27, 2023

Yes, libusb_init() is guaranteed to continue to work in 1.0.x (we haven't adopted the full x.y.z semantic version convention yet, but that is another discussion, so maybe even 1.x.x). I think everybody has been agreeing to that from the start. Marking something as deprecated doesn't make it not work. The question is if we should do these -Werrors people a favour and not deprecate using compiler attributes.

Not only -Werror people. It seems to me OpenOCD project is willing to change the codes. Rather you can see other people are against the change. Personally I do not like to see the warning because of this change. To me it is not a necessary change at least for 1.0.x series.

Actually I think we should not deprecate of libusb_set_debug either.

On a related note, you can see libusb_set_option() is causing problems for Python bindings.

Related discussion:

@mcuee
Copy link
Member Author

mcuee commented Jan 29, 2023

The fix from OpenOCD project seems to be simple enough.
https://review.openocd.org/c/openocd/+/7467/2/src/jtag/drivers/libusb_helper.c

#if !defined(LIBUSB_API_VERSION) || (LIBUSB_API_VERSION < 0x0100010A)
#define libusb_init_context(a, b, c) libusb_init(a)
#endif

@mcuee
Copy link
Member Author

mcuee commented Jan 29, 2023

@hselasky

I believe the above change from OpenOCD is also okay for FreeBSD. But I am just wondering if you want to implement something like LIBUSB_API_VERSION for FreeBSD, maybe LIBUSB_FREEBSD_API_VERSION or similar.

@hselasky
Copy link

I have a change for FreeBSD. This should be fixed in libusb itself by removing those deprecated warnings like already agreed!

@mcuee
Copy link
Member Author

mcuee commented Jan 31, 2023

I have a change for FreeBSD. This should be fixed in libusb itself by removing those deprecated warnings like already agreed!

I have created PR #1241. Let's see if other maintainers approve the PR or not.

@hjelmn
Copy link
Member

hjelmn commented Feb 10, 2023

I have no strong feelings one way or another. Deprecation of an old function is done a lot and precedes the removal in the next major version. It is annoying that -Werror fails the compile in this case because it is perfectly fine to use it. Just don't count on it being held over to the next major version (if we ever do one that is). Documenting the deprecation is a good work around.

@hjelmn
Copy link
Member

hjelmn commented Feb 10, 2023

Do have to say this issue came about not from lack of maintainers but from lack of involvement from users. We asked for feedback on multiple occasions. Oh well.

tormodvolden pushed a commit to tormodvolden/libusb that referenced this issue Mar 12, 2023
These functions are used in a lot of existing downstream code
and the deprecation makes -Werror builds fail.

It seems reasonable to keep these functions supported at least until a
major API overhaul.

Closes libusb#990
Closes libusb#1236
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

No branches or pull requests

7 participants