diff mbox series

[OpenWrt-Devel] build: reflect DEVICE_TYPE to top level config

Message ID 20200529122039.1246329-1-linus.walleij@linaro.org
State New
Headers show
Series [OpenWrt-Devel] build: reflect DEVICE_TYPE to top level config | expand

Commit Message

Linus Walleij May 29, 2020, 12:20 p.m. UTC
I made a patch to select a tool inside busybox by
default on NAS type boxes, but this does not properly
work because the package and image build processes are
cleanly separate entities.

I also noticed that this becomes a problem if you
build multiple profiles: maybe one of them is NAS
and one of them is a router. You still want the least
common denominator to decide: if you selected both
NAS:es and routers, build packages that will be
suitable for both NAS and routers.

To achieve this I reflect the DEVICE_TYPE up to the
Kconfig level and define two Kconfig symbols:

config DEVICE_TYPE_ROUTER
       bool

config DEVICE_TYPE_NAS
       bool

These will be set from the DEVICE_TYPE of each
profile and it is possible to select both.

I then modify the busybox config to take this into
account and conditionally build hdparm for
CONFIG_DEVICE_TYPE_NAS.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 include/image.mk               |  1 +
 include/target.mk              |  1 +
 package/utils/busybox/Makefile |  2 +-
 scripts/metadata.pm            |  2 ++
 scripts/target-metadata.pl     | 12 ++++++++++++
 5 files changed, 17 insertions(+), 1 deletion(-)

Comments

Adrian Schmutzler May 29, 2020, 12:42 p.m. UTC | #1
> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Linus Walleij
> Sent: Freitag, 29. Mai 2020 14:21
> To: openwrt-devel@lists.openwrt.org
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Subject: [OpenWrt-Devel] [PATCH] build: reflect DEVICE_TYPE to top level
> config
> 
> I made a patch to select a tool inside busybox by default on NAS type boxes,
> but this does not properly work because the package and image build
> processes are cleanly separate entities.
> 
> I also noticed that this becomes a problem if you build multiple profiles:
> maybe one of them is NAS and one of them is a router. You still want the
> least common denominator to decide: if you selected both NAS:es and
> routers, build packages that will be suitable for both NAS and routers.
> 
> To achieve this I reflect the DEVICE_TYPE up to the Kconfig level and define
> two Kconfig symbols:

Hi,

that might be irrelevant for your patch in its current state, but note that DEVICE_TYPE is not properly implemented at the moment.

A partial discussion about that can be found here:
http://lists.infradead.org/pipermail/openwrt-devel/2020-February/021809.html

Effectively, DEVICE_TYPE currently is a per-target variable, but for some devices it's incorrectly used as if it was a per-device variable. (by that, effectively using the last value of the variable for the entire target.)

Therefore, the configuration for individual devices might be different than what's expected from the unaware observer.

This should be fixed by properly setting up DEVICE_TYPE as a per-device variable, which itself is not a problem. However, it might need some effort to correctly adjust the current cases where it is used as per-target variable by intention.

I will try to have another look at this myself soon.

Best

Adrian

