| Message ID | 20211025094453.245364-1-br015@umbiko.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] package/mpd: update to version 0.23.2 | expand |
Hello Andreas, Thanks for this patch. I have some comments below, but more importantly, I would really like to get the review/approval from Jörg Krause on this patch, as he has usually been maintaining this package. On Mon, 25 Oct 2021 11:44:53 +0200 Andreas Ziegler <br015@umbiko.net> wrote: > In addition to various bug fixes, mpd version 0.23 introduces a new > dependency (mft) and a change in the configuration of the UPnP client > functionality. > > Optional new features (openmpt decoder, pipewire and snapcast outputs) are > currently not available as Buildroot packages and were not considered. We do have pipewire as a package in Buildroot, so that aspect doesn't seem completely true. If you don't handle those dependencies in mpd.mk (which is fine), could you disable them explicitly by passing the appropriate -Dfoo=disabled, if available? > The change log can be found in [1] > > Introduce new dependency for fmt library. Change configuration of UPnP plugin > to submenu, allowing to optionally select libupnp, libnpupnp or disabled. The > default setting is 'no UPnP client functionality'. Correct a typo in toolchain > dependency comment. Adapt sparc patch to changes in source layout. The typo in the toolchain dependency comment should be addressed in a separate patch, as it is really an unrelated bug fix. > diff --git a/package/mpd/Config.in b/package/mpd/Config.in > index 7a2597558b..7b5aeb4bf6 100644 > --- a/package/mpd/Config.in > +++ b/package/mpd/Config.in > @@ -8,6 +8,7 @@ menuconfig BR2_PACKAGE_MPD > depends on BR2_TOOLCHAIN_GCC_AT_LEAST_8 # C++17 > depends on BR2_HOST_GCC_AT_LEAST_8 # C++17 > select BR2_PACKAGE_BOOST > + select BR2_PACKAGE_FMT You're adding this here, but it's not added in MPD_DEPENDENCIES. Is fmt a build-time or a run-time dependency? You can check this by doing "make clean && make mpd" and verify that it does build. > select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE > help > MPD is a flexible, powerful, server-side application > @@ -361,7 +362,7 @@ config BR2_PACKAGE_MPD_LIBMPDCLIENT > > config BR2_PACKAGE_MPD_NEIGHBOR_DISCOVERY_SUPPORT > bool "neighbor discovery support" > - depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || BR2_PACKAGE_MPD_UPNP > + depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || !BR2_PACKAGE_MPD_UPNP_DISABLED > help > Enable support for neighbor discovery. > This option can be used in conjunction with the smbclient > @@ -380,13 +381,39 @@ config BR2_PACKAGE_MPD_TCP > You want this on if MPD and the client(s) work > on different machines (the usual scenario). > > -config BR2_PACKAGE_MPD_UPNP > - bool "UPnP" > +choice Kind of annoying to have a "choice" here. Is there a good reason to support both uPNP implementations? > + prompt "UPnP" > + default BR2_PACKAGE_MPD_UPNP_DISABLED > + help > + Enable MPD UPnP client support. > + > +config BR2_PACKAGE_MPD_UPNP_PUPNP > + bool "pupnp" > select BR2_PACKAGE_EXPAT > select BR2_PACKAGE_LIBUPNP > select BR2_PACKAGE_MPD_CURL > help > - Enable MPD UPnP client support. > + Provides UPnP functionality through libupnp > + (the legacy Portable SDK for UPnP devices). > + > + Introduces least additional dependencies. > + > +config BR2_PACKAGE_MPD_UPNP_NPUPNP > + bool "npupnp" > + select BR2_PACKAGE_LIBNPUPNP > + select BR2_PACKAGE_MPD_CURL > + help > + Provides UPnP functionality through libnpupnp > + (a C++ reimplementation of the Portable UPnP library). > + > + Prefer this option if you plan to use upmpdcli. > + > +config BR2_PACKAGE_MPD_UPNP_DISABLED I think I would prefer BR2_PACKAGE_MPD_UPNP_NONE. Thanks a lot! Thomas
Hi Thomas, thank you for taking a look at this patch. Some answers to your remarks below; I will also prepare a v2 of the change. I believe a legacy entry for BR2_PACKAGE_MPD_UPNP is also necessary ... On 2021-11-14 15:08, Thomas Petazzoni wrote: > Hello Andreas, > > Thanks for this patch. I have some comments below, but more > importantly, I would really like to get the review/approval from Jörg > Krause on this patch, as he has usually been maintaining this package. > > On Mon, 25 Oct 2021 11:44:53 +0200 > Andreas Ziegler <br015@umbiko.net> wrote: > >> In addition to various bug fixes, mpd version 0.23 introduces a new >> dependency (mft) and a change in the configuration of the UPnP client >> functionality. >> >> Optional new features (openmpt decoder, pipewire and snapcast outputs) >> are >> currently not available as Buildroot packages and were not considered. > > We do have pipewire as a package in Buildroot, so that aspect doesn't > seem completely true. If you don't handle those dependencies in mpd.mk > (which is fine), could you disable them explicitly by passing the > appropriate -Dfoo=disabled, if available? > package/pipewire is the server component - unfortunately also the only provider of libpipewire, which is needed here. Installing the complete package would probably satisfy the dependency, but in an embedded context it would be better to break pipewire into two components, libpipewire and pipewired. Since I have no use for this feature, I would prefer to disable it; I will also re-phrase the commit message. >> The change log can be found in [1] >> >> Introduce new dependency for fmt library. Change configuration of UPnP >> plugin >> to submenu, allowing to optionally select libupnp, libnpupnp or >> disabled. The >> default setting is 'no UPnP client functionality'. Correct a typo in >> toolchain >> dependency comment. Adapt sparc patch to changes in source layout. > > The typo in the toolchain dependency comment should be addressed in a > separate patch, as it is really an unrelated bug fix. > OK >> diff --git a/package/mpd/Config.in b/package/mpd/Config.in >> index 7a2597558b..7b5aeb4bf6 100644 >> --- a/package/mpd/Config.in >> +++ b/package/mpd/Config.in >> @@ -8,6 +8,7 @@ menuconfig BR2_PACKAGE_MPD >> depends on BR2_TOOLCHAIN_GCC_AT_LEAST_8 # C++17 >> depends on BR2_HOST_GCC_AT_LEAST_8 # C++17 >> select BR2_PACKAGE_BOOST >> + select BR2_PACKAGE_FMT > > You're adding this here, but it's not added in MPD_DEPENDENCIES. Is fmt > a build-time or a run-time dependency? > > You can check this by doing "make clean && make mpd" and verify that it > does build. > It is both, a build- and a runtime dependency. Probably works only because of alphabetical sorting of packages ... >> select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE >> help >> MPD is a flexible, powerful, server-side application >> @@ -361,7 +362,7 @@ config BR2_PACKAGE_MPD_LIBMPDCLIENT >> >> config BR2_PACKAGE_MPD_NEIGHBOR_DISCOVERY_SUPPORT >> bool "neighbor discovery support" >> - depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || BR2_PACKAGE_MPD_UPNP >> + depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || >> !BR2_PACKAGE_MPD_UPNP_DISABLED >> help >> Enable support for neighbor discovery. >> This option can be used in conjunction with the smbclient >> @@ -380,13 +381,39 @@ config BR2_PACKAGE_MPD_TCP >> You want this on if MPD and the client(s) work >> on different machines (the usual scenario). >> >> -config BR2_PACKAGE_MPD_UPNP >> - bool "UPnP" >> +choice > > Kind of annoying to have a "choice" here. Is there a good reason to > support both uPNP implementations? > Currently just one: there is a choice, there is no 'correct' option, so defer the decision to the user. libnpupnp is the more modern implementation, but impossible to configure: config options exist, but break the build, because they are not completely implemented. Buildroot therefore does a full-featured install, which uses more space than libpupnp and also brings features not needed in a client context (e.g. a web server). Since this raises additional security concerns, I would prefer the older library (which is client-only) - unless npupnp solves an issue or another package installs it anyway; currently upmpdcli does this. On the other hand, having both UPnP front-end and back-end integration is not really a valid use case ... At that point I gave up and implemented the choice. I will put some more information into the Config.in help texts. >> + prompt "UPnP" >> + default BR2_PACKAGE_MPD_UPNP_DISABLED >> + help >> + Enable MPD UPnP client support. >> + >> +config BR2_PACKAGE_MPD_UPNP_PUPNP >> + bool "pupnp" >> select BR2_PACKAGE_EXPAT >> select BR2_PACKAGE_LIBUPNP >> select BR2_PACKAGE_MPD_CURL >> help >> - Enable MPD UPnP client support. >> + Provides UPnP functionality through libupnp >> + (the legacy Portable SDK for UPnP devices). >> + >> + Introduces least additional dependencies. >> + >> +config BR2_PACKAGE_MPD_UPNP_NPUPNP >> + bool "npupnp" >> + select BR2_PACKAGE_LIBNPUPNP >> + select BR2_PACKAGE_MPD_CURL >> + help >> + Provides UPnP functionality through libnpupnp >> + (a C++ reimplementation of the Portable UPnP library). >> + >> + Prefer this option if you plan to use upmpdcli. >> + >> +config BR2_PACKAGE_MPD_UPNP_DISABLED > > I think I would prefer BR2_PACKAGE_MPD_UPNP_NONE. Good solution :-D The help texts are too technical, I will try to provide some more information here. Kind regards, Andreas > > Thanks a lot! > > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering and training > https://bootlin.com
diff --git a/package/mpd/0001-src-event-meson.build-add-atomic-dependency-for-spar.patch b/package/mpd/0001-src-event-meson.build-add-atomic-dependency-for-spar.patch index 81bd981d65..390cf12583 100644 --- a/package/mpd/0001-src-event-meson.build-add-atomic-dependency-for-spar.patch +++ b/package/mpd/0001-src-event-meson.build-add-atomic-dependency-for-spar.patch @@ -20,7 +20,7 @@ diff --git a/src/event/meson.build b/src/event/meson.build index bc13bbcd2..88370c03a 100644 --- a/src/event/meson.build +++ b/src/event/meson.build -@@ -21,9 +21,13 @@ event = static_library( +@@ -47,9 +47,13 @@ event = declare_dependency( ], ) @@ -32,8 +32,8 @@ index bc13bbcd2..88370c03a 100644 dependencies: [ + atomic_dep, thread_dep, + net_dep, system_dep, - boost_dep, -- 2.20.1 diff --git a/package/mpd/Config.in b/package/mpd/Config.in index 7a2597558b..7b5aeb4bf6 100644 --- a/package/mpd/Config.in +++ b/package/mpd/Config.in @@ -8,6 +8,7 @@ menuconfig BR2_PACKAGE_MPD depends on BR2_TOOLCHAIN_GCC_AT_LEAST_8 # C++17 depends on BR2_HOST_GCC_AT_LEAST_8 # C++17 select BR2_PACKAGE_BOOST + select BR2_PACKAGE_FMT select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE help MPD is a flexible, powerful, server-side application @@ -361,7 +362,7 @@ config BR2_PACKAGE_MPD_LIBMPDCLIENT config BR2_PACKAGE_MPD_NEIGHBOR_DISCOVERY_SUPPORT bool "neighbor discovery support" - depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || BR2_PACKAGE_MPD_UPNP + depends on BR2_PACKAGE_MPD_LIBSMBCLIENT || !BR2_PACKAGE_MPD_UPNP_DISABLED help Enable support for neighbor discovery. This option can be used in conjunction with the smbclient @@ -380,13 +381,39 @@ config BR2_PACKAGE_MPD_TCP You want this on if MPD and the client(s) work on different machines (the usual scenario). -config BR2_PACKAGE_MPD_UPNP - bool "UPnP" +choice + prompt "UPnP" + default BR2_PACKAGE_MPD_UPNP_DISABLED + help + Enable MPD UPnP client support. + +config BR2_PACKAGE_MPD_UPNP_PUPNP + bool "pupnp" select BR2_PACKAGE_EXPAT select BR2_PACKAGE_LIBUPNP select BR2_PACKAGE_MPD_CURL help - Enable MPD UPnP client support. + Provides UPnP functionality through libupnp + (the legacy Portable SDK for UPnP devices). + + Introduces least additional dependencies. + +config BR2_PACKAGE_MPD_UPNP_NPUPNP + bool "npupnp" + select BR2_PACKAGE_LIBNPUPNP + select BR2_PACKAGE_MPD_CURL + help + Provides UPnP functionality through libnpupnp + (a C++ reimplementation of the Portable UPnP library). + + Prefer this option if you plan to use upmpdcli. + +config BR2_PACKAGE_MPD_UPNP_DISABLED + bool "disabled" + help + No UPnP client functionality. + +endchoice comment "Tag plugins" @@ -398,7 +425,7 @@ config BR2_PACKAGE_MPD_ID3TAG endif -comment "mpd needs a toolchain w/ C++, threads, wchar, gcc >= 7, host gcc >= 7" +comment "mpd needs a toolchain w/ C++, threads, wchar, gcc >= 8, host gcc >= 8" depends on BR2_USE_MMU depends on BR2_TOOLCHAIN_HAS_ATOMIC depends on !BR2_INSTALL_LIBSTDCPP || !BR2_USE_WCHAR || \ diff --git a/package/mpd/mpd.hash b/package/mpd/mpd.hash index 2501be65de..af660d3690 100644 --- a/package/mpd/mpd.hash +++ b/package/mpd/mpd.hash @@ -1,3 +1,3 @@ # Locally calculated after checking pgp signature -sha256 143f7f34aaee6e87888f3dd35d49aade6656052651b960ca42b46cbb518ca0a0 mpd-0.22.11.tar.xz +sha256 74ec75689746baaeab7c65d70019f96f70b31b658cb25cfd2ebcca03f65acddf mpd-0.23.2.tar.xz sha256 ab15fd526bd8dd18a9e77ebc139656bf4d33e97fc7238cd11bf60e2b9b8666c6 COPYING diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk index fe6b8d539b..c649cd1d8f 100644 --- a/package/mpd/mpd.mk +++ b/package/mpd/mpd.mk @@ -4,8 +4,8 @@ # ################################################################################ -MPD_VERSION_MAJOR = 0.22 -MPD_VERSION = $(MPD_VERSION_MAJOR).11 +MPD_VERSION_MAJOR = 0.23 +MPD_VERSION = $(MPD_VERSION_MAJOR).2 MPD_SOURCE = mpd-$(MPD_VERSION).tar.xz MPD_SITE = http://www.musicpd.org/download/mpd/$(MPD_VERSION_MAJOR) MPD_DEPENDENCIES = host-pkgconf boost @@ -298,12 +298,18 @@ else MPD_CONF_OPTS += -Dtwolame=disabled endif -ifeq ($(BR2_PACKAGE_MPD_UPNP),y) +ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y) MPD_DEPENDENCIES += \ expat \ libupnp -MPD_CONF_OPTS += -Dupnp=enabled -else +MPD_CONF_OPTS += -Dupnp=pupnp +endif +ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y) +MPD_DEPENDENCIES += \ + libnpupnp +MPD_CONF_OPTS += -Dupnp=npupnp +endif +ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y) MPD_CONF_OPTS += -Dupnp=disabled endif
In addition to various bug fixes, mpd version 0.23 introduces a new dependency (mft) and a change in the configuration of the UPnP client functionality. Optional new features (openmpt decoder, pipewire and snapcast outputs) are currently not available as Buildroot packages and were not considered. The change log can be found in [1] Introduce new dependency for fmt library. Change configuration of UPnP plugin to submenu, allowing to optionally select libupnp, libnpupnp or disabled. The default setting is 'no UPnP client functionality'. Correct a typo in toolchain dependency comment. Adapt sparc patch to changes in source layout. [1] https://raw.githubusercontent.com/MusicPlayerDaemon/MPD/v0.23.2/NEWS Signed-off-by: Andreas Ziegler <br015@umbiko.net> --- ...build-add-atomic-dependency-for-spar.patch | 4 +- package/mpd/Config.in | 37 ++++++++++++++++--- package/mpd/mpd.hash | 2 +- package/mpd/mpd.mk | 16 +++++--- 4 files changed, 46 insertions(+), 13 deletions(-)