| Message ID | 1452489360-115700-1-git-send-email-openwrt@daniel.thecshore.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2016-01-11 06:16, openwrt@daniel.thecshore.com wrote: > From: Daniel Dickinson <openwrt@daniel.thecshore.com> > > Certain platforms have large numbers of possible images, and it can be > desirable to build neither all images nor only a single image, > therefore this patch makes selecting target profiles a menu instead of a > single choice, which allows the user to build a specific subset of all > possible images for a target. > > Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com> > --- > scripts/metadata.pl | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/scripts/metadata.pl b/scripts/metadata.pl > index 48b1b7a..4487d26 100755 > --- a/scripts/metadata.pl > +++ b/scripts/metadata.pl > @@ -275,8 +275,7 @@ EOF > print <<EOF; > endchoice > > -choice > - prompt "Target Profile" > +menu "Target Profile" > > EOF > > @@ -288,8 +287,35 @@ EOF > config TARGET_$target->{conf}_$profile->{id} > bool "$profile->{name}" > depends on TARGET_$target->{conf} > +EOF > + > + if (not (($profile->{id} eq 'Default') || ($profile->{id} eq 'Minimal'))) { > + print <<EOF; > + default y if TARGET_$target->{conf}_Default > + default n if TARGET_$target->{conf}_Minimal > +EOF I like the idea of allowing the user to select multiple profiles. However, there also needs to be a clean and simple way to select a single profile without going through the list and deselecting everything. Also, I don't like hardcoded profile names in metadata.pl, a better approach would be to have a flag as part of a profile that indicates that it's not a device profile. - Felix
Felix, Would it be unreasonable to have overridable defaults like suggested in metadata.pl? Convention over configuration and all that. Eric On Mon, Jan 11, 2016 at 7:35 AM, Felix Fietkau <nbd@openwrt.org> wrote: > On 2016-01-11 06:16, openwrt@daniel.thecshore.com wrote: > > From: Daniel Dickinson <openwrt@daniel.thecshore.com> > > > > Certain platforms have large numbers of possible images, and it can be > > desirable to build neither all images nor only a single image, > > therefore this patch makes selecting target profiles a menu instead of a > > single choice, which allows the user to build a specific subset of all > > possible images for a target. > > > > Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com> > > --- > > scripts/metadata.pl | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/metadata.pl b/scripts/metadata.pl > > index 48b1b7a..4487d26 100755 > > --- a/scripts/metadata.pl > > +++ b/scripts/metadata.pl > > @@ -275,8 +275,7 @@ EOF > > print <<EOF; > > endchoice > > > > -choice > > - prompt "Target Profile" > > +menu "Target Profile" > > > > EOF > > > > @@ -288,8 +287,35 @@ EOF > > config TARGET_$target->{conf}_$profile->{id} > > bool "$profile->{name}" > > depends on TARGET_$target->{conf} > > +EOF > > + > > + if (not (($profile->{id} eq 'Default') || > ($profile->{id} eq 'Minimal'))) { > > + print <<EOF; > > + default y if TARGET_$target->{conf}_Default > > + default n if TARGET_$target->{conf}_Minimal > > +EOF > I like the idea of allowing the user to select multiple profiles. > However, there also needs to be a clean and simple way to select a > single profile without going through the list and deselecting everything. > Also, I don't like hardcoded profile names in metadata.pl, a better > approach would be to have a flag as part of a profile that indicates > that it's not a device profile. > > - Felix > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >
On 11/01/16 08:35 AM, Felix Fietkau wrote: >> Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com> >> --- >> scripts/metadata.pl | 32 +++++++++++++++++++++++++++++--- >> 1 file changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/metadata.pl b/scripts/metadata.pl >> index 48b1b7a..4487d26 100755 >> --- a/scripts/metadata.pl >> +++ b/scripts/metadata.pl >> @@ -275,8 +275,7 @@ EOF >> print <<EOF; >> endchoice >> >> -choice >> - prompt "Target Profile" >> +menu "Target Profile" >> >> EOF >> >> @@ -288,8 +287,35 @@ EOF >> config TARGET_$target->{conf}_$profile->{id} >> bool "$profile->{name}" >> depends on TARGET_$target->{conf} >> +EOF >> + >> + if (not (($profile->{id} eq 'Default') || ($profile->{id} eq 'Minimal'))) { >> + print <<EOF; >> + default y if TARGET_$target->{conf}_Default >> + default n if TARGET_$target->{conf}_Minimal >> +EOF > I like the idea of allowing the user to select multiple profiles. > However, there also needs to be a clean and simple way to select a > single profile without going through the list and deselecting everything. Actually, while it would need to be extended to other platforms, on ar71xx that is exactly what Minimal does. In fact, assuming your on the first run of menuconfig (i.e. the only time defaults work, due to the way KConfig works; this is also a problem for ALL_KMODS and ALL (that de-selecting all kmods and for de-selecting all packages), if you deselect Default, all the items selected will become unselected. It is not possible with KConfig to have an *existing* .config and have an option that causes deselection of selected items, that does not also force deselection without the option to override (e.g. depends on !MINIMAL won't work because it would force the item to off without option to override). If you have some clever way to avoid this problem, I'd love to hear it, because I don't know how to do it in KConfig. I don't see how to allow an *overridable* deselection of *all* items, unless perhaps one could introduce a new submenu that is a choice section (I'd have to test to see if that would actually work, or if duplicate symbols would be a problem). Regards, Daniel > Also, I don't like hardcoded profile names in metadata.pl, a better > approach would be to have a flag as part of a profile that indicates > that it's not a device profile. > > - Felix >
On 2016-01-11 21:11, Daniel Dickinson wrote: > On 11/01/16 08:35 AM, Felix Fietkau wrote: >> I like the idea of allowing the user to select multiple profiles. >> However, there also needs to be a clean and simple way to select a >> single profile without going through the list and deselecting everything. > > Actually, while it would need to be extended to other platforms, on > ar71xx that is exactly what Minimal does. In fact, assuming your on the > first run of menuconfig (i.e. the only time defaults work, due to the > way KConfig works; this is also a problem for ALL_KMODS and ALL (that > de-selecting all kmods and for de-selecting all packages), if you > deselect Default, all the items selected will become unselected. I think that will not be clear to users at all, and it means you can't conveniently switch from a config with all profiles to one with just one profile enabled without re-making your changes. > It is not possible with KConfig to have an *existing* .config and have > an option that causes deselection of selected items, that does not also > force deselection without the option to override (e.g. depends on > !MINIMAL won't work because it would force the item to off without > option to override). > > If you have some clever way to avoid this problem, I'd love to hear it, > because I don't know how to do it in KConfig. I don't see how to allow > an *overridable* deselection of *all* items, unless perhaps one could > introduce a new submenu that is a choice section (I'd have to test to > see if that would actually work, or if duplicate symbols would be a > problem). You could emit some extra code to have an option to select a single profile, which will deselect all profiles except for one that is selected by a choice value. - Felix
Actually I thought of a solution, but it starts to get ugly. A choice submemenu with non-duplicate (i.e. TARGET_SINGLE_...) symbols that also select TARGET_SINGLE, AND have all the normal TARGET_... depend on !(TARGET_SYMBOL && !TARGET_SINGLE_..profile). That would probably work, but it's rather ugly IMO. Regards, Daniel On 11/01/16 08:35 AM, Felix Fietkau wrote: > On 2016-01-11 06:16, openwrt@daniel.thecshore.com wrote: >> From: Daniel Dickinson <openwrt@daniel.thecshore.com> >> >> Certain platforms have large numbers of possible images, and it can be >> desirable to build neither all images nor only a single image, >> therefore this patch makes selecting target profiles a menu instead of a >> single choice, which allows the user to build a specific subset of all >> possible images for a target. >> >> Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com> >> --- >> scripts/metadata.pl | 32 +++++++++++++++++++++++++++++--- >> 1 file changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/metadata.pl b/scripts/metadata.pl >> index 48b1b7a..4487d26 100755 >> --- a/scripts/metadata.pl >> +++ b/scripts/metadata.pl >> @@ -275,8 +275,7 @@ EOF >> print <<EOF; >> endchoice >> >> -choice >> - prompt "Target Profile" >> +menu "Target Profile" >> >> EOF >> >> @@ -288,8 +287,35 @@ EOF >> config TARGET_$target->{conf}_$profile->{id} >> bool "$profile->{name}" >> depends on TARGET_$target->{conf} >> +EOF >> + >> + if (not (($profile->{id} eq 'Default') || ($profile->{id} eq 'Minimal'))) { >> + print <<EOF; >> + default y if TARGET_$target->{conf}_Default >> + default n if TARGET_$target->{conf}_Minimal >> +EOF > I like the idea of allowing the user to select multiple profiles. > However, there also needs to be a clean and simple way to select a > single profile without going through the list and deselecting everything. > Also, I don't like hardcoded profile names in metadata.pl, a better > approach would be to have a flag as part of a profile that indicates > that it's not a device profile. > > - Felix >
On 2016-01-11 21:23, Daniel Dickinson wrote: > Actually I thought of a solution, but it starts to get ugly. A choice > submemenu with non-duplicate (i.e. TARGET_SINGLE_...) symbols that also > select TARGET_SINGLE, AND have all the normal TARGET_... depend on > !(TARGET_SYMBOL && !TARGET_SINGLE_..profile). > > That would probably work, but it's rather ugly IMO. This is close to what I had in mind in my earlier response. Yes, it's ugly, but it's a lot less ugly than taking your approach without dealing with the issues that I pointed out. - Felix
On 2016-01-11 20:27, Eric Schultz wrote: > Felix, > > Would it be unreasonable to have overridable defaults like suggested in > metadata.pl? Convention over configuration and all that. I don't understand what you're asking. Could you elaborate? - Felix
On 11/01/16 04:05 PM, Felix Fietkau wrote: > On 2016-01-11 20:27, Eric Schultz wrote: >> Felix, >> >> Would it be unreasonable to have overridable defaults like suggested in >> metadata.pl? Convention over configuration and all that. > I don't understand what you're asking. Could you elaborate? To be honest I think you were supporting my effort, but I'm also not clear on quite what you were trying to say. Regards, Daniel
Hi Felix, On 11/01/16 03:28 PM, Felix Fietkau wrote: > This is close to what I had in mind in my earlier response. Yes, it's > ugly, but it's a lot less ugly than taking your approach without dealing > with the issues that I pointed out. Since a bit of ugliness for improved user experience seems to be acceptable, how do you feel about the following proposal to deal with current brokenness of ALL and ALL_KMODS (specifically that once there is an existing .config that selecting/deselecting those options no longer does anything; that is they only work when starting with no .config): Make ALL a configuration option created by metadata.pl that does select <package> for all packages (except kmods), and ALL_KMODS a configuration option create by metadata.pl that does select <kmod-package> for all kmods and is default y if ALL. This generated config could go under tmp and be sourced by Config-build.in in the current location of ALL and ALL_KMODS. Even though it's kind of ugly, since you dislike the similar issues with profile selection as menu enough to accept some ugliness, I'm wondering if you could accept some ugliness to make ALL and ALL_KMODS work better too (as I'm not sure you'd even realized there was in issue before). Regards, Daniel
On 2016-01-12 03:53, Daniel Dickinson wrote: > Hi Felix, > > On 11/01/16 03:28 PM, Felix Fietkau wrote: > > This is close to what I had in mind in my earlier response. Yes, it's > > ugly, but it's a lot less ugly than taking your approach without dealing > > with the issues that I pointed out. > > Since a bit of ugliness for improved user experience seems to be > acceptable, how do you feel about the following proposal to deal with > current brokenness of ALL and ALL_KMODS (specifically that once there is > an existing .config that selecting/deselecting those options no longer > does anything; that is they only work when starting with no .config): > > Make ALL a configuration option created by metadata.pl that does select > <package> for all packages (except kmods), and > ALL_KMODS a configuration option create by metadata.pl that does select > <kmod-package> for all kmods and is default y if ALL. > > This generated config could go under tmp and be sourced by > Config-build.in in the current location of ALL and ALL_KMODS. > > Even though it's kind of ugly, since you dislike the similar issues with > profile selection as menu enough to accept some ugliness, I'm wondering > if you could accept some ugliness to make ALL and ALL_KMODS work better > too (as I'm not sure you'd even realized there was in issue before). Of course I realized this issue pretty much from the beginning when we added support for it to the tree. The problem with forcing ALL and ALL_KMODS to select packages is that it makes it difficult to deselect a package afterwards. Also unlike the profile case, those options are typically selected when you start building a configuration for a device, not so much after you've already configured everything. So while there are superficial similarities with the profile issue, there are enough differences in how it gets used by users that we should treat it differently. - Felix
Hi, I'm looking at the 'Default' profiles in architectures and as far as I can tell the only architecture that currently uses Default to mean 'build all profiles for this arch-subtarget'. Is that correct. I've seen other 00-default.mk profiles by on most archs this seems to be used as a generic profile rather than as a way to select more than one. Is that actually correct, or am I just missing how e.g. brcm63xx selects more than than one profile in response to the default profile (I use that example because it has both a default and generic profiles, but I don't see how default builds more than single image). Any clarity on this? Regards, Daniel On 11/01/16 08:35 AM, Felix Fietkau wrote: > On 2016-01-11 06:16, openwrt@daniel.thecshore.com wrote: >> From: Daniel Dickinson <openwrt@daniel.thecshore.com> >> >> Certain platforms have large numbers of possible images, and it can be >> desirable to build neither all images nor only a single image, >> therefore this patch makes selecting target profiles a menu instead of a >> single choice, which allows the user to build a specific subset of all >> possible images for a target. >> >> Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com> >> --- >> scripts/metadata.pl | 32 +++++++++++++++++++++++++++++--- >> 1 file changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/metadata.pl b/scripts/metadata.pl >> index 48b1b7a..4487d26 100755 >> --- a/scripts/metadata.pl >> +++ b/scripts/metadata.pl >> @@ -275,8 +275,7 @@ EOF >> print <<EOF; >> endchoice >> >> -choice >> - prompt "Target Profile" >> +menu "Target Profile" >> >> EOF >> >> @@ -288,8 +287,35 @@ EOF >> config TARGET_$target->{conf}_$profile->{id} >> bool "$profile->{name}" >> depends on TARGET_$target->{conf} >> +EOF >> + >> + if (not (($profile->{id} eq 'Default') || ($profile->{id} eq 'Minimal'))) { >> + print <<EOF; >> + default y if TARGET_$target->{conf}_Default >> + default n if TARGET_$target->{conf}_Minimal >> +EOF > I like the idea of allowing the user to select multiple profiles. > However, there also needs to be a clean and simple way to select a > single profile without going through the list and deselecting everything. > Also, I don't like hardcoded profile names in metadata.pl, a better > approach would be to have a flag as part of a profile that indicates > that it's not a device profile. > > - Felix >
On 2016-01-13 18:04, Daniel Dickinson wrote: > Hi, > > I'm looking at the 'Default' profiles in architectures and as far as I > can tell the only architecture that currently uses Default to mean > 'build all profiles for this arch-subtarget'. Is that correct. > > I've seen other 00-default.mk profiles by on most archs this seems to be > used as a generic profile rather than as a way to select more than one. > > Is that actually correct, or am I just missing how e.g. brcm63xx selects > more than than one profile in response to the default profile (I use > that example because it has both a default and generic profiles, but I > don't see how default builds more than single image). Sometimes there are subtle differences, e.g. on ar71xx it does not include ath10k which is only needed by a few devices. - Felix
Sorry, I wasn clear I guess. I mean I'm not clear on how or if other targets use the 'Default' profile to select building all images for that target. That is, my question is whether ar71xx is the only target for which the Default profile selects building all available images. Regards, Daniel On 13/01/16 12:26 PM, Felix Fietkau wrote: > On 2016-01-13 18:04, Daniel Dickinson wrote: >> Hi, >> >> I'm looking at the 'Default' profiles in architectures and as far as I >> can tell the only architecture that currently uses Default to mean >> 'build all profiles for this arch-subtarget'. Is that correct. >> >> I've seen other 00-default.mk profiles by on most archs this seems to be >> used as a generic profile rather than as a way to select more than one. >> >> Is that actually correct, or am I just missing how e.g. brcm63xx selects >> more than than one profile in response to the default profile (I use >> that example because it has both a default and generic profiles, but I >> don't see how default builds more than single image). > Sometimes there are subtle differences, e.g. on ar71xx it does not > include ath10k which is only needed by a few devices. > > - Felix >
On 2016-01-13 18:38, Daniel Dickinson wrote: > Sorry, I wasn clear I guess. I mean I'm not clear on how or if other > targets use the 'Default' profile to select building all images for that > target. That is, my question is whether ar71xx is the only target for > which the Default profile selects building all available images. I think having the Default profile build all images is not ar71xx specific. - Felix
diff --git a/scripts/metadata.pl b/scripts/metadata.pl index 48b1b7a..4487d26 100755 --- a/scripts/metadata.pl +++ b/scripts/metadata.pl @@ -275,8 +275,7 @@ EOF print <<EOF; endchoice -choice - prompt "Target Profile" +menu "Target Profile" EOF @@ -288,8 +287,35 @@ EOF config TARGET_$target->{conf}_$profile->{id} bool "$profile->{name}" depends on TARGET_$target->{conf} +EOF + + if (not (($profile->{id} eq 'Default') || ($profile->{id} eq 'Minimal'))) { + print <<EOF; + default y if TARGET_$target->{conf}_Default + default n if TARGET_$target->{conf}_Minimal +EOF + + + } + if ($profile->{id} eq 'Default') { + print <<EOF; + depends on !TARGET_$target->{conf}_Minimal + default y +EOF + + } + + if ($profile->{id} eq 'Minimal') { + print <<EOF; + default n +EOF + + } + + print <<EOF; $profile->{config} EOF + $profile->{kconfig} and print "\tselect PROFILE_KCONFIG\n"; my @pkglist = merge_package_lists($target->{packages}, $profile->{packages}); foreach my $pkg (@pkglist) { @@ -308,7 +334,7 @@ EOF } print <<EOF; -endchoice +endmenu config HAS_SUBTARGETS bool