| Message ID | 1539989903-19803-1-git-send-email-Afshin.Nasser@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | merge_config.sh: Fix finding redundant config mechanism | expand |
Nasser, On Fri, Oct 19, 2018 at 11:58 PM Nasser <afshin.nasser@gmail.com> wrote: > > We use BR2_* style for configuration variables in buildroot so we should > use this style when extracting configuration options. Otherwise CFG_LIST > will almost always be empty. > > The CONFIG_* style has been taken form the Linux kernel and is not > appropriate in this context. > The merge_config.sh is used for a couple scenarios - Appending kconfigs together (CONFIG_*) - Buildroot cfgs for runtime tests (BR2_*) - As a tool by users to merge together Buildroot configs I'm not sure of the cleanest approach to support both - you could detect if the file is one or the other and adjust the regex - do the inverse and build a list of lines that are not comments Matt
On 20/10/2018 15:56, Matthew Weber wrote: > Nasser, > > On Fri, Oct 19, 2018 at 11:58 PM Nasser <afshin.nasser@gmail.com> wrote: >> >> We use BR2_* style for configuration variables in buildroot so we should >> use this style when extracting configuration options. Otherwise CFG_LIST >> will almost always be empty. >> >> The CONFIG_* style has been taken form the Linux kernel and is not >> appropriate in this context. A similar patch was actually submitted earlier by Angelo [1]. I commented on that that it should be simplified like you propose. However... >> > > The merge_config.sh is used for a couple scenarios > - Appending kconfigs together (CONFIG_*) ... I forgot about this one. Indeed, the buildroot merge_config.sh script is used in pkg-kconfig.mk to merge kernel configs. Ideally we should change that to use the package's merge_config.sh script. However, the location of that script may vary, or it may even be missing... > - Buildroot cfgs for runtime tests (BR2_*) > - As a tool by users to merge together Buildroot configs > > I'm not sure of the cleanest approach to support both > - you could detect if the file is one or the other and adjust the regex > - do the inverse and build a list of lines that are not comments I think Angelo's patch is the best approach after all. However, there is one comment that I made on Angelo's patch that still applies here: Since the kconfig stuff comes from upstream but is modified, we also maintain the changes as a stack of patches in support/kconfig/patches. So you should generate a new patch for this change and add it to the series file. I'm not sure why we don't use a vendor branch and just merge, but that's the way we do it :-). Also, Nasser, we require your Signed-off-by to contain your full name. The Signed-off-by is a short way for you to assert that you are entitled to contribute the patch under buildroot's GPL license. See http://elinux.org/Developer_Certificate_Of_Origin for more details. Itis an official statement, so it should be done with your full and real name. And please make sure that the author information matches the Signed-off-by. Regards, Arnout [1] http://patchwork.ozlabs.org/patch/824051/
Hi, > > The merge_config.sh is used for a couple scenarios > > - Appending kconfigs together (CONFIG_*) > ... I forgot about this one. Indeed, the buildroot merge_config.sh script is > used in pkg-kconfig.mk to merge kernel configs. Ideally we should change that to > use the package's merge_config.sh script. However, the location of that script > may vary, or it may even be missing... > > - Buildroot cfgs for runtime tests (BR2_*) > > - As a tool by users to merge together Buildroot configs > > I'm not sure of the cleanest approach to support both > > - you could detect if the file is one or the other and adjust the regex > > - do the inverse and build a list of lines that are not comments > I think Angelo's patch is the best approach after all. +1. I also think keep using our merge_config.sh with -b switch from Angelo's patch [1] is the way. > However, there is one comment that I made on Angelo's patch that still applies > here: > Since the kconfig stuff comes from upstream but is modified, we also maintain > the changes as a stack of patches in support/kconfig/patches. So you should > generate a new patch for this change and add it to the series file. I'm not sure > why we don't use a vendor branch and just merge, but that's the way we do it :-). IMHO it's a common practice across distributions packaging to use quilt for own patches. I guess it's to help show easily distro changes (in our case patches in support/kconfig/patches/ directory are in tarballs, not just in git). Kind regards, Petr > Regards, > Arnout > [1] http://patchwork.ozlabs.org/patch/824051/
Petr, On Sun, Oct 21, 2018 at 6:27 PM Petr Vorel <petr.vorel@gmail.com> wrote: > > Hi, > > > > The merge_config.sh is used for a couple scenarios > > > - Appending kconfigs together (CONFIG_*) > > > ... I forgot about this one. Indeed, the buildroot merge_config.sh script is > > used in pkg-kconfig.mk to merge kernel configs. Ideally we should change that to > > use the package's merge_config.sh script. However, the location of that script > > may vary, or it may even be missing... > > > > - Buildroot cfgs for runtime tests (BR2_*) > > > - As a tool by users to merge together Buildroot configs > > > > I'm not sure of the cleanest approach to support both > > > - you could detect if the file is one or the other and adjust the regex > > > - do the inverse and build a list of lines that are not comments > > > I think Angelo's patch is the best approach after all. > +1. I also think keep using our merge_config.sh with -b switch from Angelo's > patch [1] is the way. Do you think you'll be able to provide an updated patch based on Angelo's work? > > > However, there is one comment that I made on Angelo's patch that still applies > > here: > > > Since the kconfig stuff comes from upstream but is modified, we also maintain > > the changes as a stack of patches in support/kconfig/patches. So you should > > generate a new patch for this change and add it to the series file. I'm not sure > > why we don't use a vendor branch and just merge, but that's the way we do it :-). > IMHO it's a common practice across distributions packaging to use quilt for own > patches. I guess it's to help show easily distro changes (in our case patches in > support/kconfig/patches/ directory are in tarballs, not just in git). > >
Hi Matthew, Angelo, > Petr, > On Sun, Oct 21, 2018 at 6:27 PM Petr Vorel <petr.vorel@gmail.com> wrote: > > Hi, > > > > The merge_config.sh is used for a couple scenarios > > > > - Appending kconfigs together (CONFIG_*) > > > ... I forgot about this one. Indeed, the buildroot merge_config.sh script is > > > used in pkg-kconfig.mk to merge kernel configs. Ideally we should change that to > > > use the package's merge_config.sh script. However, the location of that script > > > may vary, or it may even be missing... > > > > - Buildroot cfgs for runtime tests (BR2_*) > > > > - As a tool by users to merge together Buildroot configs > > > > I'm not sure of the cleanest approach to support both > > > > - you could detect if the file is one or the other and adjust the regex > > > > - do the inverse and build a list of lines that are not comments > > > I think Angelo's patch is the best approach after all. > > +1. I also think keep using our merge_config.sh with -b switch from Angelo's > > patch [1] is the way. > Do you think you'll be able to provide an updated patch based on Angelo's work? Matthew, sure I can have look into it. Unless Angelo is planning to finish it. Kind regards, Petr
Hi Arnout, On Sat, Oct 20, 2018 at 05:01:33PM +0100, Arnout Vandecappelle wrote: > > > On 20/10/2018 15:56, Matthew Weber wrote: > > Nasser, > > > > On Fri, Oct 19, 2018 at 11:58 PM Nasser <afshin.nasser@gmail.com> wrote: > >> > >> We use BR2_* style for configuration variables in buildroot so we should > >> use this style when extracting configuration options. Otherwise CFG_LIST > >> will almost always be empty. > >> > >> The CONFIG_* style has been taken form the Linux kernel and is not > >> appropriate in this context. > > A similar patch was actually submitted earlier by Angelo [1]. I commented on > that that it should be simplified like you propose. However... > > >> > > > > The merge_config.sh is used for a couple scenarios > > - Appending kconfigs together (CONFIG_*) > > ... I forgot about this one. Indeed, the buildroot merge_config.sh script is > used in pkg-kconfig.mk to merge kernel configs. Ideally we should change that to > use the package's merge_config.sh script. However, the location of that script > may vary, or it may even be missing... > > > - Buildroot cfgs for runtime tests (BR2_*) > > - As a tool by users to merge together Buildroot configs > > > > I'm not sure of the cleanest approach to support both > > - you could detect if the file is one or the other and adjust the regex > > - do the inverse and build a list of lines that are not comments > > I think Angelo's patch is the best approach after all. I agree with you. The -b switch can be used to have a general solution for both cases. > > However, there is one comment that I made on Angelo's patch that still applies > here: > > Since the kconfig stuff comes from upstream but is modified, we also maintain > the changes as a stack of patches in support/kconfig/patches. So you should > generate a new patch for this change and add it to the series file. I'm not sure > why we don't use a vendor branch and just merge, but that's the way we do it :-). > > > Also, Nasser, we require your Signed-off-by to contain your full name. The > Signed-off-by is a short way for you to assert that you are entitled to > contribute the patch under buildroot's GPL license. See > http://elinux.org/Developer_Certificate_Of_Origin for more details. Itis an > official statement, so it should be done with your full and real name. And > please make sure that the author information matches the Signed-off-by. Sorry, this was a miss-configuration on the machine I've used to send the patch. I'll correct my Signed-off-by clause. Should I resend the patch applying Angelo -b approach? > > > Regards, > Arnout > > [1] http://patchwork.ozlabs.org/patch/824051/ > Regards, Nasser Afshin
On 10/23/18 4:19 PM, Nasser Afshin wrote: > Hi Arnout, > > On Sat, Oct 20, 2018 at 05:01:33PM +0100, Arnout Vandecappelle wrote: [snip] >> I think Angelo's patch is the best approach after all. > > I agree with you. The -b switch can be used to have a general solution > for both cases. > >> >> However, there is one comment that I made on Angelo's patch that still applies >> here: >> >> Since the kconfig stuff comes from upstream but is modified, we also maintain >> the changes as a stack of patches in support/kconfig/patches. So you should >> generate a new patch for this change and add it to the series file. I'm not sure >> why we don't use a vendor branch and just merge, but that's the way we do it :-). >> >> >> Also, Nasser, we require your Signed-off-by to contain your full name. The >> Signed-off-by is a short way for you to assert that you are entitled to >> contribute the patch under buildroot's GPL license. See >> http://elinux.org/Developer_Certificate_Of_Origin for more details. Itis an >> official statement, so it should be done with your full and real name. And >> please make sure that the author information matches the Signed-off-by. > > Sorry, this was a miss-configuration on the machine I've used to send the > patch. I'll correct my Signed-off-by clause. > > Should I resend the patch applying Angelo -b approach? Yes, please do. Make sure you keep Angelo's authorship and add your Sob. And please fix the remaining two comments I made: - remove the hunk that replaces alldefconfig with olddefconfig; - a patch should be added to support/kconfig/patches. As it happens, I talked today with the guy who submitted the merge_config.sh script to the kernel (apparently this script comes from yocto, originally). He thinks it would be a good idea to upstream this -b option to the kernel. I didn't find the kconfig maintainer (Masahiro) though, but it's worth a try. Regards, Arnout >> >> >> Regards, >> Arnout >> >> [1] http://patchwork.ozlabs.org/patch/824051/ >> > > Regards, > Nasser Afshin >
Hi Arnout, Nasser, > > Should I resend the patch applying Angelo -b approach? > Yes, please do. Make sure you keep Angelo's authorship and add your Sob. Nice. So I'm not going to implement it (asked by Arnout). If you CC me, I'll review your patch. > And please fix the remaining two comments I made: > - remove the hunk that replaces alldefconfig with olddefconfig; > - a patch should be added to support/kconfig/patches. > As it happens, I talked today with the guy who submitted the merge_config.sh > script to the kernel (apparently this script comes from yocto, originally). He > thinks it would be a good idea to upstream this -b option to the kernel. I > didn't find the kconfig maintainer (Masahiro) though, but it's worth a try. I was planning to send a patch to linux-kbuild as well (after we complete this patch) as I also think it's something which upstream could take, so projects didn't have to carry patch for it. But feel free to implement send it there. > Regards, > Arnout Kind regards, Petr
Hi Petr On Wed, Oct 24, 2018 at 09:15:39PM +0200, Petr Vorel wrote: > Hi Arnout, Nasser, > > > > Should I resend the patch applying Angelo -b approach? > > Yes, please do. Make sure you keep Angelo's authorship and add your Sob. > Nice. So I'm not going to implement it (asked by Arnout). > If you CC me, I'll review your patch. > Sure, I'm preparing the patch. I'll send it here when it's ready. I appreciate your review and help. > > And please fix the remaining two comments I made: > > > - remove the hunk that replaces alldefconfig with olddefconfig; > > > - a patch should be added to support/kconfig/patches. > > > > As it happens, I talked today with the guy who submitted the merge_config.sh > > script to the kernel (apparently this script comes from yocto, originally). He > > thinks it would be a good idea to upstream this -b option to the kernel. I > > didn't find the kconfig maintainer (Masahiro) though, but it's worth a try. > I was planning to send a patch to linux-kbuild as well (after we complete this > patch) as I also think it's something which upstream could take, so projects > didn't have to carry patch for it. But feel free to implement send it there. > OK. > > Regards, > > Arnout > > Kind regards, > Petr Kind regards, Nasser
diff --git a/support/kconfig/merge_config.sh b/support/kconfig/merge_config.sh index 67d1314..87a8d02 100755 --- a/support/kconfig/merge_config.sh +++ b/support/kconfig/merge_config.sh @@ -99,7 +99,7 @@ if [ ! -r "$INITFILE" ]; then fi MERGE_LIST=$* -SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(CONFIG_[a-zA-Z0-9_]*\)[= ].*/\2/p" +SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(BR2_[a-zA-Z0-9_]*\)[= ].*/\2/p" TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX) echo "Using $INITFILE as base"
We use BR2_* style for configuration variables in buildroot so we should use this style when extracting configuration options. Otherwise CFG_LIST will almost always be empty. The CONFIG_* style has been taken form the Linux kernel and is not appropriate in this context. Signed-off-by: Nasser <Afshin.Nasser@gmail.com> --- If you add 'set -x' in the beginning of the script you can see that $CFG_LIST is always empty. When having redundancy in the configuration fragments, you can see that the $TMP_FILE file has redundant configuration options when calling 'make'. Note that after applying this patch, calling 'make' in this script, will not produce any warnings due to redundancy issues anymore. This is because now repeated options are all properly omitted before calling 'make'. After applying this patch '-r -m' options work as expected. Further more, note that without this patch, any inconsistent re-assignment is not reported by this script. Although there are some warnings if you did not specify -m option. They are all 'make' reports. support/kconfig/merge_config.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)