| Message ID | 20200529172238.43399-4-freifunk@adrianschmutzler.de |
|---|---|
| State | Rejected |
| Delegated to: | Adrian Schmutzler |
| Headers | show |
| Series | treewide: tidy up use of DEVICE_TYPE | expand |
On 5/29/20 7:22 PM, Adrian Schmutzler wrote: > The prefix "DEVICE_" for Make variables is only used for per-device > variables with the only exception of DEVICE_TYPE. This is misleading > as it leads people to incorrectly assume it can be set per device like > all the other DEVICE_* variables, as has been observed in the past. > > This renames this (rarely used) variable to clearly indicate that > it's not a device-dependent variable, and stays in line with the > DEFAULT_PACKAGES variable. > > Note that there is also a (single) package in the packages feed that > needs to be updated. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de> > --- Default type of what? IMO "DEFAULT_TYPE" is significantly worse than "DEVICE_TYPE": "DEVICE_TYPE" may be slightly misleading, but at least it's somehow conveying the information that we're talking about types of devices. The part "DEFAULT" is also misleading in a different way, as I would expect to be able to override a default value (for example in a device definiton). The variable is used rarely enough that we could well make this a bit more verbose. "TARGET_DEVICE_TYPE"? If it weren't for the busybox config change (which seems hacky to me at best*), we could also go with something like TARGET_PACKAGEGROUP, as the package selection would be the only thing controlled by this variable... [*] Busybox is missing the "nonshared" flag, so opkg updates of busybox would gain or lose the CONFIG_HDPARM setting, depending one the target used to build the busybox package for a given architecture... Matthias > include/target.mk | 4 ++-- > package/utils/busybox/Makefile | 2 +- > target/linux/arc770/Makefile | 2 +- > target/linux/archs38/Makefile | 2 +- > target/linux/oxnas/Makefile | 2 +- > 5 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/target.mk b/include/target.mk > index a2ceb7f783..8374de2ebd 100644 > --- a/include/target.mk > +++ b/include/target.mk > @@ -10,7 +10,7 @@ ifneq ($(__target_inc),1) > __target_inc=1 > > # default device type > -DEVICE_TYPE?=router > +DEFAULT_TYPE?=router > > # Default packages - the really basic set > DEFAULT_PACKAGES:=base-files libc libgcc busybox dropbear mtd uci opkg netifd fstools uclient-fetch logd urandom-seed urngd > @@ -53,7 +53,7 @@ else > endif > > # Add device specific packages (here below to allow device type set from subtarget) > -DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) > +DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEFAULT_TYPE)) > > filter_packages = $(filter-out -% $(patsubst -%,%,$(filter -%,$(1))),$(1)) > extra_packages = $(if $(filter wpad-mini wpad-basic wpad nas,$(1)),iwinfo) > diff --git a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile > index 01441d1e87..81dde74d0b 100644 > --- a/package/utils/busybox/Makefile > +++ b/package/utils/busybox/Makefile > @@ -94,7 +94,7 @@ endif > define Build/Configure > rm -f $(PKG_BUILD_DIR)/.config > touch $(PKG_BUILD_DIR)/.config > -ifeq ($(DEVICE_TYPE),nas) > +ifeq ($(DEFAULT_TYPE),nas) > echo "CONFIG_HDPARM=y" >> $(PKG_BUILD_DIR)/.config > endif > grep 'CONFIG_BUSYBOX_$(BUSYBOX_SYM)' $(TOPDIR)/.config | sed -e "s,\\(# \)\\?CONFIG_BUSYBOX_$(BUSYBOX_SYM)_\\(.*\\),\\1CONFIG_\\2,g" >> $(PKG_BUILD_DIR)/.config > diff --git a/target/linux/arc770/Makefile b/target/linux/arc770/Makefile > index 018d6e5448..d1f3e2dc82 100644 > --- a/target/linux/arc770/Makefile > +++ b/target/linux/arc770/Makefile > @@ -13,7 +13,7 @@ SUBTARGETS:=generic > > KERNEL_PATCHVER:=4.14 > > -DEVICE_TYPE:=basic > +DEFAULT_TYPE:=basic > > define Target/Description > Synopsys DesignWare boards > diff --git a/target/linux/archs38/Makefile b/target/linux/archs38/Makefile > index 5b3650ef8d..891583b2d2 100644 > --- a/target/linux/archs38/Makefile > +++ b/target/linux/archs38/Makefile > @@ -14,7 +14,7 @@ SUBTARGETS:=generic > > KERNEL_PATCHVER:=5.4 > > -DEVICE_TYPE:=basic > +DEFAULT_TYPE:=basic > > define Target/Description > Synopsys DesignWare boards > diff --git a/target/linux/oxnas/Makefile b/target/linux/oxnas/Makefile > index 750eddbcbb..10d05e914f 100644 > --- a/target/linux/oxnas/Makefile > +++ b/target/linux/oxnas/Makefile > @@ -5,7 +5,7 @@ BOARD:=oxnas > BOARDNAME:=PLXTECH/Oxford NAS782x/OX8xx > SUBTARGETS:=ox810se ox820 > FEATURES:=gpio ramdisk rtc squashfs > -DEVICE_TYPE:=nas > +DEFAULT_TYPE:=nas > > KERNEL_PATCHVER:=5.4 > >
> > The prefix "DEVICE_" for Make variables is only used for per-device > > variables with the only exception of DEVICE_TYPE. This is misleading > > as it leads people to incorrectly assume it can be set per device like > > all the other DEVICE_* variables, as has been observed in the past. > > > > This renames this (rarely used) variable to clearly indicate that > > it's not a device-dependent variable, and stays in line with the > > DEFAULT_PACKAGES variable. > > > > Note that there is also a (single) package in the packages feed that > > needs to be updated. > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de> > > --- > Default type of what? IMO "DEFAULT_TYPE" is significantly worse than > "DEVICE_TYPE": "DEVICE_TYPE" may be slightly misleading, but at least it's > somehow conveying the information that we're talking about types of > devices. The part "DEFAULT" is also misleading in a different way, as I > would expect to be able to override a default value (for example in a > device definiton). > The variable is used rarely enough that we could well make this a bit more > verbose. "TARGET_DEVICE_TYPE"? If it weren't for the busybox config change TARGET_DEVICE_TYPE also was my "second-best" idea, and in contrast to our recent similar discussion I can well live with this alternative :-) > (which seems hacky to me at best*), we could also go with something like > TARGET_PACKAGEGROUP, as the package selection would be the only thing > controlled by this variable... After all, I personally think that this DEVICE_TYPE/TARGET_DEVICE_TYPE variable should be removed entirely. What remains would be the concept of different predefined sets of DEFAULT_PACKAGES, which could be indeed solved by something like TARGET_PACKAGEGROUP := nas Or we just drop the variable at all, and do DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router at the beginning (!) of target.mk, so targets (effectively just 3 of them) can just overwrite it with DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.nas directly in the few cases where that is necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names then). The switch of busybox could then be implemented separately, which would most probably follow a simpler attempt of Linus' separate patch or something completely different ... If I understood Linus' commit message correctly, the current solution there doesn't work properly anyway. Personally, I'd prefer one of the latter options, with DEFAULT_PACKAGES selection separated from any config symbols. Best Adrian
> Or we just drop the variable at all, and do > DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router > at the beginning (!) of target.mk, so targets (effectively just 3 of them) can just overwrite it with > DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.nas > directly in the few cases where that is necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names then). I've pushed a quick draft of this approach here: https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=refs/heads/devicetypedrop Only the most topmost patch is relevant. From "make menuconfig" it seems to work as expected. The if/else in busybox is not considered in this patch.
On 5/29/20 10:52 PM, mail@adrianschmutzler.de wrote: >> Or we just drop the variable at all, and do >> DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router >> at the beginning (!) of target.mk, so targets (effectively just 3 of them) can just overwrite it with >> DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.nas >> directly in the few cases where that is necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names then). > > I've pushed a quick draft of this approach here: > > https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=refs/heads/devicetypedrop > > Only the most topmost patch is relevant. From "make menuconfig" it seems to work as expected. I would prefer to find a solution that doesn't require adding $(DEFAULT_PACKAGES_BASIC) to the other default package lists. I'll have to ponder over this a bit more. Posting the patch - possibly marked as [RFC] - would make discussing this easier. > > The if/else in busybox is not considered in this patch. > Meanwhile I've found another target-specific config setting in the busybox package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx only. I assume "truncate" is tiny enough that it doesn't really justify making busybox non-shared, we could just build in truncate unconditionally. I don't know how contrained some of the "nas" targets are, but maybe we should just replace the busybox hack with a full-featured hdparm on these targets? Matthias
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. > On May 29, 2020, at 5:12 PM, Matthias Schiffer <mschiffer@universe-factory.net> wrote: > > Meanwhile I've found another target-specific config setting in the busybox > package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx only. > > I assume "truncate" is tiny enough that it doesn't really justify making > busybox non-shared, we could just build in truncate unconditionally. I > don't know how contrained some of the "nas" targets are, but maybe we > should just replace the busybox hack with a full-featured hdparm on these > targets? > > Matthias Two of NAS type targets that want hdparm are the kirkwood devices and oxnas devices. Most of the kirkwood and oxnas devices have at least 128MB NAND flash, those that don’t usually use the disk drive for storage. Ray
Hi Adrian, Matthias, I was preparing my own patch for converting DEVICE_TYPE to a device-specific variable. https://github.com/mans0n/openwrt/commit/4d41dd963ae8d595ef38ea0a38ea08abdac1415d But I stumbled on some blockers so I left it behind... One of the blockers was the busybox hdparm. I'd also found that DEVICE_TYPE in the busybox Makefile does not work as intended, thanks to Linus for dealing with this. > On 5/29/20 10:52 PM, mail at adrianschmutzler.de wrote: >>> Or we just drop the variable at all, and do >>> DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router >>> at the beginning (!) of target.mk, so targets (effectively just 3 of them) can just overwrite it with >>> DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.nas >>> directly in the few cases where that is necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names then). >> >> I've pushed a quick draft of this approach here: >> >> https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=refs/heads/devicetypedrop >> >> Only the most topmost patch is relevant. From "make menuconfig" it seems to work as expected. > > I would prefer to find a solution that doesn't require adding > $(DEFAULT_PACKAGES_BASIC) to the other default package lists. I'll have to > ponder over this a bit more. Posting the patch - possibly marked as [RFC] - > would make discussing this easier. > > >> >> The if/else in busybox is not considered in this patch. >> > > Meanwhile I've found another target-specific config setting in the busybox > package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx only. > > I assume "truncate" is tiny enough that it doesn't really justify making > busybox non-shared, we could just build in truncate unconditionally. I > don't know how contrained some of the "nas" targets are, but maybe we > should just replace the busybox hack with a full-featured hdparm on these > targets? Busybox hdparm is about 8k and full hdparm is about 93k. I think most NAS devices can manage that space, so I agree with Matthias. But the problem is that full hdparm is in the package feed, so it shouldn't be included in DEFAULT_PACKAGES (unless we move the package into the main repo). Now I prefer removing DEVICE_TYPE entirely as Adrian suggested. I can't see any use case of it other than package selections. Perhaps we can create some meta packges (only containing dependencies) as an alternative? Thanks. > > Matthias >
On Fri, May 29, 2020 at 11:12 PM Matthias Schiffer <mschiffer@universe-factory.net> wrote: > Meanwhile I've found another target-specific config setting in the busybox > package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx only. > > I assume "truncate" is tiny enough that it doesn't really justify making > busybox non-shared, we could just build in truncate unconditionally. I > don't know how contrained some of the "nas" targets are, but maybe we > should just replace the busybox hack with a full-featured hdparm on these > targets? That works. My approach in the patch that started the discussion was more along the lines of "least common denominator" - if any target needs hdparm then all targets get hdparm - but that is I suppose ultimately not the best solution for minimalism reasons. BTW I noticed this when using ArchLinux ARM and OpenWrt on the same device an the hard drive would not spin down on OpenWrt and finally hashed out that ArchLinux was setting the spin down time using hdparm. I have a small rc script to do the same on OpenWrt as well that I will add once we hashed out the overall structure for NAS type devices. Yours, Linus Walleij
On Sat, May 30, 2020 at 12:20 PM mans0n <mans0n@gorani.run> wrote: > Busybox hdparm is about 8k and full hdparm is about 93k. I think most > NAS devices can manage that space, so I agree with Matthias. > But the problem is that full hdparm is in the package feed, so it > shouldn't be included in DEFAULT_PACKAGES (unless we move the package > into the main repo). Actually I think my approach in the initial patch to bring the device type up to the Kconfig level solves that, you can use Kconfig to default y if DEVICE_TYPE_NAS (My Kconfig name idea, could be something else, it is a simple bool.) That will make the package or any feed package default on NAS types using just Kconfig to pass information of the device type around. Yours, Linus Walleij
On 5/30/20 12:20 PM, mans0n wrote: > Hi Adrian, Matthias, > > I was preparing my own patch for converting DEVICE_TYPE to a > device-specific variable. > https://github.com/mans0n/openwrt/commit/4d41dd963ae8d595ef38ea0a38ea08abdac1415d > > But I stumbled on some blockers so I left it behind... > > One of the blockers was the busybox hdparm. > I'd also found that DEVICE_TYPE in the busybox Makefile does not work as > intended, thanks to Linus for dealing with this. > >> On 5/29/20 10:52 PM, mail at adrianschmutzler.de wrote: >>>> Or we just drop the variable at all, and do DEFAULT_PACKAGES := >>>> DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router at the beginning (!) of >>>> target.mk, so targets (effectively just 3 of them) can just overwrite >>>> it with DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.nas >>>> directly in the few cases where that is necessary (I'd rather use >>>> DEFAULT_PACKAGES_BASIC etc. as names then). >>> >>> I've pushed a quick draft of this approach here: >>> >>> https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=refs/heads/devicetypedrop >>> >>> >>> Only the most topmost patch is relevant. From "make menuconfig" it seems >>> to work as expected. >> >> I would prefer to find a solution that doesn't require adding >> $(DEFAULT_PACKAGES_BASIC) to the other default package lists. I'll have to >> ponder over this a bit more. Posting the patch - possibly marked as [RFC] - >> would make discussing this easier. >> >> >>> >>> The if/else in busybox is not considered in this patch. >>> >> >> Meanwhile I've found another target-specific config setting in the busybox >> package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx only. >> >> I assume "truncate" is tiny enough that it doesn't really justify making >> busybox non-shared, we could just build in truncate unconditionally. I >> don't know how contrained some of the "nas" targets are, but maybe we >> should just replace the busybox hack with a full-featured hdparm on these >> targets? > > Busybox hdparm is about 8k and full hdparm is about 93k. I think most NAS > devices can manage that space, so I agree with Matthias. > But the problem is that full hdparm is in the package feed, so it shouldn't > be included in DEFAULT_PACKAGES (unless we move the package into the main > repo). Moving hdparm to OpenWrt base sounds fine to me. I can take care of that, and removing the target-specific busybox config, sometime this weekend. Mtthias > > Now I prefer removing DEVICE_TYPE entirely as Adrian suggested. I can't see > any use case of it other than package selections. > Perhaps we can create some meta packges (only containing dependencies) as > an alternative? > > Thanks. > >> >> Matthias >> > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Hi, > -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of mans0n > Sent: Samstag, 30. Mai 2020 12:20 > To: 'Matthias Schiffer' <mschiffer@universe-factory.net>; > mail@adrianschmutzler.de > Cc: 'Linus Walleij' <linus.walleij@linaro.org>; openwrt- > devel@lists.openwrt.org > Subject: Re: [OpenWrt-Devel] [PATCH 3/3] treewide: rename DEVICE_TYPE > to DEFAULT_TYPE > > Hi Adrian, Matthias, > > I was preparing my own patch for converting DEVICE_TYPE to a device- > specific variable. > https://github.com/mans0n/openwrt/commit/4d41dd963ae8d595ef38ea0a3 > 8ea08abdac1415d > But I stumbled on some blockers so I left it behind... One of the problems of this approach (changing DEVICE_PACKAGES) is that it will only work if CONFIG_TARGET_PER_DEVICE_ROOTFS is set, as only then DEVICE_PACKAGES will be evaluated IIRC. So, this won't help for building Default Profile and I don't know whether it will work for a single target device being selected directly (without Multi Profile). So I don't think this will help for package selection at least. Best Adrian > > One of the blockers was the busybox hdparm. > I'd also found that DEVICE_TYPE in the busybox Makefile does not work as > intended, thanks to Linus for dealing with this. > > > On 5/29/20 10:52 PM, mail at adrianschmutzler.de wrote: > >>> Or we just drop the variable at all, and do DEFAULT_PACKAGES := > >>> DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router at the beginning > (!) > >>> of target.mk, so targets (effectively just 3 of them) can just > >>> overwrite it with DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic > >>> DEFAULT_PACKAGES.nas directly in the few cases where that is > necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names then). > >> > >> I've pushed a quick draft of this approach here: > >> > >> https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=re > >> fs/heads/devicetypedrop > >> > >> Only the most topmost patch is relevant. From "make menuconfig" it > seems to work as expected. > > > > I would prefer to find a solution that doesn't require adding > > $(DEFAULT_PACKAGES_BASIC) to the other default package lists. I'll > > have to ponder over this a bit more. Posting the patch - possibly > > marked as [RFC] - would make discussing this easier. > > > > > >> > >> The if/else in busybox is not considered in this patch. > >> > > > > Meanwhile I've found another target-specific config setting in the > > busybox > > package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx > only. > > > > I assume "truncate" is tiny enough that it doesn't really justify > > making busybox non-shared, we could just build in truncate > > unconditionally. I don't know how contrained some of the "nas" targets > > are, but maybe we should just replace the busybox hack with a > > full-featured hdparm on these targets? > > Busybox hdparm is about 8k and full hdparm is about 93k. I think most NAS > devices can manage that space, so I agree with Matthias. > But the problem is that full hdparm is in the package feed, so it shouldn't be > included in DEFAULT_PACKAGES (unless we move the package into the main > repo). > > Now I prefer removing DEVICE_TYPE entirely as Adrian suggested. I can't see > any use case of it other than package selections. > Perhaps we can create some meta packges (only containing dependencies) > as an alternative? > > Thanks. > > > > > Matthias > > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Matthias Schiffer > Sent: Samstag, 30. Mai 2020 16:45 > To: mans0n <mans0n@gorani.run> > Cc: 'Linus Walleij' <linus.walleij@linaro.org>; openwrt- > devel@lists.openwrt.org; mail@adrianschmutzler.de > Subject: Re: [OpenWrt-Devel] [PATCH 3/3] treewide: rename DEVICE_TYPE > to DEFAULT_TYPE > > On 5/30/20 12:20 PM, mans0n wrote: > > Hi Adrian, Matthias, > > > > I was preparing my own patch for converting DEVICE_TYPE to a > > device-specific variable. > > > https://github.com/mans0n/openwrt/commit/4d41dd963ae8d595ef38ea0a3 > 8ea0 > > 8abdac1415d > > > > But I stumbled on some blockers so I left it behind... > > > > One of the blockers was the busybox hdparm. > > I'd also found that DEVICE_TYPE in the busybox Makefile does not work > > as intended, thanks to Linus for dealing with this. > > > >> On 5/29/20 10:52 PM, mail at adrianschmutzler.de wrote: > >>>> Or we just drop the variable at all, and do DEFAULT_PACKAGES := > >>>> DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router at the > beginning (!) > >>>> of target.mk, so targets (effectively just 3 of them) can just > >>>> overwrite it with DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic > >>>> DEFAULT_PACKAGES.nas directly in the few cases where that is > >>>> necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names > then). > >>> > >>> I've pushed a quick draft of this approach here: > >>> > >>> https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=r > >>> efs/heads/devicetypedrop > >>> > >>> > >>> Only the most topmost patch is relevant. From "make menuconfig" it > >>> seems to work as expected. > >> > >> I would prefer to find a solution that doesn't require adding > >> $(DEFAULT_PACKAGES_BASIC) to the other default package lists. I'll > >> have to ponder over this a bit more. Posting the patch - possibly > >> marked as [RFC] - would make discussing this easier. > >> > >> > >>> > >>> The if/else in busybox is not considered in this patch. > >>> > >> > >> Meanwhile I've found another target-specific config setting in the > >> busybox > >> package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx > only. > >> > >> I assume "truncate" is tiny enough that it doesn't really justify > >> making busybox non-shared, we could just build in truncate > >> unconditionally. I don't know how contrained some of the "nas" > >> targets are, but maybe we should just replace the busybox hack with a > >> full-featured hdparm on these targets? > > > > Busybox hdparm is about 8k and full hdparm is about 93k. I think most > > NAS devices can manage that space, so I agree with Matthias. > > But the problem is that full hdparm is in the package feed, so it > > shouldn't be included in DEFAULT_PACKAGES (unless we move the > package > > into the main repo). > > Moving hdparm to OpenWrt base sounds fine to me. I can take care of that, > and removing the target-specific busybox config, sometime this weekend. I'd be happy about anything that removes the DEVICE_TYPE dependency from package config, as I don't really think it's worth keeping that entire mechanism for the small set of options and devices that are effectively affected here (referring to the busybox switch here, DEFAULT_PACKAGES are a separate discussion). Best Adrian > > Mtthias > > > > > > Now I prefer removing DEVICE_TYPE entirely as Adrian suggested. I > > can't see any use case of it other than package selections. > > Perhaps we can create some meta packges (only containing dependencies) > > as an alternative? > > > > Thanks. > > > >> > >> Matthias > >> > > > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel >
On 5/31/20 12:06 AM, Adrian Schmutzler wrote: > Hi, > >> -----Original Message----- >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] >> On Behalf Of mans0n >> Sent: Samstag, 30. Mai 2020 12:20 >> To: 'Matthias Schiffer' <mschiffer@universe-factory.net>; >> mail@adrianschmutzler.de >> Cc: 'Linus Walleij' <linus.walleij@linaro.org>; openwrt- >> devel@lists.openwrt.org >> Subject: Re: [OpenWrt-Devel] [PATCH 3/3] treewide: rename DEVICE_TYPE >> to DEFAULT_TYPE >> >> Hi Adrian, Matthias, >> >> I was preparing my own patch for converting DEVICE_TYPE to a device- >> specific variable. >> https://github.com/mans0n/openwrt/commit/4d41dd963ae8d595ef38ea0a3 >> 8ea08abdac1415d >> But I stumbled on some blockers so I left it behind... > > One of the problems of this approach (changing DEVICE_PACKAGES) is that it > will only work if CONFIG_TARGET_PER_DEVICE_ROOTFS is set, as only then > DEVICE_PACKAGES will be evaluated IIRC. So, this won't help for building > Default Profile and I don't know whether it will work for a single target > device being selected directly (without Multi Profile). DEVICE_PACKAGES works fine without multi-profile (in fact, it has existed for a bit longer than CONFIG_TARGET_PER_DEVICE_ROOTFS). I don't think we need to care about multi-profile without PER_DEVICE_ROOTFS - both OpenWrt buildbots and Gluon use PER_DEVICE_ROOTFS. IMO "pultiprofile without per-device rootfs" and the "default profile" are legacy features that aren't very useful now that better alternatives exist. Matthias > > So I don't think this will help for package selection at least. > > Best > > Adrian > >> >> One of the blockers was the busybox hdparm. >> I'd also found that DEVICE_TYPE in the busybox Makefile does not work as >> intended, thanks to Linus for dealing with this. >> >>> On 5/29/20 10:52 PM, mail at adrianschmutzler.de wrote: >>>>> Or we just drop the variable at all, and do DEFAULT_PACKAGES := >>>>> DEFAULT_PACKAGES.basic DEFAULT_PACKAGES.router at the beginning >> (!) >>>>> of target.mk, so targets (effectively just 3 of them) can just >>>>> overwrite it with DEFAULT_PACKAGES := DEFAULT_PACKAGES.basic >>>>> DEFAULT_PACKAGES.nas directly in the few cases where that is >> necessary (I'd rather use DEFAULT_PACKAGES_BASIC etc. as names then). >>>> >>>> I've pushed a quick draft of this approach here: >>>> >>>> https://git.openwrt.org/?p=openwrt/staging/adrian.git;a=shortlog;h=re >>>> fs/heads/devicetypedrop >>>> >>>> Only the most topmost patch is relevant. From "make menuconfig" it >> seems to work as expected. >>> >>> I would prefer to find a solution that doesn't require adding >>> $(DEFAULT_PACKAGES_BASIC) to the other default package lists. I'll >>> have to ponder over this a bit more. Posting the patch - possibly >>> marked as [RFC] - would make discussing this easier. >>> >>> >>>> >>>> The if/else in busybox is not considered in this patch. >>>> >>> >>> Meanwhile I've found another target-specific config setting in the >>> busybox >>> package: BUSYBOX_DEFAULT_TRUNCATE is enabled for TARGET_bcm53xx >> only. >>> >>> I assume "truncate" is tiny enough that it doesn't really justify >>> making busybox non-shared, we could just build in truncate >>> unconditionally. I don't know how contrained some of the "nas" targets >>> are, but maybe we should just replace the busybox hack with a >>> full-featured hdparm on these targets? >> >> Busybox hdparm is about 8k and full hdparm is about 93k. I think most NAS >> devices can manage that space, so I agree with Matthias. >> But the problem is that full hdparm is in the package feed, so it > shouldn't be >> included in DEFAULT_PACKAGES (unless we move the package into the main >> repo). >> >> Now I prefer removing DEVICE_TYPE entirely as Adrian suggested. I can't > see >> any use case of it other than package selections. >> Perhaps we can create some meta packges (only containing dependencies) >> as an alternative? >> >> Thanks. >> >>> >>> Matthias >>> >> >> _______________________________________________ >> openwrt-devel mailing list >> openwrt-devel@lists.openwrt.org >> https://lists.openwrt.org/mailman/listinfo/openwrt-devel >
diff --git a/include/target.mk b/include/target.mk index a2ceb7f783..8374de2ebd 100644 --- a/include/target.mk +++ b/include/target.mk @@ -10,7 +10,7 @@ ifneq ($(__target_inc),1) __target_inc=1 # default device type -DEVICE_TYPE?=router +DEFAULT_TYPE?=router # Default packages - the really basic set DEFAULT_PACKAGES:=base-files libc libgcc busybox dropbear mtd uci opkg netifd fstools uclient-fetch logd urandom-seed urngd @@ -53,7 +53,7 @@ else endif # Add device specific packages (here below to allow device type set from subtarget) -DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEVICE_TYPE)) +DEFAULT_PACKAGES += $(DEFAULT_PACKAGES.$(DEFAULT_TYPE)) filter_packages = $(filter-out -% $(patsubst -%,%,$(filter -%,$(1))),$(1)) extra_packages = $(if $(filter wpad-mini wpad-basic wpad nas,$(1)),iwinfo) diff --git a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile index 01441d1e87..81dde74d0b 100644 --- a/package/utils/busybox/Makefile +++ b/package/utils/busybox/Makefile @@ -94,7 +94,7 @@ endif define Build/Configure rm -f $(PKG_BUILD_DIR)/.config touch $(PKG_BUILD_DIR)/.config -ifeq ($(DEVICE_TYPE),nas) +ifeq ($(DEFAULT_TYPE),nas) echo "CONFIG_HDPARM=y" >> $(PKG_BUILD_DIR)/.config endif grep 'CONFIG_BUSYBOX_$(BUSYBOX_SYM)' $(TOPDIR)/.config | sed -e "s,\\(# \)\\?CONFIG_BUSYBOX_$(BUSYBOX_SYM)_\\(.*\\),\\1CONFIG_\\2,g" >> $(PKG_BUILD_DIR)/.config diff --git a/target/linux/arc770/Makefile b/target/linux/arc770/Makefile index 018d6e5448..d1f3e2dc82 100644 --- a/target/linux/arc770/Makefile +++ b/target/linux/arc770/Makefile @@ -13,7 +13,7 @@ SUBTARGETS:=generic KERNEL_PATCHVER:=4.14 -DEVICE_TYPE:=basic +DEFAULT_TYPE:=basic define Target/Description Synopsys DesignWare boards diff --git a/target/linux/archs38/Makefile b/target/linux/archs38/Makefile index 5b3650ef8d..891583b2d2 100644 --- a/target/linux/archs38/Makefile +++ b/target/linux/archs38/Makefile @@ -14,7 +14,7 @@ SUBTARGETS:=generic KERNEL_PATCHVER:=5.4 -DEVICE_TYPE:=basic +DEFAULT_TYPE:=basic define Target/Description Synopsys DesignWare boards diff --git a/target/linux/oxnas/Makefile b/target/linux/oxnas/Makefile index 750eddbcbb..10d05e914f 100644 --- a/target/linux/oxnas/Makefile +++ b/target/linux/oxnas/Makefile @@ -5,7 +5,7 @@ BOARD:=oxnas BOARDNAME:=PLXTECH/Oxford NAS782x/OX8xx SUBTARGETS:=ox810se ox820 FEATURES:=gpio ramdisk rtc squashfs -DEVICE_TYPE:=nas +DEFAULT_TYPE:=nas KERNEL_PATCHVER:=5.4
The prefix "DEVICE_" for Make variables is only used for per-device variables with the only exception of DEVICE_TYPE. This is misleading as it leads people to incorrectly assume it can be set per device like all the other DEVICE_* variables, as has been observed in the past. This renames this (rarely used) variable to clearly indicate that it's not a device-dependent variable, and stays in line with the DEFAULT_PACKAGES variable. Note that there is also a (single) package in the packages feed that needs to be updated. Cc: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de> --- include/target.mk | 4 ++-- package/utils/busybox/Makefile | 2 +- target/linux/arc770/Makefile | 2 +- target/linux/archs38/Makefile | 2 +- target/linux/oxnas/Makefile | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-)