> 
> config DEVICE_TYPE_ROUTER
>        bool
> 
> config DEVICE_TYPE_NAS
>        bool
> 
> These will be set from the DEVICE_TYPE of each profile and it is possible to
> select both.
> 
> I then modify the busybox config to take this into account and conditionally
> build hdparm for CONFIG_DEVICE_TYPE_NAS.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  include/image.mk               |  1 +
>  include/target.mk              |  1 +
>  package/utils/busybox/Makefile |  2 +-
>  scripts/metadata.pm            |  2 ++
>  scripts/target-metadata.pl     | 12 ++++++++++++
>  5 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/image.mk b/include/image.mk index
> 984b64fb9c73..8104c040a1f7 100644
> --- a/include/image.mk
> +++ b/include/image.mk
> @@ -634,6 +634,7 @@ endef
>  define Device/DumpInfo
>  Target-Profile: DEVICE_$(1)
>  Target-Profile-Name: $(DEVICE_DISPLAY)
> +Target-Profile-Devicetype: $(DEVICE_TYPE)
>  Target-Profile-Packages: $(DEVICE_PACKAGES)
>  Target-Profile-hasImageMetadata: $(if $(foreach
> image,$(IMAGES),$(findstring append-metadata,$(IMAGE/$(image)))),1,0)
>  Target-Profile-SupportedDevices: $(SUPPORTED_DEVICES) diff --git
> a/include/target.mk b/include/target.mk index 9bd4c14936c1..e6f26cbfdf3d
> 100644
> --- a/include/target.mk
> +++ b/include/target.mk
> @@ -73,6 +73,7 @@ define Profile
>  	echo "Target-Profile: $(1)"; \
>  	$(if $(PRIORITY), echo "Target-Profile-Priority: $(PRIORITY)"; ) \
>  	echo "Target-Profile-Name: $(NAME)"; \
> +	echo "Target-Profile-Devicetype: $(DEVICE_TYPE)"; \
>  	echo "Target-Profile-Packages: $(PACKAGES) $(call
> extra_packages,$(DEFAULT_PACKAGES) $(PACKAGES))"; \
>  	echo "Target-Profile-Description:"; \
>  	echo "$$$$$$$$$(call shvar,Profile/$(1)/Description)"; \ diff --git
> a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile index
> 01441d1e87d1..f504117f60f3 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 ($(CONFIG_DEVICE_TYPE_NAS),y)
>  	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/scripts/metadata.pm
> b/scripts/metadata.pm index 1826a040a116..5062dba37ec0 100644
> --- a/scripts/metadata.pm
> +++ b/scripts/metadata.pm
> @@ -140,6 +140,7 @@ sub parse_target_metadata($) {
>  			$profile = {
>  				id => $1,
>  				name => $1,
> +				device_type => "router",
>  				has_image_metadata => 0,
>  				supported_devices => [],
>  				priority => 999,
> @@ -150,6 +151,7 @@ sub parse_target_metadata($) {
>  			push @{$target->{profiles}}, $profile;
>  		};
>  		/^Target-Profile-Name:\s*(.+)\s*$/ and $profile->{name} =
> $1;
> +		/^Target-Profile-Devicetype:\s*(.+)\s*$/ and $profile-
> >{device_type}
> += $1;
>  		/^Target-Profile-hasImageMetadata:\s*(\d+)\s*$/ and
> $profile->{has_image_metadata} = $1;
>  		/^Target-Profile-SupportedDevices:\s*(.+)\s*$/ and $profile-
> >{supported_devices} = [ split(/\s+/, $1) ];
>  		/^Target-Profile-Priority:\s*(\d+)\s*$/ and do { diff --git
> a/scripts/target-metadata.pl b/scripts/target-metadata.pl index
> ee0ab5a71811..fbd9fa70c08b 100755
> --- a/scripts/target-metadata.pl
> +++ b/scripts/target-metadata.pl
> @@ -244,6 +244,12 @@ EOF
>  				print "\tselect DEFAULT_$pkg\n";
>  				$defaults{$pkg} = 1;
>  			}
> +			if ($profile->{device_type} =~ "router") {
> +				print "\tselect DEVICE_TYPE_ROUTER\n";
> +			}
> +			if ($profile->{device_type} =~ "nas") {
> +				print "\tselect DEVICE_TYPE_NAS\n";
> +			}
>  			my $help = $profile->{desc};
>  			if ($help =~ /\w+/) {
>  				$help =~ s/^\s*/\t  /mg;
> @@ -328,6 +334,12 @@ config HAS_SUBTARGETS  config HAS_DEVICES
>  	bool
> 
> +config DEVICE_TYPE_ROUTER
> +	bool
> +
> +config DEVICE_TYPE_NAS
> +	bool
> +
>  config TARGET_BOARD
>  	string
> 
> --
> 2.26.2
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Adrian Schmutzler May 29, 2020, 5:39 p.m. UTC | #2
> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Linus Walleij
> Sent: Freitag, 29. Mai 2020 14:21
> To: openwrt-devel@lists.openwrt.org
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Subject: [OpenWrt-Devel] [PATCH] build: reflect DEVICE_TYPE to top level
> config

Hi again,

sorry for intruding into this subject, but this has annoyed me for a long time already.

I've just sent a small patchset to tidy up the existing situation.

> 
> I made a patch to select a tool inside busybox by default on NAS type boxes,
> but this does not properly work because the package and image build
> processes are cleanly separate entities.
> 
> I also noticed that this becomes a problem if you build multiple profiles:
> maybe one of them is NAS and one of them is a router. You still want the
> least common denominator to decide: if you selected both NAS:es and
> routers, build packages that will be suitable for both NAS and routers.
> 
> To achieve this I reflect the DEVICE_TYPE up to the Kconfig level and define
> two Kconfig symbols:
> 
> config DEVICE_TYPE_ROUTER
>        bool
> 
> config DEVICE_TYPE_NAS
>        bool
> 
> These will be set from the DEVICE_TYPE of each profile and it is possible to
> select both.

Unfortunately, this doesn't seem to work like (at least) I would expect it to:

For the "Default Profile", the CONFIG_DEVICE_TYPE_* is set based on the target/subtarget Makefile.
If you select an individual device as target profile, the setting in the (sub)target Makefile is ignored (!), and the default ("router") is used. If the device has a DEVICE_TYPE in the device definition (which is wrong based on the initial concept, see my patch 1/3), then this value will be used.
If you select "Multiple devices", then CONFIG_DEVICE_TYPE_* won't be set at all.

As you stated earlier, it's just not so easy to connect the target and device scopes with each other. At the moment, I see two ways out of this:

1. We just live with the fact that the switch between router/nas/basic is per subtarget and adjust the code based on that.
2. We make the DEVICE_TYPE a real device-dependent variable and move it from target.mk to image.mk. Then, we could still set default values per target, but would have to adjust DEVICE_PACKAGES instead of DEFAULT_PACKAGES, which would lead to problems when building the Default Profile, but would make it much easier to deal with the individual devices.

Anyway, thanks for stirring this topic up again. Unfortunately, I don't think this will come cheap.

Best

Adrian

> 
> I then modify the busybox config to take this into account and conditionally
> build hdparm for CONFIG_DEVICE_TYPE_NAS.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  include/image.mk               |  1 +
>  include/target.mk              |  1 +
>  package/utils/busybox/Makefile |  2 +-
>  scripts/metadata.pm            |  2 ++
>  scripts/target-metadata.pl     | 12 ++++++++++++
>  5 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/image.mk b/include/image.mk index
> 984b64fb9c73..8104c040a1f7 100644
> --- a/include/image.mk
> +++ b/include/image.mk
> @@ -634,6 +634,7 @@ endef
>  define Device/DumpInfo
>  Target-Profile: DEVICE_$(1)
>  Target-Profile-Name: $(DEVICE_DISPLAY)
> +Target-Profile-Devicetype: $(DEVICE_TYPE)
>  Target-Profile-Packages: $(DEVICE_PACKAGES)
>  Target-Profile-hasImageMetadata: $(if $(foreach
> image,$(IMAGES),$(findstring append-metadata,$(IMAGE/$(image)))),1,0)
>  Target-Profile-SupportedDevices: $(SUPPORTED_DEVICES) diff --git
> a/include/target.mk b/include/target.mk index 9bd4c14936c1..e6f26cbfdf3d
> 100644
> --- a/include/target.mk
> +++ b/include/target.mk
> @@ -73,6 +73,7 @@ define Profile
>  	echo "Target-Profile: $(1)"; \
>  	$(if $(PRIORITY), echo "Target-Profile-Priority: $(PRIORITY)"; ) \
>  	echo "Target-Profile-Name: $(NAME)"; \
> +	echo "Target-Profile-Devicetype: $(DEVICE_TYPE)"; \
>  	echo "Target-Profile-Packages: $(PACKAGES) $(call
> extra_packages,$(DEFAULT_PACKAGES) $(PACKAGES))"; \
>  	echo "Target-Profile-Description:"; \
>  	echo "$$$$$$$$$(call shvar,Profile/$(1)/Description)"; \ diff --git
> a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile index
> 01441d1e87d1..f504117f60f3 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 ($(CONFIG_DEVICE_TYPE_NAS),y)
>  	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/scripts/metadata.pm
> b/scripts/metadata.pm index 1826a040a116..5062dba37ec0 100644
> --- a/scripts/metadata.pm
> +++ b/scripts/metadata.pm
> @@ -140,6 +140,7 @@ sub parse_target_metadata($) {
>  			$profile = {
>  				id => $1,
>  				name => $1,
> +				device_type => "router",
>  				has_image_metadata => 0,
>  				supported_devices => [],
>  				priority => 999,
> @@ -150,6 +151,7 @@ sub parse_target_metadata($) {
>  			push @{$target->{profiles}}, $profile;
>  		};
>  		/^Target-Profile-Name:\s*(.+)\s*$/ and $profile->{name} =
> $1;
> +		/^Target-Profile-Devicetype:\s*(.+)\s*$/ and $profile-
> >{device_type}
> += $1;
>  		/^Target-Profile-hasImageMetadata:\s*(\d+)\s*$/ and
> $profile->{has_image_metadata} = $1;
>  		/^Target-Profile-SupportedDevices:\s*(.+)\s*$/ and $profile-
> >{supported_devices} = [ split(/\s+/, $1) ];
>  		/^Target-Profile-Priority:\s*(\d+)\s*$/ and do { diff --git
> a/scripts/target-metadata.pl b/scripts/target-metadata.pl index
> ee0ab5a71811..fbd9fa70c08b 100755
> --- a/scripts/target-metadata.pl
> +++ b/scripts/target-metadata.pl
> @@ -244,6 +244,12 @@ EOF
>  				print "\tselect DEFAULT_$pkg\n";
>  				$defaults{$pkg} = 1;
>  			}
> +			if ($profile->{device_type} =~ "router") {
> +				print "\tselect DEVICE_TYPE_ROUTER\n";
> +			}
> +			if ($profile->{device_type} =~ "nas") {
> +				print "\tselect DEVICE_TYPE_NAS\n";
> +			}
>  			my $help = $profile->{desc};
>  			if ($help =~ /\w+/) {
>  				$help =~ s/^\s*/\t  /mg;
> @@ -328,6 +334,12 @@ config HAS_SUBTARGETS  config HAS_DEVICES
>  	bool
> 
> +config DEVICE_TYPE_ROUTER
> +	bool
> +
> +config DEVICE_TYPE_NAS
> +	bool
> +
>  config TARGET_BOARD
>  	string
> 
> --
> 2.26.2
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Linus Walleij May 30, 2020, 11:17 a.m. UTC | #3
On Fri, May 29, 2020 at 7:39 PM <mail@adrianschmutzler.de> wrote:

> sorry for intruding into this subject, but this has annoyed me for a long time already.

I am very happy that you are taking up the subject :D

> I've just sent a small patchset to tidy up the existing situation.

Excellent, will look.

> As you stated earlier, it's just not so easy to connect the target and device scopes with each other. At the moment, I see two ways out of this:
>
> 1. We just live with the fact that the switch between router/nas/basic is per subtarget and adjust the code based on that.
> 2. We make the DEVICE_TYPE a real device-dependent variable and move it from target.mk to image.mk. Then, we could still set default values per target, but would have to adjust DEVICE_PACKAGES instead of DEFAULT_PACKAGES, which would lead to problems when building the Default Profile, but would make it much easier to deal with the individual devices.

I vote for (2)

> Anyway, thanks for stirring this topic up again. Unfortunately, I don't think this will come cheap.

Yeah I started looking at it and it was "mildly confusing" but as I
anyway had to
learn how OpenWrt is generating the Kconfig menus from Makefiles using
a perl script it was a minor thing compared to figuring out the overall picture.
I knew I would get it wrong anyways. ;)

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/include/image.mk b/include/image.mk
index 984b64fb9c73..8104c040a1f7 100644
--- a/include/image.mk
+++ b/include/image.mk
@@ -634,6 +634,7 @@  endef
 define Device/DumpInfo
 Target-Profile: DEVICE_$(1)
 Target-Profile-Name: $(DEVICE_DISPLAY)
+Target-Profile-Devicetype: $(DEVICE_TYPE)
 Target-Profile-Packages: $(DEVICE_PACKAGES)
 Target-Profile-hasImageMetadata: $(if $(foreach image,$(IMAGES),$(findstring append-metadata,$(IMAGE/$(image)))),1,0)
 Target-Profile-SupportedDevices: $(SUPPORTED_DEVICES)
diff --git a/include/target.mk b/include/target.mk
index 9bd4c14936c1..e6f26cbfdf3d 100644
--- a/include/target.mk
+++ b/include/target.mk
@@ -73,6 +73,7 @@  define Profile
 	echo "Target-Profile: $(1)"; \
 	$(if $(PRIORITY), echo "Target-Profile-Priority: $(PRIORITY)"; ) \
 	echo "Target-Profile-Name: $(NAME)"; \
+	echo "Target-Profile-Devicetype: $(DEVICE_TYPE)"; \
 	echo "Target-Profile-Packages: $(PACKAGES) $(call extra_packages,$(DEFAULT_PACKAGES) $(PACKAGES))"; \
 	echo "Target-Profile-Description:"; \
 	echo "$$$$$$$$$(call shvar,Profile/$(1)/Description)"; \
diff --git a/package/utils/busybox/Makefile b/package/utils/busybox/Makefile
index 01441d1e87d1..f504117f60f3 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 ($(CONFIG_DEVICE_TYPE_NAS),y)
 	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/scripts/metadata.pm b/scripts/metadata.pm
index 1826a040a116..5062dba37ec0 100644
--- a/scripts/metadata.pm
+++ b/scripts/metadata.pm
@@ -140,6 +140,7 @@  sub parse_target_metadata($) {
 			$profile = {
 				id => $1,
 				name => $1,
+				device_type => "router",
 				has_image_metadata => 0,
 				supported_devices => [],
 				priority => 999,
@@ -150,6 +151,7 @@  sub parse_target_metadata($) {
 			push @{$target->{profiles}}, $profile;
 		};
 		/^Target-Profile-Name:\s*(.+)\s*$/ and $profile->{name} = $1;
+		/^Target-Profile-Devicetype:\s*(.+)\s*$/ and $profile->{device_type} = $1;
 		/^Target-Profile-hasImageMetadata:\s*(\d+)\s*$/ and $profile->{has_image_metadata} = $1;
 		/^Target-Profile-SupportedDevices:\s*(.+)\s*$/ and $profile->{supported_devices} = [ split(/\s+/, $1) ];
 		/^Target-Profile-Priority:\s*(\d+)\s*$/ and do {
diff --git a/scripts/target-metadata.pl b/scripts/target-metadata.pl
index ee0ab5a71811..fbd9fa70c08b 100755
--- a/scripts/target-metadata.pl
+++ b/scripts/target-metadata.pl
@@ -244,6 +244,12 @@  EOF
 				print "\tselect DEFAULT_$pkg\n";
 				$defaults{$pkg} = 1;
 			}
+			if ($profile->{device_type} =~ "router") {
+				print "\tselect DEVICE_TYPE_ROUTER\n";
+			}
+			if ($profile->{device_type} =~ "nas") {
+				print "\tselect DEVICE_TYPE_NAS\n";
+			}
 			my $help = $profile->{desc};
 			if ($help =~ /\w+/) {
 				$help =~ s/^\s*/\t  /mg;
@@ -328,6 +334,12 @@  config HAS_SUBTARGETS
 config HAS_DEVICES
 	bool
 
+config DEVICE_TYPE_ROUTER
+	bool
+
+config DEVICE_TYPE_NAS
+	bool
+
 config TARGET_BOARD
 	string