| Message ID | 20181101010953.20996-1-afshin.nasser@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | merge_config.sh: Fix merging buildroot config files | expand |
Hi. > -----Original Message----- > From: buildroot [mailto:buildroot-bounces@busybox.net] On Behalf Of > Nasser Afshin > Sent: Thursday, November 01, 2018 10:10 AM > To: Petr Vorel <petr.vorel@gmail.com> > Cc: Nasser Afshin <afshin.nasser@gmail.com>; buildroot > <buildroot@buildroot.org> > Subject: [Buildroot] [PATCH] merge_config.sh: Fix merging buildroot config > files > > This patch allows us to define config prefix with CONFIG_ environment > variable. Now we can define BR2_ prefix when we are going to merge > buildroot fragment configs. > > By setting the proper config prefix, we will have proper 'redundant > configuration warnings' when we use '-r -m' options. > > This is actually an upstream patch [1] from linux/kbuild project. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild > .git/commit/?h=for-next&id=f6fe19e9d84400e1c9544c1909952c2c7c6a90a2 Please refer to commit ID only after it gets stable in Linus' tree. I will end up with rebasing In order to drop some other commits causing a problem. Thanks.
Hi. > > This is actually an upstream patch [1] from linux/kbuild project. > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild > > .git/commit/?h=for-next&id=f6fe19e9d84400e1c9544c1909952c2c7c6a90a2 > Please refer to commit ID only after it gets stable in Linus' tree. > I will end up with rebasing > In order to drop some other commits causing a problem. Make sense, thanks, Yamada! Nasser, maybe use link to patch in patchwork. I'd put this link also into the patch itself (IMHO good idea for faster resolving when doing kconfig update in buildroot). Once the patch gets into mainline, we can update the link. Kind regards, Petr [1] https://patchwork.kernel.org/patch/10660207/
Hi Nasser, ... > @@ -157,6 +162,7 @@ fi > # Use the merged file as the starting point for: > # alldefconfig: Fills in any missing symbols with Kconfig default > # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set > +unset CONFIG_ You added unset, which isn't in upstream patch. This has no effect, so I'd be for removing it. Even if it has (if it was needed), it'd be better to 1) ask upstream to add it 2) keep it in separate patch in meanwhile (otherwise it complicates patch rebasing during update and can be lost). ... > diff --git a/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch b/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch ... > + # Use the merged file as the starting point for: > + # alldefconfig: Fills in any missing symbols with Kconfig default > + # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set > ++unset CONFIG_ BTW (I noted that before) your patch contain trailing whitespace and mixing tab and spaces. git am and pwclient fixes that, only when applying with patch they get committed, so nothing serious. $ pwclient git-am -p buildroot 991781 .git/rebase-apply/patch:59: space before tab in indent. echo " -r list redundant entries when merging fragments" .git/rebase-apply/patch:60: space before tab in indent. echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." .git/rebase-apply/patch:61: space before tab in indent. echo " -e colon-separated list of br2-external trees to use (optional)" .git/rebase-apply/patch:66: trailing whitespace. .git/rebase-apply/patch:72: trailing whitespace. warning: squelched 6 whitespace errors warning: 10 lines applied after fixing whitespace errors. Applying: merge_config.sh: Fix merging buildroot config files Applying patch #991781 using 'git am' Description: merge_config.sh: Fix merging buildroot config files Kind regards, Petr
On Thu, Nov 01, 2018 at 06:49:42AM +0100, Petr Vorel wrote: > Hi. > > > > This is actually an upstream patch [1] from linux/kbuild project. > > > > [1] > > > https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild > > > .git/commit/?h=for-next&id=f6fe19e9d84400e1c9544c1909952c2c7c6a90a2 > > > > Please refer to commit ID only after it gets stable in Linus' tree. > > > I will end up with rebasing > > In order to drop some other commits causing a problem. > Make sense, thanks, Yamada! > Nasser, maybe use link to patch in patchwork. > I'd put this link also into the patch itself (IMHO good idea for faster > resolving when doing kconfig update in buildroot). > Once the patch gets into mainline, we can update the link. Ok. > > > Kind regards, > Petr > > [1] https://patchwork.kernel.org/patch/10660207/ Thank you for adding the link here.
Hi Petr, On Thu, Nov 01, 2018 at 07:24:49AM +0100, Petr Vorel wrote: > Hi Nasser, > > ... > > @@ -157,6 +162,7 @@ fi > > # Use the merged file as the starting point for: > > # alldefconfig: Fills in any missing symbols with Kconfig default > > # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set > > +unset CONFIG_ > You added unset, which isn't in upstream patch. This has no effect, so I'd be > for removing it. Even if it has (if it was needed), it'd be better to 1) ask > upstream to add it 2) keep it in separate patch in meanwhile (otherwise it > complicates patch rebasing during update and can be lost). Removing unset will cause the following make command work wrong. Note that the value of this environment variable is checked [1] and used. The result of removing the unset line is that we will have double BR2_ prefixes in the final .config file as well as lots of warnings when checking if all specified config values have been taken. I think we should follow your steps to include it. > > ... > > diff --git a/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch b/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch > ... > > + # Use the merged file as the starting point for: > > + # alldefconfig: Fills in any missing symbols with Kconfig default > > + # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set > > ++unset CONFIG_ > > BTW (I noted that before) your patch contain trailing whitespace and mixing tab > and spaces. git am and pwclient fixes that, only when applying with patch they > get committed, so nothing serious. > > $ pwclient git-am -p buildroot 991781 > .git/rebase-apply/patch:59: space before tab in indent. > echo " -r list redundant entries when merging fragments" > .git/rebase-apply/patch:60: space before tab in indent. > echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." > .git/rebase-apply/patch:61: space before tab in indent. > echo " -e colon-separated list of br2-external trees to use (optional)" > .git/rebase-apply/patch:66: trailing whitespace. > > .git/rebase-apply/patch:72: trailing whitespace. > > warning: squelched 6 whitespace errors > warning: 10 lines applied after fixing whitespace errors. > Applying: merge_config.sh: Fix merging buildroot config files > Applying patch #991781 using 'git am' > Description: merge_config.sh: Fix merging buildroot config files > I think this is because we are including a patch in another patch. As you can see in your previous commit: a3366b270562f42723a3b8032f73bb03b541d113, this patch has the same issue because it includes another patch. I don't know how to avoid that if we should do so. > Kind regards, > Petr [1] https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/tree/scripts/kconfig/lkc.h?h=for-next&id=f6fe19e9d84400e1c9544c1909952c2c7c6a90#n26 Kind regards, Nasser
On 01/11/18 11:55, Nasser wrote: > Hi Petr, > On Thu, Nov 01, 2018 at 07:24:49AM +0100, Petr Vorel wrote: >> Hi Nasser, >> >> ... >>> @@ -157,6 +162,7 @@ fi >>> # Use the merged file as the starting point for: >>> # alldefconfig: Fills in any missing symbols with Kconfig default >>> # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set >>> +unset CONFIG_ >> You added unset, which isn't in upstream patch. This has no effect, so I'd be >> for removing it. Even if it has (if it was needed), it'd be better to 1) ask >> upstream to add it 2) keep it in separate patch in meanwhile (otherwise it >> complicates patch rebasing during update and can be lost). > Removing unset will cause the following make command work wrong. Note > that the value of this environment variable is checked [1] and used. The > result of removing the unset line is that we will have double BR2_ > prefixes in the final .config file as well as lots of warnings when > checking if all specified config values have been taken. We have no prefix, the BR2_ is mere convention. So when calling merge_config, we should set CONFIG_ to empty, not to BR2_. So indeed, the unset should be removed. > > I think we should follow your steps to include it. >> >> ... >>> diff --git a/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch b/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch >> ... >>> + # Use the merged file as the starting point for: >>> + # alldefconfig: Fills in any missing symbols with Kconfig default >>> + # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set >>> ++unset CONFIG_ >> >> BTW (I noted that before) your patch contain trailing whitespace and mixing tab >> and spaces. git am and pwclient fixes that, only when applying with patch they >> get committed, so nothing serious. >> >> $ pwclient git-am -p buildroot 991781 >> .git/rebase-apply/patch:59: space before tab in indent. >> echo " -r list redundant entries when merging fragments" >> .git/rebase-apply/patch:60: space before tab in indent. >> echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." >> .git/rebase-apply/patch:61: space before tab in indent. >> echo " -e colon-separated list of br2-external trees to use (optional)" >> .git/rebase-apply/patch:66: trailing whitespace. >> >> .git/rebase-apply/patch:72: trailing whitespace. >> >> warning: squelched 6 whitespace errors >> warning: 10 lines applied after fixing whitespace errors. >> Applying: merge_config.sh: Fix merging buildroot config files >> Applying patch #991781 using 'git am' >> Description: merge_config.sh: Fix merging buildroot config files >> > I think this is because we are including a patch in another patch. As > you can see in your previous commit: > a3366b270562f42723a3b8032f73bb03b541d113, this patch has the same issue > because it includes another patch. > I don't know how to avoid that if we should do so. You're correct, it's expected for *.patch files to have whitespace errors. I'm not entirely sure if we should include the *.patch file in this case, since it actually does come from upstream. However, it definitely doesn't hurt to include it, it can be removed again when the kconfig is updated again. Regards, Arnout >> Kind regards, >> Petr > [1] https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/tree/scripts/kconfig/lkc.h?h=for-next&id=f6fe19e9d84400e1c9544c1909952c2c7c6a90#n26 > Kind regards, > Nasser >
Hi Nasser, > > > +unset CONFIG_ > > You added unset, which isn't in upstream patch. This has no effect, so I'd be > > for removing it. Even if it has (if it was needed), it'd be better to 1) ask > > upstream to add it 2) keep it in separate patch in meanwhile (otherwise it > > complicates patch rebasing during update and can be lost). > Removing unset will cause the following make command work wrong. Note > that the value of this environment variable is checked [1] and used. The > result of removing the unset line is that we will have double BR2_ > prefixes in the final .config file as well as lots of warnings when > checking if all specified config values have been taken. You're right. The reason is that we don't actually use BR2_ as a real prefix (as Masahiro noted [1], but I didn't realize that), but here in utils/test-pkg we pass it as merge_config.sh needs to work with prefixes [2]: - support/kconfig/merge_config.sh -O "${dir}" \ + CONFIG_=BR2_ support/kconfig/merge_config.sh -O "${dir}" \ Delta is smaller, but it's still a patch, which is needed :(. > > BTW (I noted that before) your patch contain trailing whitespace and mixing tab > > and spaces. git am and pwclient fixes that, only when applying with patch they > > get committed, so nothing serious. > > $ pwclient git-am -p buildroot 991781 > > .git/rebase-apply/patch:59: space before tab in indent. > > echo " -r list redundant entries when merging fragments" > > .git/rebase-apply/patch:60: space before tab in indent. > > echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." > > .git/rebase-apply/patch:61: space before tab in indent. > > echo " -e colon-separated list of br2-external trees to use (optional)" > > .git/rebase-apply/patch:66: trailing whitespace. > > .git/rebase-apply/patch:72: trailing whitespace. > > warning: squelched 6 whitespace errors > > warning: 10 lines applied after fixing whitespace errors. > > Applying: merge_config.sh: Fix merging buildroot config files > > Applying patch #991781 using 'git am' > > Description: merge_config.sh: Fix merging buildroot config files > I think this is because we are including a patch in another patch. As > you can see in your previous commit: > a3366b270562f42723a3b8032f73bb03b541d113, this patch has the same issue > because it includes another patch. > I don't know how to avoid that if we should do so. Again, you're right. I'm sorry for bothering you with it. Kind regards, Petr [1] https://patchwork.kernel.org/patch/10658589/#22290097 [2] https://patchwork.ozlabs.org/patch/991781/
Hi Arnout, > > Removing unset will cause the following make command work wrong. Note > > that the value of this environment variable is checked [1] and used. The > > result of removing the unset line is that we will have double BR2_ > > prefixes in the final .config file as well as lots of warnings when > > checking if all specified config values have been taken. > We have no prefix, the BR2_ is mere convention. So when calling merge_config, > we should set CONFIG_ to empty, not to BR2_. > So indeed, the unset should be removed. +1. I overlooked that regexp works with empty $CONFIG_ as well. Thanks for explanation. Kind regards, Petr
Hi Petr, Arnout, On Thu, Nov 01, 2018 at 05:19:19PM +0100, Petr Vorel wrote: > Hi Arnout, > > > > Removing unset will cause the following make command work wrong. Note > > > that the value of this environment variable is checked [1] and used. The > > > result of removing the unset line is that we will have double BR2_ > > > prefixes in the final .config file as well as lots of warnings when > > > checking if all specified config values have been taken. > > > We have no prefix, the BR2_ is mere convention. So when calling merge_config, > > we should set CONFIG_ to empty, not to BR2_. > > > So indeed, the unset should be removed. > > +1. I overlooked that regexp works with empty $CONFIG_ as well. > Thanks for explanation. As we discussed earlier [1], setting CONFIG_ to empty, makes most comments to be identified as config fragments and generate false warnings. > > Kind regards, > Petr Kind regards, Nasser [1] https://patchwork.ozlabs.org/comment/2018492/
Hi Petr, On Thu, Nov 01, 2018 at 02:23:51PM +0100, Petr Vorel wrote: > Hi Nasser, > > > > > +unset CONFIG_ > > > You added unset, which isn't in upstream patch. This has no effect, so I'd be > > > for removing it. Even if it has (if it was needed), it'd be better to 1) ask > > > upstream to add it 2) keep it in separate patch in meanwhile (otherwise it > > > complicates patch rebasing during update and can be lost). > > Removing unset will cause the following make command work wrong. Note > > that the value of this environment variable is checked [1] and used. The > > result of removing the unset line is that we will have double BR2_ > > prefixes in the final .config file as well as lots of warnings when > > checking if all specified config values have been taken. > You're right. The reason is that we don't actually use BR2_ as a real prefix > (as Masahiro noted [1], but I didn't realize that), but here in utils/test-pkg > we pass it as merge_config.sh needs to work with prefixes [2]: > > - support/kconfig/merge_config.sh -O "${dir}" \ > + CONFIG_=BR2_ support/kconfig/merge_config.sh -O "${dir}" \ > > Delta is smaller, but it's still a patch, which is needed :(. OK. I'll prepare a separate patch for this chunk. > > > > BTW (I noted that before) your patch contain trailing whitespace and mixing tab > > > and spaces. git am and pwclient fixes that, only when applying with patch they > > > get committed, so nothing serious. > > > > $ pwclient git-am -p buildroot 991781 > > > .git/rebase-apply/patch:59: space before tab in indent. > > > echo " -r list redundant entries when merging fragments" > > > .git/rebase-apply/patch:60: space before tab in indent. > > > echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." > > > .git/rebase-apply/patch:61: space before tab in indent. > > > echo " -e colon-separated list of br2-external trees to use (optional)" > > > .git/rebase-apply/patch:66: trailing whitespace. > > > > .git/rebase-apply/patch:72: trailing whitespace. > > > > warning: squelched 6 whitespace errors > > > warning: 10 lines applied after fixing whitespace errors. > > > Applying: merge_config.sh: Fix merging buildroot config files > > > Applying patch #991781 using 'git am' > > > Description: merge_config.sh: Fix merging buildroot config files > > > I think this is because we are including a patch in another patch. As > > you can see in your previous commit: > > a3366b270562f42723a3b8032f73bb03b541d113, this patch has the same issue > > because it includes another patch. > > I don't know how to avoid that if we should do so. > Again, you're right. I'm sorry for bothering you with it. No problem :-). > > > Kind regards, > Petr > > [1] https://patchwork.kernel.org/patch/10658589/#22290097 > [2] https://patchwork.ozlabs.org/patch/991781/ Kind regards, Nasser
Hi. > -----Original Message----- > From: Nasser [mailto:afshin.nasser@gmail.com] > Sent: Friday, November 02, 2018 11:13 AM > To: Petr Vorel <petr.vorel@gmail.com> > Cc: Arnout Vandecappelle <arnout@mind.be>; buildroot@buildroot.org; > Matthew Weber <matthew.weber@rockwellcollins.com>; Angelo Compagnucci > <angelo.compagnucci@gmail.com>; Yamada, Masahiro/山田 真弘 > <yamada.masahiro@socionext.com> > Subject: Re: [PATCH] merge_config.sh: Fix merging buildroot config files > > Hi Petr, Arnout, > On Thu, Nov 01, 2018 at 05:19:19PM +0100, Petr Vorel wrote: > > Hi Arnout, > > > > > > Removing unset will cause the following make command work wrong. Note > > > > that the value of this environment variable is checked [1] and used. > The > > > > result of removing the unset line is that we will have double BR2_ > > > > prefixes in the final .config file as well as lots of warnings when > > > > checking if all specified config values have been taken. > > > > > We have no prefix, the BR2_ is mere convention. So when calling > merge_config, > > > we should set CONFIG_ to empty, not to BR2_. > > > > > So indeed, the unset should be removed. > > > > +1. I overlooked that regexp works with empty $CONFIG_ as well. > > Thanks for explanation. > As we discussed earlier [1], setting CONFIG_ to empty, makes most comments > to > be identified as config fragments and generate false warnings. I was also being worried about this. We could improve the sed pattern. How about this? https://patchwork.kernel.org/patch/10665119/ > > > > Kind regards, > > Petr > Kind regards, > Nasser > > [1] https://patchwork.ozlabs.org/comment/2018492/
On 02/11/18 08:55, yamada.masahiro@socionext.com wrote: > Hi. > >> -----Original Message----- >> From: Nasser [mailto:afshin.nasser@gmail.com] >> Sent: Friday, November 02, 2018 11:13 AM >> To: Petr Vorel <petr.vorel@gmail.com> >> Cc: Arnout Vandecappelle <arnout@mind.be>; buildroot@buildroot.org; >> Matthew Weber <matthew.weber@rockwellcollins.com>; Angelo Compagnucci >> <angelo.compagnucci@gmail.com>; Yamada, Masahiro/山田 真弘 >> <yamada.masahiro@socionext.com> >> Subject: Re: [PATCH] merge_config.sh: Fix merging buildroot config files >> >> Hi Petr, Arnout, >> On Thu, Nov 01, 2018 at 05:19:19PM +0100, Petr Vorel wrote: >>> Hi Arnout, >>> >>>>> Removing unset will cause the following make command work wrong. Note >>>>> that the value of this environment variable is checked [1] and used. >> The >>>>> result of removing the unset line is that we will have double BR2_ >>>>> prefixes in the final .config file as well as lots of warnings when >>>>> checking if all specified config values have been taken. >>> >>>> We have no prefix, the BR2_ is mere convention. So when calling >> merge_config, >>>> we should set CONFIG_ to empty, not to BR2_. >>> >>>> So indeed, the unset should be removed. >>> >>> +1. I overlooked that regexp works with empty $CONFIG_ as well. >>> Thanks for explanation. >> As we discussed earlier [1], setting CONFIG_ to empty, makes most comments >> to >> be identified as config fragments and generate false warnings. > > I was also being worried about this. > > We could improve the sed pattern. > > How about this? > https://patchwork.kernel.org/patch/10665119/ Perhaps it's better to make the entire pattern explicit, like: SED_CONFIG_EXP="s/^\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)=.*$\|^# \(${CONFIG_PREFIX}[a-zA-Z0-9_]*\) is not set$/\1\2/p" (\1\2 works because only one of the two subexpressions matches, so either \1 or \2 must be empty). Regards, Arnout
Hi Arnout, > > How about this? > > https://patchwork.kernel.org/patch/10665119/ > Perhaps it's better to make the entire pattern explicit, like: > SED_CONFIG_EXP="s/^\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)=.*$\|^# > \(${CONFIG_PREFIX}[a-zA-Z0-9_]*\) is not set$/\1\2/p" > (\1\2 works because only one of the two subexpressions matches, so either \1 or > \2 must be empty). I'd prefer original Masahiro's version - it's easier to read and without duplicity. > Regards, > Arnout Kind regards, Petr
Hi Petr, Mahasiro, On Fri, Nov 02, 2018 at 11:50:58PM +0100, Petr Vorel wrote: > Hi Arnout, > > > > How about this? > > > https://patchwork.kernel.org/patch/10665119/ > > > Perhaps it's better to make the entire pattern explicit, like: > > > SED_CONFIG_EXP="s/^\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)=.*$\|^# > > \(${CONFIG_PREFIX}[a-zA-Z0-9_]*\) is not set$/\1\2/p" > > > (\1\2 works because only one of the two subexpressions matches, so either \1 or > > \2 must be empty). > I'd prefer original Masahiro's version - it's easier to read and without > duplicity. > I agree with you. +1 for not having duplicity. This patch makes merge_config.sh to work well in many more tests. I'm waiting for Masahiro's version to be applied to the his kbuild tree and then re-prepare the patch here. > > Regards, > > Arnout > > > Kind regards, > Petr > Kind regards, Nasser
Hi. > -----Original Message----- > From: Nasser [mailto:afshin.nasser@gmail.com] > Sent: Sunday, November 04, 2018 6:32 AM > To: Petr Vorel <petr.vorel@gmail.com> > Cc: Arnout Vandecappelle <arnout@mind.be>; Yamada, Masahiro/山田 真弘 > <yamada.masahiro@socionext.com>; buildroot@buildroot.org; > matthew.weber@rockwellcollins.com; angelo.compagnucci@gmail.com > Subject: Re: [PATCH] merge_config.sh: Fix merging buildroot config files > > Hi Petr, Mahasiro, > On Fri, Nov 02, 2018 at 11:50:58PM +0100, Petr Vorel wrote: > > Hi Arnout, > > > > > > How about this? > > > > https://patchwork.kernel.org/patch/10665119/ > > > > > Perhaps it's better to make the entire pattern explicit, like: > > > > > SED_CONFIG_EXP="s/^\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)=.*$\|^# > > > \(${CONFIG_PREFIX}[a-zA-Z0-9_]*\) is not set$/\1\2/p" > > > > > (\1\2 works because only one of the two subexpressions matches, so either > \1 or > > > \2 must be empty). > > I'd prefer original Masahiro's version - it's easier to read and without > > duplicity. > > > I agree with you. +1 for not having duplicity. > This patch makes merge_config.sh to work well in many more tests. I'm waiting > for Masahiro's version to be applied to > the his kbuild tree and then re-prepare the patch here. OK, I think Arnout's one is unreadable, but we can make it readable by splitting the complex pattern into two. I posted this for comparison. https://patchwork.kernel.org/patch/10667571/ Thanks.
Hi, > > > > Perhaps it's better to make the entire pattern explicit, like: > > > > SED_CONFIG_EXP="s/^\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)=.*$\|^# > > > > \(${CONFIG_PREFIX}[a-zA-Z0-9_]*\) is not set$/\1\2/p" > > > > (\1\2 works because only one of the two subexpressions matches, so either > > \1 or > > > > \2 must be empty). > > > I'd prefer original Masahiro's version - it's easier to read and without > > > duplicity. > > I agree with you. +1 for not having duplicity. > > This patch makes merge_config.sh to work well in many more tests. I'm waiting > > for Masahiro's version to be applied to > > the his kbuild tree and then re-prepare the patch here. > OK, I think Arnout's one is unreadable, but > we can make it readable by splitting the complex pattern into two. > I posted this for comparison. > https://patchwork.kernel.org/patch/10667571/ LGTM. It's quite verbose, but more precise regexp is better and readability is also good. Kind regards, Petr
diff --git a/support/kconfig/merge_config.sh b/support/kconfig/merge_config.sh index 50de5114dc..15441182cf 100755 --- a/support/kconfig/merge_config.sh +++ b/support/kconfig/merge_config.sh @@ -34,12 +34,16 @@ usage() { echo " -r list redundant entries when merging fragments" echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." echo " -e colon-separated list of br2-external trees to use (optional)" + echo + echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ + environment variable." } RUNMAKE=true ALLTARGET=alldefconfig WARNREDUN=false OUTPUT=. +CONFIG_PREFIX=${CONFIG_-CONFIG_} while true; do case $1 in @@ -105,7 +109,8 @@ 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\}\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)[= ].*/\2/p" + TMP_FILE=$(mktemp -t .tmp.config.XXXXXXXXXX) echo "Using $INITFILE as base" @@ -157,6 +162,7 @@ fi # Use the merged file as the starting point for: # alldefconfig: Fills in any missing symbols with Kconfig default # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set +unset CONFIG_ make KCONFIG_ALLCONFIG=$TMP_FILE $EXTERNAL_ARG $OUTPUT_ARG $ALLTARGET diff --git a/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch b/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch new file mode 100644 index 0000000000..7b05736b05 --- /dev/null +++ b/support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch @@ -0,0 +1,39 @@ +Index: kconfig/merge_config.sh +=================================================================== +--- kconfig.orig/merge_config.sh ++++ kconfig/merge_config.sh +@@ -34,12 +34,16 @@ usage() { + echo " -r list redundant entries when merging fragments" + echo " -O dir to put generated output files. Consider setting \$KCONFIG_CONFIG instead." + echo " -e colon-separated list of br2-external trees to use (optional)" ++ echo ++ echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ ++ environment variable." + } + + RUNMAKE=true + ALLTARGET=alldefconfig + WARNREDUN=false + OUTPUT=. ++CONFIG_PREFIX=${CONFIG_-CONFIG_} + + while true; do + case $1 in +@@ -105,7 +109,8 @@ 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\}\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)[= ].*/\2/p" ++ + TMP_FILE=$(mktemp -t .tmp.config.XXXXXXXXXX) + + echo "Using $INITFILE as base" +@@ -157,6 +162,7 @@ fi + # Use the merged file as the starting point for: + # alldefconfig: Fills in any missing symbols with Kconfig default + # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set ++unset CONFIG_ + make KCONFIG_ALLCONFIG=$TMP_FILE $EXTERNAL_ARG $OUTPUT_ARG $ALLTARGET + + diff --git a/support/kconfig/patches/series b/support/kconfig/patches/series index e136de7937..be8627db13 100644 --- a/support/kconfig/patches/series +++ b/support/kconfig/patches/series @@ -8,3 +8,4 @@ 17-backport-kecho.patch 18-merge-config.sh-create-temporary-files-in-tmp.patch 19-merge_config.sh-add-br2-external-support.patch +20-merge_config.sh-Allow-to-define-config-prefix.patch diff --git a/utils/test-pkg b/utils/test-pkg index aa91ee02cf..f2d519b4f6 100755 --- a/utils/test-pkg +++ b/utils/test-pkg @@ -129,7 +129,7 @@ build_one() { mkdir -p "${dir}" - support/kconfig/merge_config.sh -O "${dir}" \ + CONFIG_=BR2_ support/kconfig/merge_config.sh -O "${dir}" \ "${toolchainconfig}" "support/config-fragments/minimal.config" "${cfg}" \ >> "${dir}/logfile" 2>&1 # We want all the options from the snippet to be present as-is (set
This patch allows us to define config prefix with CONFIG_ environment variable. Now we can define BR2_ prefix when we are going to merge buildroot fragment configs. By setting the proper config prefix, we will have proper 'redundant configuration warnings' when we use '-r -m' options. This is actually an upstream patch [1] from linux/kbuild project. [1] https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/commit/?h=for-next&id=f6fe19e9d84400e1c9544c1909952c2c7c6a90a2 Signed-off-by: Nasser Afshin <afshin.nasser@gmail.com> --- support/kconfig/merge_config.sh | 8 ++++- ...e_config.sh-Allow-to-define-config-prefix.patch | 39 ++++++++++++++++++++++ support/kconfig/patches/series | 1 + utils/test-pkg | 2 +- 4 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 support/kconfig/patches/20-merge_config.sh-Allow-to-define-config-prefix.patch