| Message ID | 20200529172238.43399-2-freifunk@adrianschmutzler.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | treewide: tidy up use of DEVICE_TYPE | expand |
On Friday, 29 May 2020 19:22:36 CEST Adrian Schmutzler wrote: > DEVICE_TYPE is a target/subtarget variable, and it does not have > any effect when set in a device definition. It can only be set > in a target's or subtarget's Makefile. > > Consequently, having it set anyway is misleading, so this drops > all cases. Well, I can tell you where it matters for devices. You'll have to look at this: <https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=include/target.mk;h=9bd4c14936c1438df2be87e5697f5f5568699d2b;hb=HEAD#l54> |# Add device specific packages (here below to allow device type set from subtarget) |DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) So, the MBL gets "DEFAULT_PACKAGES.nas" (block-mount fdisk lsblk mdadm) over "DEFAULT_PACKAGES.router" (dnsmasq iptables ip6tables ppp ppp-mod-pppoe firewall odhcpd-ipv6only odhcp6c kmod-ipt-offload) which makes much more sense for other devices as well. If you want to revert these changes and make this transparent, you'll probably want to amend each device package list with the appropriate -package (i.e -ppp -firewall -dnsmasq ...) all around. Cheers, Christian
> > Consequently, having it set anyway is misleading, so this drops all > > cases. > > Well, I can tell you where it matters for devices. > > You'll have to look at this: > > <https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=include/target. > mk;h=9bd4c14936c1438df2be87e5697f5f5568699d2b;hb=HEAD#l54> > > |# Add device specific packages (here below to allow device type set > |from subtarget) DEFAULT_PACKAGES += > $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) > > So, the MBL gets "DEFAULT_PACKAGES.nas" (block-mount fdisk lsblk > mdadm) over "DEFAULT_PACKAGES.router" (dnsmasq iptables ip6tables ppp > ppp-mod-pppoe firewall odhcpd-ipv6only odhcp6c kmod-ipt-offload) which > makes much more sense for other devices as well. Hi Christian, that's exactly my point. Since this is target.mk as far as I can tell this selection does not work when DEVICE_TYPE is set within the device definition, but only when it's set in the (sub)target Makefile. As I understand it (and tested with make menuconfig), DEFAULT_PACKAGES is a per-target variable, and thus the DEVICE_TYPE from within the device definition will never be applied, and DEFAULT_PACKAGES.router will be used anyway for these devices. Or am I completely misled here? Best Adrian > > If you want to revert these changes and make this transparent, you'll > probably want to amend each device package list with the appropriate - > package (i.e -ppp -firewall -dnsmasq ...) all around. > > Cheers, > Christian > > > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On 5/29/20 7:45 PM, mail@adrianschmutzler.de wrote: >>> Consequently, having it set anyway is misleading, so this drops all >>> cases. >> >> Well, I can tell you where it matters for devices. >> >> You'll have to look at this: >> >> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=include/target. >> mk;h=9bd4c14936c1438df2be87e5697f5f5568699d2b;hb=HEAD#l54> >> >> |# Add device specific packages (here below to allow device type set >> |from subtarget) DEFAULT_PACKAGES += >> $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) >> >> So, the MBL gets "DEFAULT_PACKAGES.nas" (block-mount fdisk lsblk >> mdadm) over "DEFAULT_PACKAGES.router" (dnsmasq iptables ip6tables ppp >> ppp-mod-pppoe firewall odhcpd-ipv6only odhcp6c kmod-ipt-offload) which >> makes much more sense for other devices as well. > > Hi Christian, > > that's exactly my point. Since this is target.mk as far as I can tell this selection does not work when DEVICE_TYPE is set within the device definition, but only when it's set in the (sub)target Makefile. As I understand it (and tested with make menuconfig), DEFAULT_PACKAGES is a per-target variable, and thus the DEVICE_TYPE from within the device definition will never be applied, and DEFAULT_PACKAGES.router will be used anyway for these devices. > > Or am I completely misled here? I believe you are right, it seems DEVICE_TYPE is not evaluated when set in a device definition. Matthias > > Best > > Adrian > >> >> If you want to revert these changes and make this transparent, you'll >> probably want to amend each device package list with the appropriate - >> package (i.e -ppp -firewall -dnsmasq ...) all around. >> >> Cheers, >> Christian >> >> >> >> >> _______________________________________________ >> openwrt-devel mailing list >> openwrt-devel@lists.openwrt.org >> https://lists.openwrt.org/mailman/listinfo/openwrt-devel >> >> _______________________________________________ >> openwrt-devel mailing list >> openwrt-devel@lists.openwrt.org >> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Friday, 29 May 2020 21:32:59 CEST Matthias Schiffer wrote: > On 5/29/20 7:45 PM, mail@adrianschmutzler.de wrote: > >>> Consequently, having it set anyway is misleading, so this drops all > >>> cases. > >> > >> Well, I can tell you where it matters for devices. > >> > >> You'll have to look at this: > >> > >> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=include/target. > >> mk;h=9bd4c14936c1438df2be87e5697f5f5568699d2b;hb=HEAD#l54> > >> > >> |# Add device specific packages (here below to allow device type set > >> |from subtarget) DEFAULT_PACKAGES += > >> $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) > >> > >> So, the MBL gets "DEFAULT_PACKAGES.nas" (block-mount fdisk lsblk > >> mdadm) over "DEFAULT_PACKAGES.router" (dnsmasq iptables ip6tables ppp > >> ppp-mod-pppoe firewall odhcpd-ipv6only odhcp6c kmod-ipt-offload) which > >> makes much more sense for other devices as well. > > > > Hi Christian, > > > > that's exactly my point. Since this is target.mk as far as I can tell this selection does not work when DEVICE_TYPE is set within the device definition, but only when it's set in the (sub)target Makefile. As I understand it (and tested with make menuconfig), DEFAULT_PACKAGES is a per-target variable, and thus the DEVICE_TYPE from within the device definition will never be applied, and DEFAULT_PACKAGES.router will be used anyway for these devices. > > > > Or am I completely misled here? > > I believe you are right, it seems DEVICE_TYPE is not evaluated when set in > a device definition. True, question is then, should this really be called "DEVICE"_TYPE? It's not like other DEVICE_* variables (DEVICE_NAME, DEVICE_PACKAGES or DEVICE_DTS). Because the "targets" of ath79/ipq40xx/etc... wouldn't work if they all had to share a single, common DEVICE_NAME/_DTS/_PACKAGE. As for the MBLs, if I got this all correctly, that DEVICE_TYPE could be simply moved to the apm821xx/sata target.mk --- --- a/target/linux/apm821xx/image/Makefile +++ b/target/linux/apm821xx/image/Makefile @@ -251,7 +251,6 @@ define Device/wd_mybooklive DEVICE_VENDOR := Western Digital DEVICE_MODEL := My Book Live Series (Single + Duo) DEVICE_PACKAGES := kmod-usb-dwc2 kmod-usb-ledtrig-usbport kmod-usb-storage kmod-fs-vfat wpad-basic - DEVICE_TYPE := nas DEVICE_DTS := wd-mybooklive SUPPORTED_DEVICES += mbl wd,mybooklive-duo BLOCKSIZE := 1k --- a/target/linux/apm821xx/sata/target.mk +++ b/target/linux/apm821xx/sata/target.mk @@ -1,5 +1,6 @@ BOARDNAME := Devices which boot from SATA (NAS) FEATURES += ext4 usb ramdisk squashfs rootfs-part boot-part +DEVICE_TYPE := nas DEFAULT_PACKAGES += badblocks block-mount e2fsprogs kmod-hwmon-drivetemp \ kmod-dm kmod-md-mod partx-utils mkf2fs f2fsck --- And it would work as expected, right? Cheers, Christian
> > > Or am I completely misled here? > > > > I believe you are right, it seems DEVICE_TYPE is not evaluated when > > set in a device definition. > True, question is then, should this really be called "DEVICE"_TYPE? > It's not like other DEVICE_* variables (DEVICE_NAME, DEVICE_PACKAGES or > DEVICE_DTS). > Because the "targets" of ath79/ipq40xx/etc... wouldn't work if they all had to > share a single, common DEVICE_NAME/_DTS/_PACKAGE. > > As for the MBLs, if I got this all correctly, that DEVICE_TYPE could be simply > moved to the apm821xx/sata target.mk > --- > --- a/target/linux/apm821xx/image/Makefile > +++ b/target/linux/apm821xx/image/Makefile > @@ -251,7 +251,6 @@ define Device/wd_mybooklive > DEVICE_VENDOR := Western Digital > DEVICE_MODEL := My Book Live Series (Single + Duo) > DEVICE_PACKAGES := kmod-usb-dwc2 kmod-usb-ledtrig-usbport kmod- > usb-storage kmod-fs-vfat wpad-basic > - DEVICE_TYPE := nas > DEVICE_DTS := wd-mybooklive > SUPPORTED_DEVICES += mbl wd,mybooklive-duo > BLOCKSIZE := 1k > --- a/target/linux/apm821xx/sata/target.mk > +++ b/target/linux/apm821xx/sata/target.mk > @@ -1,5 +1,6 @@ > BOARDNAME := Devices which boot from SATA (NAS) FEATURES += ext4 > usb ramdisk squashfs rootfs-part boot-part > +DEVICE_TYPE := nas > DEFAULT_PACKAGES += badblocks block-mount e2fsprogs kmod-hwmon- > drivetemp \ > kmod-dm kmod-md-mod partx-utils mkf2fs f2fsck > > --- > And it would work as expected, right? > > Cheers, > Christian > Yes, in this case this would work as expected after change. Of course, this assumes that future additions to the subtarget would be "NAS-devices" as well. > True, question is then, should this really be called "DEVICE"_TYPE? That's exactly the question we are discussing in 3/3 of this series which went to the openwrt-devel, I only Cc-ed you for the 1/3. I personally tend towards dropping DEVICE_TYPE entirely and separate the selection of different subsets for DEFAULT_PACKAGES from providing a config option for packages like busybox. I will convert my patch so your solution proposed here will be included. Best Adrian
On Friday, 29 May 2020 22:32:10 CEST mail@adrianschmutzler.de wrote: > > > > Or am I completely misled here? > > > > > > I believe you are right, it seems DEVICE_TYPE is not evaluated when > > > set in a device definition. > > True, question is then, should this really be called "DEVICE"_TYPE? > > It's not like other DEVICE_* variables (DEVICE_NAME, DEVICE_PACKAGES or > > DEVICE_DTS). > > Because the "targets" of ath79/ipq40xx/etc... wouldn't work if they all had to > > share a single, common DEVICE_NAME/_DTS/_PACKAGE. > > > > As for the MBLs, if I got this all correctly, that DEVICE_TYPE could be simply > > moved to the apm821xx/sata target.mk > > --- > > --- a/target/linux/apm821xx/image/Makefile > > +++ b/target/linux/apm821xx/image/Makefile > > @@ -251,7 +251,6 @@ define Device/wd_mybooklive > > DEVICE_VENDOR := Western Digital > > DEVICE_MODEL := My Book Live Series (Single + Duo) > > DEVICE_PACKAGES := kmod-usb-dwc2 kmod-usb-ledtrig-usbport kmod- > > usb-storage kmod-fs-vfat wpad-basic > > - DEVICE_TYPE := nas > > DEVICE_DTS := wd-mybooklive > > SUPPORTED_DEVICES += mbl wd,mybooklive-duo > > BLOCKSIZE := 1k > > --- a/target/linux/apm821xx/sata/target.mk > > +++ b/target/linux/apm821xx/sata/target.mk > > @@ -1,5 +1,6 @@ > > BOARDNAME := Devices which boot from SATA (NAS) FEATURES += ext4 > > usb ramdisk squashfs rootfs-part boot-part > > +DEVICE_TYPE := nas > > DEFAULT_PACKAGES += badblocks block-mount e2fsprogs kmod-hwmon- > > drivetemp \ > > kmod-dm kmod-md-mod partx-utils mkf2fs f2fsck > > > > --- > > And it would work as expected, right? > > > > Cheers, > > Christian > > > > Yes, in this case this would work as expected after change. Of course, this assumes that future additions to the subtarget would be "NAS-devices" as well. > > > True, question is then, should this really be called "DEVICE"_TYPE? > > That's exactly the question we are discussing in 3/3 of this series which went to the openwrt-devel, I only Cc-ed you for the 1/3. Ah ok, that might explain more than you think. I got hit by a bug interaction with debian's cyrus-imap in my mail setup: Well, thankfully it's fixed now: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=960558>. I know that I got sliently dropped from various MLs, since the mailsystem generated a ton of bounces all at once. Sorry! > I personally tend towards dropping DEVICE_TYPE entirely and separate the > selection of different subsets for DEFAULT_PACKAGES from providing a config > option for packages like busybox. Ok, I guess it's time to say farewell to DEVICE_TYPE then... The nice thing about DEVICE_TYPE was that it automatically included a bunch of generally useful packages. On the other hand, if these would all become default, then there will be more "-$package" showing up in DEVICE_PACKAGES variables and possibly heated commit wars about whenever a package should be included or dropped (but that's already happening right now too). I liked the idea of the DEVICE_TYPE variable though. But yes, it doesn't really work the way its named. For this to have any merrit, DEVICE_TYPE would need to stop meddling with DEFAULT_PACKAGES and add the selected packages with something like a second "DEVICE_PACKAGES"... and hope that TARGET_PER_DEVICE_ROOTFS can enforce the barriers between the devices (well, sadly it can't do that 100%). Cheers, Christian
diff --git a/target/linux/apm821xx/image/Makefile b/target/linux/apm821xx/image/Makefile index d732141c8f..c26c015751 100644 --- a/target/linux/apm821xx/image/Makefile +++ b/target/linux/apm821xx/image/Makefile @@ -230,7 +230,6 @@ define Device/wd_mybooklive DEVICE_VENDOR := Western Digital DEVICE_MODEL := My Book Live Series (Single + Duo) DEVICE_PACKAGES := kmod-usb-dwc2 kmod-usb-ledtrig-usbport kmod-usb-storage kmod-fs-vfat wpad-basic - DEVICE_TYPE := nas DEVICE_DTS := wd-mybooklive SUPPORTED_DEVICES += mbl wd,mybooklive-duo BLOCKSIZE := 1k diff --git a/target/linux/gemini/image/Makefile b/target/linux/gemini/image/Makefile index a155939b8c..83f3d222d9 100644 --- a/target/linux/gemini/image/Makefile +++ b/target/linux/gemini/image/Makefile @@ -171,7 +171,6 @@ define Device/dlink_dns-313 DEVICE_VENDOR := D-Link DEVICE_MODEL := DNS-313 1-Bay Network Storage Enclosure DEVICE_DTS := gemini-dlink-dns-313 - DEVICE_TYPE := nas DEVICE_PACKAGES := $(GEMINI_NAS_PACKAGES) BLOCKSIZE := 1k FILESYSTEMS := ext4 @@ -204,7 +203,6 @@ define Device/raidsonic_ib-4220-b DEVICE_VENDOR := Raidsonic DEVICE_MODEL := NAS IB-4220-B DEVICE_DTS := gemini-nas4220b - DEVICE_TYPE := nas endef TARGET_DEVICES += raidsonic_ib-4220-b diff --git a/target/linux/kirkwood/image/Makefile b/target/linux/kirkwood/image/Makefile index e69e3f125d..552ee44ee4 100644 --- a/target/linux/kirkwood/image/Makefile +++ b/target/linux/kirkwood/image/Makefile @@ -66,7 +66,6 @@ define Device/iom_ix2-200 DEVICE_MODEL := StorCenter ix2-200 DEVICE_DTS := kirkwood-iomega_ix2_200 DEVICE_PACKAGES := kmod-gpio-button-hotplug kmod-hwmon-lm63 - DEVICE_TYPE:=nas PAGESIZE := 512 SUBPAGESIZE := 256 BLOCKSIZE := 16k
DEVICE_TYPE is a target/subtarget variable, and it does not have any effect when set in a device definition. It can only be set in a target's or subtarget's Makefile. Consequently, having it set anyway is misleading, so this drops all cases. This effectively reverts the following commits: 7a1497fd601d ("apm821xx: MBL: set DEVICE_TYPE to NAS") 5b4765c93a1b ("gemini: Classify Raidsonic NAS IB-4220-B as a NAS") cdc6de460bb4 ("gemini: D-Link DNS-313 is a NAS") For the following commit, the variable was set when adding device support: 27b2f0fc0fc5 ("kirkwood: add support for Iomega Storcenter ix2-200") Cc: Christian Lamparter <chunkeey@gmail.com> Cc: Sungbo Eo <mans0n@gorani.run> Cc: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de> --- target/linux/apm821xx/image/Makefile | 1 - target/linux/gemini/image/Makefile | 2 -- target/linux/kirkwood/image/Makefile | 1 - 3 files changed, 4 deletions(-)