| Message ID | 20170905100327.21421-1-aleksander@aleksander.es |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libqmi: udev and qmi-over-mbim are optional | expand |
Hello, On Tue, 5 Sep 2017 12:03:27 +0200, Aleksander Morgado wrote: > Don't always build without udev, as qmi-firmware-update would be very > very limited in that case. Instead, make it optional: if there is udev > support in the setup, require libgudev and configure using --with-udev > explicitly; otherwise just --without-udev. > > Also, add the qmi-over-mbim feature as optional, and require libmbim > if we're building with it enabled. > > Signed-off-by: Aleksander Morgado <aleksander@aleksander.es> > --- > package/libqmi/Config.in | 18 ++++++++++++++++++ > package/libqmi/libqmi.mk | 18 ++++++++++++++++-- > 2 files changed, 34 insertions(+), 2 deletions(-) > > diff --git a/package/libqmi/Config.in b/package/libqmi/Config.in > index f1d111b7c..a536650b5 100644 > --- a/package/libqmi/Config.in > +++ b/package/libqmi/Config.in > @@ -10,6 +10,24 @@ config BR2_PACKAGE_LIBQMI > > http://www.freedesktop.org/wiki/Software/libqmi/ > > +if BR2_PACKAGE_LIBQMI > + > +config BR2_PACKAGE_LIBQMI_UDEV > + bool "qmi-firmware-update udev support" > + depends on BR2_PACKAGE_HAS_UDEV > + select BR2_PACKAGE_LIBGUDEV libgudev has plenty of other dependencies that you need to propagate here. > + help > + This option enables udev support in the qmi-firmware-update tool > + > +config BR2_PACKAGE_LIBQMI_MBIM_QMUX > + bool "QMI-over-MBIM support" > + select BR2_PACKAGE_LIBMBIM ... and libmbim also has plenty of dependencies that you need to propagate here, including BR2_PACKAGE_HAS_UDEV. All in all, isn't it simpler to get rid of those options, and simply do: ifeq ($(BR2_PACKAGE_LIBGUDEV),y) ... enable support else ... disable support endif ifeq ($(BR2_PACKAGE_LIBMBIM),y) ... enable support else ... disable support endif Thanks! Thomas
Hey > > On Tue, 5 Sep 2017 12:03:27 +0200, Aleksander Morgado wrote: >> Don't always build without udev, as qmi-firmware-update would be very >> very limited in that case. Instead, make it optional: if there is udev >> support in the setup, require libgudev and configure using --with-udev >> explicitly; otherwise just --without-udev. >> >> Also, add the qmi-over-mbim feature as optional, and require libmbim >> if we're building with it enabled. >> >> Signed-off-by: Aleksander Morgado <aleksander@aleksander.es> >> --- >> package/libqmi/Config.in | 18 ++++++++++++++++++ >> package/libqmi/libqmi.mk | 18 ++++++++++++++++-- >> 2 files changed, 34 insertions(+), 2 deletions(-) >> >> diff --git a/package/libqmi/Config.in b/package/libqmi/Config.in >> index f1d111b7c..a536650b5 100644 >> --- a/package/libqmi/Config.in >> +++ b/package/libqmi/Config.in >> @@ -10,6 +10,24 @@ config BR2_PACKAGE_LIBQMI >> >> http://www.freedesktop.org/wiki/Software/libqmi/ >> >> +if BR2_PACKAGE_LIBQMI >> + >> +config BR2_PACKAGE_LIBQMI_UDEV >> + bool "qmi-firmware-update udev support" >> + depends on BR2_PACKAGE_HAS_UDEV >> + select BR2_PACKAGE_LIBGUDEV > > libgudev has plenty of other dependencies that you need to propagate > here. > >> + help >> + This option enables udev support in the qmi-firmware-update tool >> + >> +config BR2_PACKAGE_LIBQMI_MBIM_QMUX >> + bool "QMI-over-MBIM support" >> + select BR2_PACKAGE_LIBMBIM > > ... and libmbim also has plenty of dependencies that you need to > propagate here, including BR2_PACKAGE_HAS_UDEV. > Oh, I assumed that was somehow automatic... Does this mean that if the deps of a library are updated at some point in time, all the apps depending on the library also need to get updated to add the new dep? > All in all, isn't it simpler to get rid of those options, and simply do: > > ifeq ($(BR2_PACKAGE_LIBGUDEV),y) > ... enable support > else > ... disable support > endif > > ifeq ($(BR2_PACKAGE_LIBMBIM),y) > ... enable support > else > ... disable support > endif > Yes, probably that's a much simpler option. Will suggest a new patch. Thanks for reviewing!
Hello, On Tue, 5 Sep 2017 21:45:19 +0200, Aleksander Morgado wrote: > > ... and libmbim also has plenty of dependencies that you need to > > propagate here, including BR2_PACKAGE_HAS_UDEV. > > > > Oh, I assumed that was somehow automatic... Does this mean that if the > deps of a library are updated at some point in time, all the apps > depending on the library also need to get updated to add the new dep? Yes, exactly. When we review patches adding new dependencies to an existing package, we are always careful that those new dependencies are propagated. > Yes, probably that's a much simpler option. Will suggest a new patch. Thanks! Thomas
> > All in all, isn't it simpler to get rid of those options, and simply do: > > ifeq ($(BR2_PACKAGE_LIBGUDEV),y) > ... enable support When doing this, should I also include a direct dependency on libgudev, so that libgudev is built before libqmi always? LIBQMI_DEPENDENCIES += libgudev > else > ... disable support > endif >
Hello, On Wed, 6 Sep 2017 10:38:25 +0200, Aleksander Morgado wrote: > > All in all, isn't it simpler to get rid of those options, and simply do: > > > > ifeq ($(BR2_PACKAGE_LIBGUDEV),y) > > ... enable support > > When doing this, should I also include a direct dependency on > libgudev, so that libgudev is built before libqmi always? > LIBQMI_DEPENDENCIES += libgudev Well, if you don't do this, libgudev is not guaranteed to be built before libqmi, so you would get a build failure, right ? The canonical way to express optional dependencies is: ifeq ($(BR2_PACKAGE_FOO),y) BAR_DEPENDENCIES += foo BAR_CONF_OPTS += --enable-foo else BAR_CONF_OPTS += --disable-foo endif Best regards, Thomas
diff --git a/package/libqmi/Config.in b/package/libqmi/Config.in index f1d111b7c..a536650b5 100644 --- a/package/libqmi/Config.in +++ b/package/libqmi/Config.in @@ -10,6 +10,24 @@ config BR2_PACKAGE_LIBQMI http://www.freedesktop.org/wiki/Software/libqmi/ +if BR2_PACKAGE_LIBQMI + +config BR2_PACKAGE_LIBQMI_UDEV + bool "qmi-firmware-update udev support" + depends on BR2_PACKAGE_HAS_UDEV + select BR2_PACKAGE_LIBGUDEV + help + This option enables udev support in the qmi-firmware-update tool + +config BR2_PACKAGE_LIBQMI_MBIM_QMUX + bool "QMI-over-MBIM support" + select BR2_PACKAGE_LIBMBIM + help + This option enables support to use the QMI protocol over MBIM + for modems with MBIM_SERVICE_QMI capabilities + +endif + comment "libqmi needs a toolchain w/ wchar, threads" depends on BR2_USE_MMU depends on !BR2_USE_WCHAR || !BR2_TOOLCHAIN_HAS_THREADS diff --git a/package/libqmi/libqmi.mk b/package/libqmi/libqmi.mk index 917265f4b..129fd0fb6 100644 --- a/package/libqmi/libqmi.mk +++ b/package/libqmi/libqmi.mk @@ -15,7 +15,21 @@ LIBQMI_AUTORECONF = YES LIBQMI_DEPENDENCIES = libglib2 -# we don't want -Werror and disable gudev Gobject bindings -LIBQMI_CONF_OPTS = --enable-more-warnings=no --without-udev +# we don't want -Werror +LIBQMI_CONF_OPTS = --enable-more-warnings=no + +ifeq ($(BR2_PACKAGE_LIBQMI_UDEV),y) +LIBQMI_DEPENDENCIES += libgudev +LIBQMI_CONF_OPTS += --with-udev +else +LIBQMI_CONF_OPTS += --without-udev +endif + +ifeq ($(BR2_PACKAGE_LIBQMI_MBIM_QMUX),y) +LIBQMI_DEPENDENCIES += libmbim +LIBQMI_CONF_OPTS += --enable-mbim-qmux +else +LIBQMI_CONF_OPTS += --disable-mbim-qmux +endif $(eval $(autotools-package))
Don't always build without udev, as qmi-firmware-update would be very very limited in that case. Instead, make it optional: if there is udev support in the setup, require libgudev and configure using --with-udev explicitly; otherwise just --without-udev. Also, add the qmi-over-mbim feature as optional, and require libmbim if we're building with it enabled. Signed-off-by: Aleksander Morgado <aleksander@aleksander.es> --- package/libqmi/Config.in | 18 ++++++++++++++++++ package/libqmi/libqmi.mk | 18 ++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-)