| Message ID | 20200324233343.27749-1-pepe2k@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Piotr Dymacz |
| Headers | show |
| Series | [OpenWrt-Devel] target.mk: enable iwinfo by default with any wpad variant | expand |
Hi Piotr, > -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Piotr Dymacz > Sent: Mittwoch, 25. März 2020 00:34 > To: openwrt-devel@lists.openwrt.org > Subject: [OpenWrt-Devel] [PATCH] target.mk: enable iwinfo by default with > any wpad variant > > There are currently 7 variants of 'wpad' package but 'iwinfo' is included by > default only if 'wpad', 'wpad-{basic,mini}' or 'nas' > packages are included in {DEVICE,DEFAULT}_PACKAGES. Use 'wpad-*' > pattern to include 'iwinfo' with any 'wpad' variant. > > Signed-off-by: Piotr Dymacz <pepe2k@gmail.com> > --- > include/target.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/target.mk b/include/target.mk index > 9bd4c14936..004db1f45b 100644 > --- a/include/target.mk > +++ b/include/target.mk > @@ -55,7 +55,7 @@ endif > DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) > > filter_packages = $(filter-out -% $(patsubst -%,%,$(filter -%,$(1))),$(1)) - > extra_packages = $(if $(filter wpad-mini wpad-basic wpad nas,$(1)),iwinfo) > +extra_packages = $(if $(filter wpad wpad-% nas,$(1)),iwinfo) Since you are touching this, maybe you can elaborate why this extra_packages construct is needed at all? Why can't we just add iwinfo as selective dependency to the wpad-/nas packages as we do for all of the other packages? I'm asking because I recently had a downstream case where we use hostapd instead of wpad and wanted to get of iwinfo. I expected iwinfo to go away because nothing selected it anymore, but in this case it turned out that iwinfo is not automatically deselected, but has to be removed manually as well. Best Adrian > > define ProfileDefault > NAME:= > -- > 2.20.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Hi Adrian, On 25.03.2020 12:21, mail@adrianschmutzler.de wrote: > Hi Piotr, > >> -----Original Message----- >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] >> On Behalf Of Piotr Dymacz >> Sent: Mittwoch, 25. März 2020 00:34 >> To: openwrt-devel@lists.openwrt.org >> Subject: [OpenWrt-Devel] [PATCH] target.mk: enable iwinfo by default with >> any wpad variant [...] > Since you are touching this, maybe you can elaborate why this > extra_packages construct is needed at all? I don't know exact reason and it was added long time ago, in 2014, see: 6435b8bb27e. I suppose that was the easy way to handle it back then? > Why can't we just add iwinfo as selective dependency to the wpad-/nas > packages as we do for all of the other packages? Could you explain what you mean by 'selective dependency'? I don't think DEPENDS is correct way to handle this. Do you mean use 'select iwinfo' in wpad/nas packages config? > I'm asking because I recently had a downstream case where we use > hostapd instead of wpad and wanted to get of iwinfo. I expected > iwinfo to go away because nothing selected it anymore, but in this > case it turned out that iwinfo is not automatically deselected, but > has to be removed manually as well. I suppose you should first look at DEFAULT_PACKAGES and target.mk. In most cases, wpad-* is added there and that's how iwinfo gets selected. But I see the problem here, I'm just not sure if it's safe to change current approach. Maybe Felix or Jo are able to explain reason for using extra_packages.
> -----Original Message----- > From: Piotr Dymacz [mailto:pepe2k@gmail.com] > Sent: Mittwoch, 25. März 2020 15:08 > To: mail@adrianschmutzler.de; openwrt-devel@lists.openwrt.org > Cc: Felix Fietkau <nbd@nbd.name>; Jo-Philipp Wich <jo@mein.io> > Subject: Re: [OpenWrt-Devel] [PATCH] target.mk: enable iwinfo by default > with any wpad variant > > Hi Adrian, > > On 25.03.2020 12:21, mail@adrianschmutzler.de wrote: > > Hi Piotr, > > > >> -----Original Message----- > >> From: openwrt-devel [mailto:openwrt-devel- > bounces@lists.openwrt.org] > >> On Behalf Of Piotr Dymacz > >> Sent: Mittwoch, 25. März 2020 00:34 > >> To: openwrt-devel@lists.openwrt.org > >> Subject: [OpenWrt-Devel] [PATCH] target.mk: enable iwinfo by default > >> with any wpad variant > > [...] > > > Since you are touching this, maybe you can elaborate why this > > extra_packages construct is needed at all? > > I don't know exact reason and it was added long time ago, in 2014, see: > 6435b8bb27e. I suppose that was the easy way to handle it back then? > > > Why can't we just add iwinfo as selective dependency to the wpad-/nas > > packages as we do for all of the other packages? > > Could you explain what you mean by 'selective dependency'? > > I don't think DEPENDS is correct way to handle this. Do you mean use 'select > iwinfo' in wpad/nas packages config? From the point of _functionality_, it would just be a DEPENDS := +iwinfo for the relevant packages. However, I'm aware that this is not correct from a _conceptional_ point of view, since iwinfo is not a "dependency". > > > I'm asking because I recently had a downstream case where we use > > hostapd instead of wpad and wanted to get of iwinfo. I expected iwinfo > > to go away because nothing selected it anymore, but in this case it > > turned out that iwinfo is not automatically deselected, but has to be > > removed manually as well. > > I suppose you should first look at DEFAULT_PACKAGES and target.mk. > In most cases, wpad-* is added there and that's how iwinfo gets selected. For tiny devices, we replace wpad-mini by hostapd-mini (ar71xx/ath79): CONFIG_PACKAGE_hostapd-mini=y CONFIG_PACKAGE_wpad-mini=m However, this will not unselect iwinfo (as it should theoretically according to the extra_packages condition), which will still be at "y", so will be built into the images. This can also be observed in make menuconfig, where deselecting any wpad packages does not affect the status of iwinfo. I assume that the extra-packages condition is just evaluated "too early", and not updated during evaluation in make menuconfig like "real" dependencies. Of course, removing iwinfo/libiwinfo from images can easily be achieved by CONFIG_PACKAGE_iwinfo=m CONFIG_PACKAGE_libiwinfo=m It's just not a "live-update" like we are used to from DEPENDS. From looking at the dependencies as described in the Wiki [1], I do not think there really is an alternate solution available at the moment, though. Somebody would have to build one, something like @TARGET_ath79, but not for visibility, but for selection. So, somebody could just add the new "dependency" (using § as symbol for the example) to the iwinfo package instead of using the extra_packages construct: DEPENDS += §wpad-mini §wpad-basic §nas ... However, I'm not sure whether implementing that would really be worth it, considering that disabling extra_packages manually is still quite easy to do. Best Adrian [1] https://openwrt.org/docs/guide-developer/packages#dependency_types > > But I see the problem here, I'm just not sure if it's safe to change current > approach. Maybe Felix or Jo are able to explain reason for using > extra_packages. > > -- > Cheers, > Piotr > > > > > Best > > > > Adrian > > > >> > >> define ProfileDefault > >> NAME:= > >> -- > >> 2.20.1 > >> > >> > >> _______________________________________________ > >> openwrt-devel mailing list > >> openwrt-devel@lists.openwrt.org > >> https://lists.openwrt.org/mailman/listinfo/openwrt-devel > >
Hi Adrian, On 25.03.2020 16:53, mail@adrianschmutzler.de wrote: >> -----Original Message----- >> From: Piotr Dymacz [mailto:pepe2k@gmail.com] >> Sent: Mittwoch, 25. März 2020 15:08 >> To: mail@adrianschmutzler.de; openwrt-devel@lists.openwrt.org >> Cc: Felix Fietkau <nbd@nbd.name>; Jo-Philipp Wich <jo@mein.io> >> Subject: Re: [OpenWrt-Devel] [PATCH] target.mk: enable iwinfo by default >> with any wpad variant >> >> Hi Adrian, >> >> On 25.03.2020 12:21, mail@adrianschmutzler.de wrote: >> > Hi Piotr, >> > >> >> -----Original Message----- >> >> From: openwrt-devel [mailto:openwrt-devel- >> bounces@lists.openwrt.org] >> >> On Behalf Of Piotr Dymacz >> >> Sent: Mittwoch, 25. März 2020 00:34 >> >> To: openwrt-devel@lists.openwrt.org >> >> Subject: [OpenWrt-Devel] [PATCH] target.mk: enable iwinfo by default >> >> with any wpad variant >> >> [...] >> >> > Since you are touching this, maybe you can elaborate why this >> > extra_packages construct is needed at all? >> >> I don't know exact reason and it was added long time ago, in 2014, see: >> 6435b8bb27e. I suppose that was the easy way to handle it back then? >> >> > Why can't we just add iwinfo as selective dependency to the wpad-/nas >> > packages as we do for all of the other packages? >> >> Could you explain what you mean by 'selective dependency'? >> >> I don't think DEPENDS is correct way to handle this. Do you mean use 'select >> iwinfo' in wpad/nas packages config? > > From the point of _functionality_, it would just be a > DEPENDS := +iwinfo > for the relevant packages. > > However, I'm aware that this is not correct from a _conceptional_ point of view, since iwinfo is not a "dependency". Exactly, plus this has also run-time consequences. >> > I'm asking because I recently had a downstream case where we use >> > hostapd instead of wpad and wanted to get of iwinfo. I expected iwinfo >> > to go away because nothing selected it anymore, but in this case it >> > turned out that iwinfo is not automatically deselected, but has to be >> > removed manually as well. >> >> I suppose you should first look at DEFAULT_PACKAGES and target.mk. >> In most cases, wpad-* is added there and that's how iwinfo gets selected. > > For tiny devices, we replace wpad-mini by hostapd-mini (ar71xx/ath79): > > CONFIG_PACKAGE_hostapd-mini=y > CONFIG_PACKAGE_wpad-mini=m > > However, this will not unselect iwinfo (as it should theoretically according to the extra_packages condition), which will still be at "y", so will be built into the images. > > This can also be observed in make menuconfig, where deselecting any wpad packages does not affect the status of iwinfo. I assume that the extra-packages condition is just evaluated "too early", and not updated during evaluation in make menuconfig like "real" dependencies. > > Of course, removing iwinfo/libiwinfo from images can easily be achieved by > > CONFIG_PACKAGE_iwinfo=m > CONFIG_PACKAGE_libiwinfo=m > > It's just not a "live-update" like we are used to from DEPENDS. If you are using custom config files for building, this is the correct way to handle it. > From looking at the dependencies as described in the Wiki [1], I do not think there really is an alternate solution available at the moment, though. Are you sure (warn: haven't tried that)? https://openwrt.org/docs/guide-developer/dependencies#special_notes > Somebody would have to build one, something like @TARGET_ath79, but not for visibility, but for selection. So, somebody could just add the new "dependency" (using § as symbol for the example) to the iwinfo package instead of using the extra_packages construct: > > DEPENDS += §wpad-mini §wpad-basic §nas ... > However, I'm not sure whether implementing that would really be worth it, considering that disabling extra_packages manually is still quite easy to do. No. This is not a dependency so DEPENDS is not the correct way to handle it.
diff --git a/include/target.mk b/include/target.mk index 9bd4c14936..004db1f45b 100644 --- a/include/target.mk +++ b/include/target.mk @@ -55,7 +55,7 @@ endif DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) filter_packages = $(filter-out -% $(patsubst -%,%,$(filter -%,$(1))),$(1)) -extra_packages = $(if $(filter wpad-mini wpad-basic wpad nas,$(1)),iwinfo) +extra_packages = $(if $(filter wpad wpad-% nas,$(1)),iwinfo) define ProfileDefault NAME:=
There are currently 7 variants of 'wpad' package but 'iwinfo' is included by default only if 'wpad', 'wpad-{basic,mini}' or 'nas' packages are included in {DEVICE,DEFAULT}_PACKAGES. Use 'wpad-*' pattern to include 'iwinfo' with any 'wpad' variant. Signed-off-by: Piotr Dymacz <pepe2k@gmail.com> --- include/target.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)