| Message ID | a7fcca12e0f567efd29314172ccf1ad4cd279bf7.1689759351.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/nolibc: add minimal kernel config support - part1 | expand |
On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote: > The default DEFCONFIG_<ARCH> may not always work for all architectures, > let's allow users to tune some kernel config options with 'menuconfig'. > > This is important when porting nolibc to a new architecture, it also > allows to speed up nolibc 'run' target testing via manually disabling > tons of unnecessary kernel config options. > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > --- > tools/testing/selftests/nolibc/Makefile | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > index 058e7be070ea..06954b4b3885 100644 > --- a/tools/testing/selftests/nolibc/Makefile > +++ b/tools/testing/selftests/nolibc/Makefile > @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) > defconfig: > $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare > > +menuconfig: > + $(Q)$(MAKE_KERNEL) menuconfig What is the real benefit of this compared to just running the standard "make menuconfig" ? We should avoid adding makefile targets that do not bring value on top of that the top-level makefile does, because it will make the useful ones much harder to spot, and will tend to encourage users to use only that makefile during the tests, which is a bad practice since many targets will always be missing or work differently. It's important in my opinion that we strictly stick to what is useful to operate the tests themselves and this one doesn't make me feel like it qualifies as such. Willy
> On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote: > > The default DEFCONFIG_<ARCH> may not always work for all architectures, > > let's allow users to tune some kernel config options with 'menuconfig'. > > > > This is important when porting nolibc to a new architecture, it also > > allows to speed up nolibc 'run' target testing via manually disabling > > tons of unnecessary kernel config options. > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > --- > > tools/testing/selftests/nolibc/Makefile | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > > index 058e7be070ea..06954b4b3885 100644 > > --- a/tools/testing/selftests/nolibc/Makefile > > +++ b/tools/testing/selftests/nolibc/Makefile > > @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) > > defconfig: > > $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare > > > > +menuconfig: > > + $(Q)$(MAKE_KERNEL) menuconfig > > What is the real benefit of this compared to just running the > standard "make menuconfig" ? We should avoid adding makefile targets > that do not bring value on top of that the top-level makefile does, > because it will make the useful ones much harder to spot, and will > tend to encourage users to use only that makefile during the tests, > which is a bad practice since many targets will always be missing > or work differently. It's important in my opinion that we strictly > stick to what is useful to operate the tests themselves and this > one doesn't make me feel like it qualifies as such. Ok, get it. I did like develop nolibc in tools/testing/selftests/nolibc/ without changing directories frequently or specifying the "-C" option unnecessary ;-) Since "make menuconfig" is only required during the first porting of a new architecture, so, it is ok to drop this patch. Thanks, Zhangjin > > Willy
Hi, Willy > > On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote: > > > The default DEFCONFIG_<ARCH> may not always work for all architectures, > > > let's allow users to tune some kernel config options with 'menuconfig'. > > > > > > This is important when porting nolibc to a new architecture, it also > > > allows to speed up nolibc 'run' target testing via manually disabling > > > tons of unnecessary kernel config options. > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > > --- > > > tools/testing/selftests/nolibc/Makefile | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > > > index 058e7be070ea..06954b4b3885 100644 > > > --- a/tools/testing/selftests/nolibc/Makefile > > > +++ b/tools/testing/selftests/nolibc/Makefile > > > @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) > > > defconfig: > > > $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare > > > > > > +menuconfig: > > > + $(Q)$(MAKE_KERNEL) menuconfig > > > > What is the real benefit of this compared to just running the > > standard "make menuconfig" ? We should avoid adding makefile targets > > that do not bring value on top of that the top-level makefile does, > > because it will make the useful ones much harder to spot, and will > > tend to encourage users to use only that makefile during the tests, > > which is a bad practice since many targets will always be missing > > or work differently. It's important in my opinion that we strictly > > stick to what is useful to operate the tests themselves and this > > one doesn't make me feel like it qualifies as such. > > Ok, get it. > > I did like develop nolibc in tools/testing/selftests/nolibc/ without > changing directories frequently or specifying the "-C" option > unnecessary ;-) > > Since "make menuconfig" is only required during the first porting of a new > architecture, so, it is ok to drop this patch. > Oh, Willy, I did find this is very important again. I just want to check and test if we need to disable vsx for 32-bit powerpc too, so, I plan to re-configure kernel via menuconfig to disable VSX in kernel side, and forcely enable vsx in application side, but it was very 'painful' when I was running something like this: $ make defconfig ARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- To do a further menuconfig, I must switch to something else: $ make menuconfig ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -C ../../../../ Especially, our test is able to accept ARCH=ppc, but the top-level kernel still only accept powerpc, it makes the development very comfortable, of course, the typing of the relative or absolute path every time is either not a good idea. so, this doesn't simply duplicate with the one from top-level, it can get the right ARCH, srctree for us, and this is heavily used by our tinyconfig development to tune the config options very frequently, so, let's still add this one in the new revision? But I plan to merge the mrproper targets here to save another patch, what about your idea? menuconfig mrproper: $(Q)$(MAKE_KERNEL) $@ Thanks, Zhangjin > Thanks, > Zhangjin > > > > > Willy
Hi Zhangjin, On Thu, Jul 27, 2023 at 09:24:18PM +0800, Zhangjin Wu wrote: > Hi, Willy > > > > On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote: > > > > The default DEFCONFIG_<ARCH> may not always work for all architectures, > > > > let's allow users to tune some kernel config options with 'menuconfig'. > > > > > > > > This is important when porting nolibc to a new architecture, it also > > > > allows to speed up nolibc 'run' target testing via manually disabling > > > > tons of unnecessary kernel config options. > > > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > > > --- > > > > tools/testing/selftests/nolibc/Makefile | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > > > > index 058e7be070ea..06954b4b3885 100644 > > > > --- a/tools/testing/selftests/nolibc/Makefile > > > > +++ b/tools/testing/selftests/nolibc/Makefile > > > > @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) > > > > defconfig: > > > > $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare > > > > > > > > +menuconfig: > > > > + $(Q)$(MAKE_KERNEL) menuconfig > > > > > > What is the real benefit of this compared to just running the > > > standard "make menuconfig" ? We should avoid adding makefile targets > > > that do not bring value on top of that the top-level makefile does, > > > because it will make the useful ones much harder to spot, and will > > > tend to encourage users to use only that makefile during the tests, > > > which is a bad practice since many targets will always be missing > > > or work differently. It's important in my opinion that we strictly > > > stick to what is useful to operate the tests themselves and this > > > one doesn't make me feel like it qualifies as such. > > > > Ok, get it. > > > > I did like develop nolibc in tools/testing/selftests/nolibc/ without > > changing directories frequently or specifying the "-C" option > > unnecessary ;-) > > > > Since "make menuconfig" is only required during the first porting of a new > > architecture, so, it is ok to drop this patch. > > > > Oh, Willy, I did find this is very important again. > > I just want to check and test if we need to disable vsx for 32-bit > powerpc too, so, I plan to re-configure kernel via menuconfig to disable > VSX in kernel side, and forcely enable vsx in application side, but it > was very 'painful' when I was running something like this: > > $ make defconfig ARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- > > To do a further menuconfig, I must switch to something else: > > $ make menuconfig ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -C ../../../../ > > Especially, our test is able to accept ARCH=ppc, but the top-level > kernel still only accept powerpc, it makes the development very > comfortable, of course, the typing of the relative or absolute path > every time is either not a good idea. It is "able" but that's not what ought to be used, since it's going to be remapped to "powerpc". You're supposed to use XARCH for this one since it's a variant (we could find other names if needed). I suspect it just shows that the "override ARCH :=" is excessive and should not be there. Usually when you put override in a makefile, it's going to pop up as a bug elsewhere. Thus actually you could very well have : make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- as the prefix for all your commands. Some will prefer to work directly from the kernel dir: $ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- -C tools/testing/selftests/nolibc-test defconfig $ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- menuconfig Others would do it from the nolibc-test dir as you did above. > so, this doesn't simply duplicate with the one from top-level, it can > get the right ARCH, srctree for us, and this is heavily used by our > tinyconfig development to tune the config options very frequently, so, > let's still add this one in the new revision? I'm not *fundamentally* opposed to menuconfig, I just think that it's appearing at the wrong place. Why not oldconfig, allmodconfig, randconfig, allnoconfig etc then ? There is nothing in the test directory that is supposed to be used interactively, either it's run in a batch for cross- arch testing, or you use it to validate your kernel when you're working on it. It feels strange that one would want to configure their kernel from this sub-dir. > But I plan to merge the mrproper targets here to save another patch, > what about your idea? > > menuconfig mrproper: > $(Q)$(MAKE_KERNEL) $@ Please no. Someone doing that in hope to clean only the result of the tests would in fact ruin their config: $ make -C tools/testing/selftests/nolibc-test mrproper This is another reason for not encouraging users to develop from within that dir. Willy
Hi, Willy [...] > > > > > > > > What is the real benefit of this compared to just running the > > > > standard "make menuconfig" ? We should avoid adding makefile targets > > > > that do not bring value on top of that the top-level makefile does, > > > > because it will make the useful ones much harder to spot, and will > > > > tend to encourage users to use only that makefile during the tests, > > > > which is a bad practice since many targets will always be missing > > > > or work differently. It's important in my opinion that we strictly > > > > stick to what is useful to operate the tests themselves and this > > > > one doesn't make me feel like it qualifies as such. > > > > > > Ok, get it. > > > > > > I did like develop nolibc in tools/testing/selftests/nolibc/ without > > > changing directories frequently or specifying the "-C" option > > > unnecessary ;-) > > > > > > Since "make menuconfig" is only required during the first porting of a new > > > architecture, so, it is ok to drop this patch. > > > > > > > Oh, Willy, I did find this is very important again. > > > > I just want to check and test if we need to disable vsx for 32-bit > > powerpc too, so, I plan to re-configure kernel via menuconfig to disable > > VSX in kernel side, and forcely enable vsx in application side, but it > > was very 'painful' when I was running something like this: > > > > $ make defconfig ARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- > > > > To do a further menuconfig, I must switch to something else: > > > > $ make menuconfig ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -C ../../../../ > > > > Especially, our test is able to accept ARCH=ppc, but the top-level > > kernel still only accept powerpc, it makes the development very > > comfortable, of course, the typing of the relative or absolute path > > every time is either not a good idea. > > It is "able" but that's not what ought to be used, since it's going to > be remapped to "powerpc". You're supposed to use XARCH for this one > since it's a variant (we could find other names if needed). I suspect > it just shows that the "override ARCH :=" is excessive and should not > be there. Usually when you put override in a makefile, it's going to > pop up as a bug elsewhere. Thus actually you could very well have : > > make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- > Oh, Seems I have misunderstood your suggestions in another reply, you recommended to add ARCH variable to the help target, at last, I thought it was better to only reserve ARCH as the consistent variable to users, so, the 'override' keyword is added to allow passing anyone of powerpc, ppc, ppc64 and ppc64le to ARCH, since XARCH only accept ppc, ppc64 and ppc64le. Further passing 'ARCH=powerpc XARCH=ppc' is also ok, but the combination of ARCH and XARCH may require more time to teach a new user, is ok for me to remove the override and carefully document this usage in Makefile. Seems 'override' is really required, without it, when users wrongly try ARCH=ppc, ARCH=ppc64, ARCH=ppc64le, the ARCH passed to kernel will be completely wrong, it may mislead users ... With this 'override', it is ok for users to use ARCH only, or XARCH or even together with both of them, no failure and happy, to myself, I'm ok to remove the 'override' keyword, ;-) > as the prefix for all your commands. Some will prefer to work directly > from the kernel dir: > > $ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- -C tools/testing/selftests/nolibc-test defconfig > $ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- menuconfig > > Others would do it from the nolibc-test dir as you did above. > Yeah, we still need one more -C, but it is of course ok to do this in a standalone script as you mentioned before. > > so, this doesn't simply duplicate with the one from top-level, it can > > get the right ARCH, srctree for us, and this is heavily used by our > > tinyconfig development to tune the config options very frequently, so, > > let's still add this one in the new revision? > > I'm not *fundamentally* opposed to menuconfig, I just think that it's > appearing at the wrong place. Why not oldconfig, allmodconfig, randconfig, > allnoconfig etc then ? There is nothing in the test directory that is > supposed to be used interactively, either it's run in a batch for cross- > arch testing, or you use it to validate your kernel when you're working > on it. It feels strange that one would want to configure their kernel > from this sub-dir. Ok, it is reasonable ;-) > > > But I plan to merge the mrproper targets here to save another patch, > > what about your idea? > > > > menuconfig mrproper: > > $(Q)$(MAKE_KERNEL) $@ > > Please no. Someone doing that in hope to clean only the result of > the tests would in fact ruin their config: > > $ make -C tools/testing/selftests/nolibc-test mrproper > Yeah, really a good reason to not add mrproper there. > This is another reason for not encouraging users to develop from > within that dir. > Thanks, Zhangjin > Willy
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 058e7be070ea..06954b4b3885 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare +menuconfig: + $(Q)$(MAKE_KERNEL) menuconfig + extconfig: $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
The default DEFCONFIG_<ARCH> may not always work for all architectures, let's allow users to tune some kernel config options with 'menuconfig'. This is important when porting nolibc to a new architecture, it also allows to speed up nolibc 'run' target testing via manually disabling tons of unnecessary kernel config options. Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)