| Message ID | 20220331132230.227424-1-br015@umbiko.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] package/mpd: explicitly disable features to avoid collision with host packages | expand |
On 31/03/2022 15:22, Andreas Ziegler wrote: > Background > During configuration, the meson build system tries to determine the > availability of optional dependencies using (host) pkgconfig and cmake in that > order. If a library does not exist on the target, pkg-config will fail, but > cmake sometimes finds and reports libraries that exist as host packages. This > has been observed for host-expat (cmake dependency) and host-zlib. The link > step subsequently fails, because necessary files are not present in the target > architecture. > > Unconditionally disable optional features often found in host binaries and > modify the menu selection processing in mpd.mk to re-enable them where > necessary. Currently this concerns expat and zlib only. > > This fixes the following build errors: > > [expat] > /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: warning: libc.so.6, needed by /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7, not found (try using -rpath or -rpath-link) > /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7: undefined reference to `__errno_location@GLIBC_2.2.5' > > [zlib] > http://autobuild.buildroot.net/results/f0a/f0a9e719114f19dc9d20622ed85dd4f8e968c20f/ > > Signed-off-by: Andreas Ziegler <br015@umbiko.net> > --- > package/mpd/mpd.mk | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk > index 12da36098f..4e67c9428c 100644 > --- a/package/mpd/mpd.mk > +++ b/package/mpd/mpd.mk > @@ -14,6 +14,7 @@ MPD_LICENSE_FILES = COPYING > # these refer to the FreeBSD PPP daemon > MPD_IGNORE_CVES = CVE-2020-7465 CVE-2020-7466 > MPD_SELINUX_MODULES = mpd > +# These features are either unwanted or not selectable via the Buildroot menu > MPD_CONF_OPTS = \ > -Daudiofile=disabled \ > -Ddocumentation=disabled \ > @@ -21,6 +22,12 @@ MPD_CONF_OPTS = \ > -Dpipewire=disabled \ > -Dsnapcast=false > > +# Explicitly disable features where meson's dependency detection picks up host > +# libraries. These settings can be overridden through menu options later > +MPD_CONF_OPTS += \ > + -Dexpat=disabled \ > + -Dzlib=disabled It would be better to make these automatic conditional dependencies, like it's done for ICU. > + > # Zeroconf support depends on libdns_sd from avahi. > ifeq ($(BR2_PACKAGE_MPD_AVAHI_SUPPORT),y) > MPD_DEPENDENCIES += avahi > @@ -300,11 +307,11 @@ ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y) > MPD_DEPENDENCIES += \ > expat \ That would also allow to remove this (it will be done implicitly since expat is selected in Config.in and the automatic dependency will add it). > libupnp > -MPD_CONF_OPTS += -Dupnp=pupnp > +MPD_CONF_OPTS += -Dupnp=pupnp -Dexpat=enabled > else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y) > MPD_DEPENDENCIES += \ > libnpupnp > -MPD_CONF_OPTS += -Dupnp=npupnp > +MPD_CONF_OPTS += -Dupnp=npupnp -Dexpat=enabled This is wrong: no dependency on expat is added so expat may not even be selected or built at this point. Marked as Changes Requested. Regards, Arnout > else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y) > MPD_CONF_OPTS += -Dupnp=disabled > endif
Hi Arnout, Thank you for taking a look. On 2022-04-04 17:38, Arnout Vandecappelle wrote: > On 31/03/2022 15:22, Andreas Ziegler wrote: >> Background >> During configuration, the meson build system tries to determine the >> availability of optional dependencies using (host) pkgconfig and cmake >> in that >> order. If a library does not exist on the target, pkg-config will >> fail, but >> cmake sometimes finds and reports libraries that exist as host >> packages. This >> has been observed for host-expat (cmake dependency) and host-zlib. The >> link >> step subsequently fails, because necessary files are not present in >> the target >> architecture. >> >> Unconditionally disable optional features often found in host binaries >> and >> modify the menu selection processing in mpd.mk to re-enable them where >> necessary. Currently this concerns expat and zlib only. >> >> This fixes the following build errors: >> >> [expat] >> /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: >> warning: libc.so.6, needed by >> /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7, not found (try >> using -rpath or -rpath-link) >> /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: >> /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7: undefined >> reference to `__errno_location@GLIBC_2.2.5' >> >> [zlib] >> http://autobuild.buildroot.net/results/f0a/f0a9e719114f19dc9d20622ed85dd4f8e968c20f/ >> >> Signed-off-by: Andreas Ziegler <br015@umbiko.net> >> --- >> package/mpd/mpd.mk | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk >> index 12da36098f..4e67c9428c 100644 >> --- a/package/mpd/mpd.mk >> +++ b/package/mpd/mpd.mk >> @@ -14,6 +14,7 @@ MPD_LICENSE_FILES = COPYING >> # these refer to the FreeBSD PPP daemon >> MPD_IGNORE_CVES = CVE-2020-7465 CVE-2020-7466 >> MPD_SELINUX_MODULES = mpd >> +# These features are either unwanted or not selectable via the >> Buildroot menu >> MPD_CONF_OPTS = \ >> -Daudiofile=disabled \ >> -Ddocumentation=disabled \ >> @@ -21,6 +22,12 @@ MPD_CONF_OPTS = \ >> -Dpipewire=disabled \ >> -Dsnapcast=false >> +# Explicitly disable features where meson's dependency detection >> picks up host >> +# libraries. These settings can be overridden through menu options >> later >> +MPD_CONF_OPTS += \ >> + -Dexpat=disabled \ >> + -Dzlib=disabled > > It would be better to make these automatic conditional dependencies, > like it's done for ICU. Good idea, but inclusion of ICU should be initiated by Config.in, like every other feature. >> + >> # Zeroconf support depends on libdns_sd from avahi. >> ifeq ($(BR2_PACKAGE_MPD_AVAHI_SUPPORT),y) >> MPD_DEPENDENCIES += avahi >> @@ -300,11 +307,11 @@ ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y) >> MPD_DEPENDENCIES += \ >> expat \ > > That would also allow to remove this (it will be done implicitly > since expat is selected in Config.in and the automatic dependency will > add it). > >> libupnp >> -MPD_CONF_OPTS += -Dupnp=pupnp >> +MPD_CONF_OPTS += -Dupnp=pupnp -Dexpat=enabled >> else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y) >> MPD_DEPENDENCIES += \ >> libnpupnp >> -MPD_CONF_OPTS += -Dupnp=npupnp >> +MPD_CONF_OPTS += -Dupnp=npupnp -Dexpat=enabled > > This is wrong: no dependency on expat is added so expat may not even > be selected or built at this point. Actually, the dependency is inherited from libnpupnp, but you are right, this handling is not intuitive. > Marked as Changes Requested. Outline of the proposed next version of this change: [I did not want to make so many changes, but you are right, it makes sense.] Separate logic from implementation: Remove feature dependencies from mpd.mk and put selection logic into Config.in exclusively. In the makefile, a feature is responsible only to select or deselect its configure option and, if this feature relies on a library, add a dependency on this. Duplicated selection logic in mpd.mk will be eliminated. A build dependency will only be added if it is present in the mpd configuration, and not be inherited accidentally from concurrent packages. This impacts the following features: expat, id3tag, yajl. id3tag already is half implemented, but not used consistently (has an option entry in Config.in, but does not select this, but handles dependencies individually). expat and yajl would be created as hidden options without visibility (no user interaction necessary). Separate vorbis decoder and encoder options; currently the decoder feature BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis encoding, which is probably not intended. Create entry 'unicode' for ICU in Config.in. Set zlib to disabled (not used currently). Thoughts? Kind regards, Andreas > Regards, > Arnout > >> else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y) >> MPD_CONF_OPTS += -Dupnp=disabled >> endif
[Putting the other maintainers in Cc here because there's a bit of a policy/philosophy decision.] On 08/04/2022 15:22, Andreas Ziegler wrote: > Hi Arnout, > > [snip] > Outline of the proposed next version of this change: > > [I did not want to make so many changes, but you are right, it makes sense.] > > Separate logic from implementation: Remove feature dependencies from mpd.mk and > put selection logic into Config.in exclusively. As you can see from the description above: this is making things quite complicated. And one of the tenets of Buildroot is to keep things simple. That is the reason why we generally want to avoid sub-options in Config.in. Basically we only want sub-options in the following cases: - It makes a big difference in the final target installed size if the option is kept disabled. E.g. BR2_PACKAGE_AVAHI_DAEMON. - There are multiple possibilities and depending on the circumstances, one can be more appropriate than the other. E.g. libcurl SSL/TLS library to use. - It is not obvious which other package needs to be enabled to enable a feature. E.g. BR2_PACKAGE_BLUEZ_ALSA_HCITOP. - The package has many sub-features with various dependencies, it is typically a major top-level package (i.e. something that the user wants explicitly and is not pulled in by something else), and the sub-features are user visible. The mpd sub-options fall in the latter category. It's the most tricky one to evaluate because it pulls in complexity with a difficult trade-off. It's also the one I dislike most, because it's very subjective. A way to make it more objective is to make a user-visible option for each config option that the package provides. However, I think that easily leads to an unnecessary explosion of Config.in options with no benefit for the user and possibly causing confusion. And it's not necessarily that simple either, cfr. mesa3d (though in that case it's going to be complicated however it's approached). We don't have a consistent policy for this case, but I believe the policy should be: - Add Config.in options only for features that are important, meaningful for the user (e.g. codec support). - Add Config.in options only for features that have a size impact (usually due to the dependencies they pull in). - In the .mk file, don't use the Config.in options but instead use automatic dependencies only. That way, the .mk file is kept simple (no problematic cases like the libupnp/expat interaction in this patch). The Config.in is a bit complicated, but it doesn't explode. Bottom line: I think it's actually the reverse that needs to be done. > In the makefile, a feature is responsible only to select or deselect its > configure option and, if this feature relies on a library, add a dependency on > this. So I think the .mk file should simply contain things like: ifeq ($(BR2_PACKAGE_LIBVORBIS),y) MPD_DEPENDENCIES += libvorbis MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled else MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled endif and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting BR2_PACKAGE_LIBVORBIS. > Duplicated selection logic in mpd.mk will be eliminated. A build dependency will > only be added if it is present in the mpd configuration, and not be inherited > accidentally from concurrent packages. So this is exactly the reverse of what I'd want. I think it makes the .mk file harder to maintain for no practical gain. > This impacts the following features: expat, id3tag, yajl. id3tag already is half > implemented, but not used consistently (has an option entry in Config.in, but > does not select this, but handles dependencies individually). expat and yajl > would be created as hidden options without visibility (no user interaction > necessary). Yes, that would indeed be an alternative to keep the .mk file simpler. I take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets selected by the other MPD options that rely on expat. This indeed simplifies things, but it is still a bit more complicated than what I propose. > Separate vorbis decoder and encoder options; currently the decoder feature > BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis encoding, > which is probably not intended. I don't see a reason why you would want only one or the other. It has virtually no impact on size. In summary, I think we have the following three options for packages where we decide we want user-visible sub-options. - 1-to-1 mapping with the package configure options. Interaction between the options is expressed with select/depends in Config.in. The .mk file simply maps the Config.in options. If it's really not relevant for the user, a Config.in option can be made blind. This is what Andreas proposes. - Only automatic dependencies in the .mk file, except in cases where it has an important size or behaviour impact. Add Config.in options only in case it is relevant. This is what I propose. - Add Config.in options for important features. Express interdependencies between package configure options in the .mk file. This is the current situation for mpd. Thinking more about it, Andreas' proposal does have an attractive kind of elegance about it, which gives it simplicity even though it is more lines of code. So let's see what the other maintainers think. If you (or the other maintainers) don't agree, we can compromise and go back to the original patch (with just the npupnp/expat situation resolved). Ideally I'd like the maintainers (and anybody else, really) to come to a decision here and eventually formalize it in the manual. Because it's not the first time this discussion crops up. Regards, Arnout > Create entry 'unicode' for ICU in Config.in. > > Set zlib to disabled (not used currently). > > Thoughts? > > Kind regards, > Andreas > >> Regards, >> Arnout >> >>> else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y) >>> MPD_CONF_OPTS += -Dupnp=disabled >>> endif
Arnout, Andreas, All, On 2022-04-09 17:41 +0200, Arnout Vandecappelle spake thusly: > On 08/04/2022 15:22, Andreas Ziegler wrote: > >Outline of the proposed next version of this change: > >[I did not want to make so many changes, but you are right, it makes sense.] > >Separate logic from implementation: Remove feature dependencies from > >mpd.mk and put selection logic into Config.in exclusively. > As you can see from the description above: this is making things quite > complicated. And one of the tenets of Buildroot is to keep things simple. [--SNIP--] > We don't have a consistent policy for this case, but I believe the policy > should be: > > - Add Config.in options only for features that are important, meaningful for > the user (e.g. codec support). > > - Add Config.in options only for features that have a size impact (usually > due to the dependencies they pull in). > > - In the .mk file, don't use the Config.in options but instead use automatic > dependencies only. That would be very confusign from a user perspective: they would not enable a feature of a package, yet the package would have that feature enabled if th3e depedency is enabled. Besides the technical surprise, this could also lead to licensing issues if the combination of the two packages require special handling (e.g. because the library license propagates top the package). So, the settings from Config.in must be abode by. > That way, the .mk file is kept simple (no problematic cases like the > libupnp/expat interaction in this patch). The Config.in is a bit > complicated, but it doesn't explode. > > Bottom line: I think it's actually the reverse that needs to be done. Err. I don;'t understand what you meant here... :-( > >In the makefile, a feature is responsible only to select or deselect its > >configure option and, if this feature relies on a library, add a > >dependency on this. > > So I think the .mk file should simply contain things like: > > ifeq ($(BR2_PACKAGE_LIBVORBIS),y) > MPD_DEPENDENCIES += libvorbis > MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled > else > MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled > endif > > and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting > BR2_PACKAGE_LIBVORBIS. I highly disagree; the conditional should be on the package setting, not the dependency. > >Duplicated selection logic in mpd.mk will be eliminated. A build > >dependency will only be added if it is present in the mpd configuration, > >and not be inherited accidentally from concurrent packages. > So this is exactly the reverse of what I'd want. I think it makes the .mk > file harder to maintain for no practical gain. Yet, the proposal by Andreas is I believe the correct way to handle the situation. > >This impacts the following features: expat, id3tag, yajl. id3tag already > >is half implemented, but not used consistently (has an option entry in > >Config.in, but does not select this, but handles dependencies > >individually). expat and yajl would be created as hidden options without > >visibility (no user interaction necessary). > Yes, that would indeed be an alternative to keep the .mk file simpler. I > take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets > selected by the other MPD options that rely on expat. This indeed simplifies > things, but it is still a bit more complicated than what I propose. But more correct. > >Separate vorbis decoder and encoder options; currently the decoder feature > >BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis > >encoding, which is probably not intended. > I don't see a reason why you would want only one or the other. It has > virtually no impact on size. Indeed here, vorbis support should just enable both the encoder/decoder. The only reason that we might want to be able to chose, is if enabling one or the other have different requirements: extra size requirements, extra dependencies, legal issues in your jurisdiction, etc... > In summary, I think we have the following three options for packages where > we decide we want user-visible sub-options. > > - 1-to-1 mapping with the package configure options. Interaction between the > options is expressed with select/depends in Config.in. The .mk file simply > maps the Config.in options. If it's really not relevant for the user, a > Config.in option can be made blind. This is what Andreas proposes. I am OK with that. > - Only automatic dependencies in the .mk file, except in cases where it has > an important size or behaviour impact. Add Config.in options only in case it > is relevant. This is what I propose. I am OK with the principle, but this does not look like what you proposed above, as the build-dependencies would be on the dependency being enabled, not the package option (e.g. BR2_PACKAGE_MPD_VORBIS vs. BR2_PACKAGE_LIBVORBIS as you showed above). > - Add Config.in options for important features. Express interdependencies > between package configure options in the .mk file. This is the current > situation for mpd. I don;t see a difference here: "Add Config.in options only in case it is relevant" and "Add Config.in options for important features" are exactly the same in mny eyes... > So let's see what the other maintainers think. If you (or the other > maintainers) don't agree, we can compromise and go back to the original > patch (with just the npupnp/expat situation resolved). So, I'll summariser my position: - add Config.in options when it makes sense: - important size delta - legal issues those options select the appropriate packages - in the .mk: - add conditional blocks based on those options, add build dependencies as appropriate; - add conditoinal dependencies on package for automatic dependencies > Ideally I'd like the maintainers (and anybody else, really) to come to a > decision here and eventually formalize it in the manual. Because it's not > the first time this discussion crops up. Regards, Yann E. MORIN.
Hi Yann, Arnout, and whoever else is interested, two additional thoughts from my side below: On 2022-04-09 16:09, Yann E. MORIN wrote: > Arnout, Andreas, All, > > On 2022-04-09 17:41 +0200, Arnout Vandecappelle spake thusly: >> On 08/04/2022 15:22, Andreas Ziegler wrote: >> >Outline of the proposed next version of this change: >> >[I did not want to make so many changes, but you are right, it makes sense.] >> >Separate logic from implementation: Remove feature dependencies from >> >mpd.mk and put selection logic into Config.in exclusively. >> As you can see from the description above: this is making things >> quite >> complicated. And one of the tenets of Buildroot is to keep things >> simple. > [--SNIP--] >> We don't have a consistent policy for this case, but I believe the >> policy >> should be: >> >> - Add Config.in options only for features that are important, >> meaningful for >> the user (e.g. codec support). >> >> - Add Config.in options only for features that have a size impact >> (usually >> due to the dependencies they pull in). >> >> - In the .mk file, don't use the Config.in options but instead use >> automatic >> dependencies only. > > That would be very confusign from a user perspective: they would not > enable a feature of a package, yet the package would have that feature > enabled if th3e depedency is enabled. > > Besides the technical surprise, this could also lead to licensing > issues > if the combination of the two packages require special handling (e.g. > because the library license propagates top the package). The security aspect increasingly becomes more important: every line of unused code is a potential attack vector. Better disable what you do not need. > So, the settings from Config.in must be abode by. > >> That way, the .mk file is kept simple (no problematic cases like the >> libupnp/expat interaction in this patch). The Config.in is a bit >> complicated, but it doesn't explode. >> >> Bottom line: I think it's actually the reverse that needs to be done. > > Err. I don;'t understand what you meant here... :-( > >> >In the makefile, a feature is responsible only to select or deselect its >> >configure option and, if this feature relies on a library, add a >> >dependency on this. >> >> So I think the .mk file should simply contain things like: >> >> ifeq ($(BR2_PACKAGE_LIBVORBIS),y) >> MPD_DEPENDENCIES += libvorbis >> MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled >> else >> MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled >> endif >> >> and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting >> BR2_PACKAGE_LIBVORBIS. > > I highly disagree; the conditional should be on the package setting, > not > the dependency. > >> >Duplicated selection logic in mpd.mk will be eliminated. A build >> >dependency will only be added if it is present in the mpd configuration, >> >and not be inherited accidentally from concurrent packages. >> So this is exactly the reverse of what I'd want. I think it makes the >> .mk >> file harder to maintain for no practical gain. > > Yet, the proposal by Andreas is I believe the correct way to handle the > situation. > >> >This impacts the following features: expat, id3tag, yajl. id3tag already >> >is half implemented, but not used consistently (has an option entry in >> >Config.in, but does not select this, but handles dependencies >> >individually). expat and yajl would be created as hidden options without >> >visibility (no user interaction necessary). >> Yes, that would indeed be an alternative to keep the .mk file >> simpler. I >> take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets >> selected by the other MPD options that rely on expat. This indeed >> simplifies >> things, but it is still a bit more complicated than what I propose. > > But more correct. > >> >Separate vorbis decoder and encoder options; currently the decoder feature >> >BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis >> >encoding, which is probably not intended. >> I don't see a reason why you would want only one or the other. It has >> virtually no impact on size. > > Indeed here, vorbis support should just enable both the > encoder/decoder. > > The only reason that we might want to be able to chose, is if enabling > one or the other have different requirements: extra size requirements, > extra dependencies, legal issues in your jurisdiction, etc... This behavior is not evident from the menu. Here you just have an option to enable the decoder; the encoder is not mentioned anywhere. If the configuration stays, at least grouping and help message of the current item need an update. >> In summary, I think we have the following three options for packages >> where >> we decide we want user-visible sub-options. >> >> - 1-to-1 mapping with the package configure options. Interaction >> between the >> options is expressed with select/depends in Config.in. The .mk file >> simply >> maps the Config.in options. If it's really not relevant for the user, >> a >> Config.in option can be made blind. This is what Andreas proposes. > > I am OK with that. > >> - Only automatic dependencies in the .mk file, except in cases where >> it has >> an important size or behaviour impact. Add Config.in options only in >> case it >> is relevant. This is what I propose. > > I am OK with the principle, but this does not look like what you > proposed above, as the build-dependencies would be on the dependency > being enabled, not the package option (e.g. BR2_PACKAGE_MPD_VORBIS vs. > BR2_PACKAGE_LIBVORBIS as you showed above). > >> - Add Config.in options for important features. Express >> interdependencies >> between package configure options in the .mk file. This is the current >> situation for mpd. > > I don;t see a difference here: "Add Config.in options only in case it > is > relevant" and "Add Config.in options for important features" are > exactly > the same in mny eyes... > >> So let's see what the other maintainers think. If you (or the other >> maintainers) don't agree, we can compromise and go back to the >> original >> patch (with just the npupnp/expat situation resolved). > > So, I'll summariser my position: > > - add Config.in options when it makes sense: > - important size delta > - legal issues > those options select the appropriate packages > > - in the .mk: > - add conditional blocks based on those options, add build > dependencies as appropriate; > - add conditoinal dependencies on package for automatic > dependencies > >> Ideally I'd like the maintainers (and anybody else, really) to come >> to a >> decision here and eventually formalize it in the manual. Because it's >> not >> the first time this discussion crops up. Kind regards, Andreas > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' > conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ > | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There > is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v > conspiracy. | > '------------------------------^-------^------------------^--------------------'
Hi Yann, Arnout, *, I would like to postpone this change for the moment; the root cause for the addressed build failures lies within the meson build system (patch coming) and fixing this will make some of the proposed modifications unnecessary. I would still like to implement the selection logic changes, because I started to notice some inconsistencies and also missing features. Kind regards, Andreas On 2022-04-09 16:09, Yann E. MORIN wrote: > Arnout, Andreas, All, > > On 2022-04-09 17:41 +0200, Arnout Vandecappelle spake thusly: >> On 08/04/2022 15:22, Andreas Ziegler wrote: >> >Outline of the proposed next version of this change: >> >[I did not want to make so many changes, but you are right, it makes sense.] >> >Separate logic from implementation: Remove feature dependencies from >> >mpd.mk and put selection logic into Config.in exclusively. >> As you can see from the description above: this is making things >> quite >> complicated. And one of the tenets of Buildroot is to keep things >> simple. > [--SNIP--] >> We don't have a consistent policy for this case, but I believe the >> policy >> should be: >> >> - Add Config.in options only for features that are important, >> meaningful for >> the user (e.g. codec support). >> >> - Add Config.in options only for features that have a size impact >> (usually >> due to the dependencies they pull in). >> >> - In the .mk file, don't use the Config.in options but instead use >> automatic >> dependencies only. > > That would be very confusign from a user perspective: they would not > enable a feature of a package, yet the package would have that feature > enabled if th3e depedency is enabled. > > Besides the technical surprise, this could also lead to licensing > issues > if the combination of the two packages require special handling (e.g. > because the library license propagates top the package). > > So, the settings from Config.in must be abode by. > >> That way, the .mk file is kept simple (no problematic cases like the >> libupnp/expat interaction in this patch). The Config.in is a bit >> complicated, but it doesn't explode. >> >> Bottom line: I think it's actually the reverse that needs to be done. > > Err. I don;'t understand what you meant here... :-( > >> >In the makefile, a feature is responsible only to select or deselect its >> >configure option and, if this feature relies on a library, add a >> >dependency on this. >> >> So I think the .mk file should simply contain things like: >> >> ifeq ($(BR2_PACKAGE_LIBVORBIS),y) >> MPD_DEPENDENCIES += libvorbis >> MPD_CONF_OPTS += -Dvorbis=enabled -Dvorbisenc=enabled >> else >> MPD_CONF_OPTS += -Dvorbis=disabled -Dvorbisenc=disabled >> endif >> >> and BR2_PACKAGE_MPD_VORBIS only has an indirect effect, by selecting >> BR2_PACKAGE_LIBVORBIS. > > I highly disagree; the conditional should be on the package setting, > not > the dependency. > >> >Duplicated selection logic in mpd.mk will be eliminated. A build >> >dependency will only be added if it is present in the mpd configuration, >> >and not be inherited accidentally from concurrent packages. >> So this is exactly the reverse of what I'd want. I think it makes the >> .mk >> file harder to maintain for no practical gain. > > Yet, the proposal by Andreas is I believe the correct way to handle the > situation. > >> >This impacts the following features: expat, id3tag, yajl. id3tag already >> >is half implemented, but not used consistently (has an option entry in >> >Config.in, but does not select this, but handles dependencies >> >individually). expat and yajl would be created as hidden options without >> >visibility (no user interaction necessary). >> Yes, that would indeed be an alternative to keep the .mk file >> simpler. I >> take it you mean to add a blind option BR2_PACKAGE_MPD_EXPAT that gets >> selected by the other MPD options that rely on expat. This indeed >> simplifies >> things, but it is still a bit more complicated than what I propose. > > But more correct. > >> >Separate vorbis decoder and encoder options; currently the decoder feature >> >BR2_PACKAGE_MPD_VORBIS enables both ogg/vorbis decoding and ogg/vorbis >> >encoding, which is probably not intended. >> I don't see a reason why you would want only one or the other. It has >> virtually no impact on size. > > Indeed here, vorbis support should just enable both the > encoder/decoder. > > The only reason that we might want to be able to chose, is if enabling > one or the other have different requirements: extra size requirements, > extra dependencies, legal issues in your jurisdiction, etc... > >> In summary, I think we have the following three options for packages >> where >> we decide we want user-visible sub-options. >> >> - 1-to-1 mapping with the package configure options. Interaction >> between the >> options is expressed with select/depends in Config.in. The .mk file >> simply >> maps the Config.in options. If it's really not relevant for the user, >> a >> Config.in option can be made blind. This is what Andreas proposes. > > I am OK with that. > >> - Only automatic dependencies in the .mk file, except in cases where >> it has >> an important size or behaviour impact. Add Config.in options only in >> case it >> is relevant. This is what I propose. > > I am OK with the principle, but this does not look like what you > proposed above, as the build-dependencies would be on the dependency > being enabled, not the package option (e.g. BR2_PACKAGE_MPD_VORBIS vs. > BR2_PACKAGE_LIBVORBIS as you showed above). > >> - Add Config.in options for important features. Express >> interdependencies >> between package configure options in the .mk file. This is the current >> situation for mpd. > > I don;t see a difference here: "Add Config.in options only in case it > is > relevant" and "Add Config.in options for important features" are > exactly > the same in mny eyes... > >> So let's see what the other maintainers think. If you (or the other >> maintainers) don't agree, we can compromise and go back to the >> original >> patch (with just the npupnp/expat situation resolved). > > So, I'll summariser my position: > > - add Config.in options when it makes sense: > - important size delta > - legal issues > those options select the appropriate packages > > - in the .mk: > - add conditional blocks based on those options, add build > dependencies as appropriate; > - add conditoinal dependencies on package for automatic > dependencies > >> Ideally I'd like the maintainers (and anybody else, really) to come >> to a >> decision here and eventually formalize it in the manual. Because it's >> not >> the first time this discussion crops up. > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' > conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ > | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There > is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v > conspiracy. | > '------------------------------^-------^------------------^--------------------'
diff --git a/package/mpd/mpd.mk b/package/mpd/mpd.mk index 12da36098f..4e67c9428c 100644 --- a/package/mpd/mpd.mk +++ b/package/mpd/mpd.mk @@ -14,6 +14,7 @@ MPD_LICENSE_FILES = COPYING # these refer to the FreeBSD PPP daemon MPD_IGNORE_CVES = CVE-2020-7465 CVE-2020-7466 MPD_SELINUX_MODULES = mpd +# These features are either unwanted or not selectable via the Buildroot menu MPD_CONF_OPTS = \ -Daudiofile=disabled \ -Ddocumentation=disabled \ @@ -21,6 +22,12 @@ MPD_CONF_OPTS = \ -Dpipewire=disabled \ -Dsnapcast=false +# Explicitly disable features where meson's dependency detection picks up host +# libraries. These settings can be overridden through menu options later +MPD_CONF_OPTS += \ + -Dexpat=disabled \ + -Dzlib=disabled + # Zeroconf support depends on libdns_sd from avahi. ifeq ($(BR2_PACKAGE_MPD_AVAHI_SUPPORT),y) MPD_DEPENDENCIES += avahi @@ -300,11 +307,11 @@ ifeq ($(BR2_PACKAGE_MPD_UPNP_PUPNP),y) MPD_DEPENDENCIES += \ expat \ libupnp -MPD_CONF_OPTS += -Dupnp=pupnp +MPD_CONF_OPTS += -Dupnp=pupnp -Dexpat=enabled else ifeq ($(BR2_PACKAGE_MPD_UPNP_NPUPNP),y) MPD_DEPENDENCIES += \ libnpupnp -MPD_CONF_OPTS += -Dupnp=npupnp +MPD_CONF_OPTS += -Dupnp=npupnp -Dexpat=enabled else ifeq ($(BR2_PACKAGE_MPD_UPNP_DISABLED),y) MPD_CONF_OPTS += -Dupnp=disabled endif
Background During configuration, the meson build system tries to determine the availability of optional dependencies using (host) pkgconfig and cmake in that order. If a library does not exist on the target, pkg-config will fail, but cmake sometimes finds and reports libraries that exist as host packages. This has been observed for host-expat (cmake dependency) and host-zlib. The link step subsequently fails, because necessary files are not present in the target architecture. Unconditionally disable optional features often found in host binaries and modify the menu selection processing in mpd.mk to re-enable them where necessary. Currently this concerns expat and zlib only. This fixes the following build errors: [expat] /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: warning: libc.so.6, needed by /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7, not found (try using -rpath or -rpath-link) /home/data/buildroot.x86_64/host/lib/gcc/x86_64-buildroot-linux-uclibc/11.2.0/../../../../x86_64-buildroot-linux-uclibc/bin/ld: /home/data/buildroot.x86_64/host/lib/libexpat.so.1.8.7: undefined reference to `__errno_location@GLIBC_2.2.5' [zlib] http://autobuild.buildroot.net/results/f0a/f0a9e719114f19dc9d20622ed85dd4f8e968c20f/ Signed-off-by: Andreas Ziegler <br015@umbiko.net> --- package/mpd/mpd.mk | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)