| Message ID | 20181028125510.9346-1-petr.vorel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] merge_config.sh: Replace prefix CONFIG_ with any prefix | expand |
On Sun, Oct 28, 2018 at 9:55 PM Petr Vorel <petr.vorel@gmail.com> wrote: > > merge_config.sh expect prefix from kernel: CONFIG_. > There some other projects using kconfig with different prefixes > (e.g. buildroot: BR2_ prefix). While there could be some option > to specify config prefix [1] [2], IMHO it's better to not specify > prefix (expect just any prefix). > > [1] http://patchwork.ozlabs.org/patch/824051/ > [2] https://patchwork.ozlabs.org/patch/988890/ > > Signed-off-by: Petr Vorel <petr.vorel@gmail.com> > Suggested-by: Marcel Patzlaff <m.patzlaff@pilz.de> > --- > NOTE: I deliberately left prefix CONFIG_ in comment: > # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set I'd like to filter "CONFIG_", "BR2_", or whatever specified prefix. Some fragment files (e.g. arch/x86/configs/xen.config) have comment lines. Matching '# ' + arbitrary string might hit comment lines accidentally. The kconfig binary checks the environment variable 'CONFIG_' to override the default prefix: https://github.com/torvalds/linux/blob/v4.19/scripts/kconfig/lkc.h#L28 For similarity, how about doing in the same way in merge_config.sh ? Does the following work for you ? (you can pass CONFIG_=BR2_ to merge_config.sh) diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh index 67d1314..391ee6c 100755 --- a/scripts/kconfig/merge_config.sh +++ b/scripts/kconfig/merge_config.sh @@ -90,6 +90,10 @@ if [ -z "$KCONFIG_CONFIG" ]; then fi fi +# Check the environment variable "CONFIG_" for the config option prefix. +# If unset, the default is "CONFIG_". +: ${CONFIG_=CONFIG_} + INITFILE=$1 shift; @@ -99,7 +103,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\}\(${CONFIG_}[a-zA-Z0-9_]*\)[= ].*/\2/p" TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX) echo "Using $INITFILE as base" -- Best Regards Masahiro Yamada
Hi Masahiro, thanks for a quick review and patch. > I'd like to filter "CONFIG_", "BR2_", or whatever specified prefix. > Some fragment files (e.g. arch/x86/configs/xen.config) > have comment lines. > Matching '# ' + arbitrary string > might hit comment lines accidentally. Make sense. > The kconfig binary checks the environment variable 'CONFIG_' > to override the default prefix: > https://github.com/torvalds/linux/blob/v4.19/scripts/kconfig/lkc.h#L28 What a crazy variable name. But we're not going to change it. > For similarity, how about doing in the same way in merge_config.sh ? Make sense to keep the same approach. > Does the following work for you ? > (you can pass CONFIG_=BR2_ to merge_config.sh) Patch is good (with one note bellow). Reviewed-by: Petr Vorel <petr.vorel@gmail.com> Could you push your patch into your tree? Or going to post a patch? If not here is mine: > diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh > index 67d1314..391ee6c 100755 > --- a/scripts/kconfig/merge_config.sh > +++ b/scripts/kconfig/merge_config.sh > @@ -90,6 +90,10 @@ if [ -z "$KCONFIG_CONFIG" ]; then > fi > fi > +# Check the environment variable "CONFIG_" for the config option prefix. > +# If unset, the default is "CONFIG_". > +: ${CONFIG_=CONFIG_} Although I guess this could be less cryptic: CONFIG_="${CONFIG_:-CONFIG_}" but too verbose, so I'm for your variant. ... > MERGE_LIST=$* > -SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(CONFIG_[a-zA-Z0-9_]*\)[= ].*/\2/p" > +SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(${CONFIG_}[a-zA-Z0-9_]*\)[= ].*/\2/p" > TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX) Kind regards, Petr
Hi Petr, On Tue, Oct 30, 2018 at 12:40 AM Petr Vorel <petr.vorel@gmail.com> wrote: > > Hi Masahiro, > > thanks for a quick review and patch. > > > I'd like to filter "CONFIG_", "BR2_", or whatever specified prefix. > > > Some fragment files (e.g. arch/x86/configs/xen.config) > > have comment lines. > > > Matching '# ' + arbitrary string > > might hit comment lines accidentally. > Make sense. > > > The kconfig binary checks the environment variable 'CONFIG_' > > to override the default prefix: > > https://github.com/torvalds/linux/blob/v4.19/scripts/kconfig/lkc.h#L28 > What a crazy variable name. But we're not going to change it. > > > For similarity, how about doing in the same way in merge_config.sh ? > Make sense to keep the same approach. > > > Does the following work for you ? > > (you can pass CONFIG_=BR2_ to merge_config.sh) > Patch is good (with one note bellow). > > Reviewed-by: Petr Vorel <petr.vorel@gmail.com> > > Could you push your patch into your tree? > Or going to post a patch? If not here is mine: Please send v2 with the commit log properly filled. I will take my credit as Suggested-by. > > diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh > > index 67d1314..391ee6c 100755 > > --- a/scripts/kconfig/merge_config.sh > > +++ b/scripts/kconfig/merge_config.sh > > @@ -90,6 +90,10 @@ if [ -z "$KCONFIG_CONFIG" ]; then > > fi > > fi > > > +# Check the environment variable "CONFIG_" for the config option prefix. > > +# If unset, the default is "CONFIG_". > > +: ${CONFIG_=CONFIG_} > Although I guess this could be less cryptic: > CONFIG_="${CONFIG_:-CONFIG_}" You should not use the colon because we want to allow the environment variable 'CONFIG_' being set as empty. In fact, Buildroot uses empty prefix instead of "BR2_". In my understanding, Buildroot just has a convention where symbols in Config.in files start with "BR2_". So, in order to make your idea even less cryptic, how about this? CONFIG_PREFIX=${CONFIG_-CONFIG_} Then, SED_CONFIG_EXP="s/^\(# \)\{0,1\}\(${CONFIG_PREFIX}[a-zA-Z0-9_]*\)[= ].*/\2/p"
Hi Masahiro, > Please send v2 with the commit log properly filled. OK I'll do. > I will take my credit as Suggested-by. Sure :). > > > +# Check the environment variable "CONFIG_" for the config option prefix. > > > +# If unset, the default is "CONFIG_". > > > +: ${CONFIG_=CONFIG_} > > Although I guess this could be less cryptic: > > CONFIG_="${CONFIG_:-CONFIG_}" > You should not use the colon > because we want to allow the environment variable > 'CONFIG_' being set as empty. How about having default 'CONFIG_' and if it's empty change it to '[A-Z0-9_]\+' ? I know, it's different from C definition in lkc.h, but IMHO it'd make sense. But I guess you prefer your suggestion CONFIG_PREFIX=${CONFIG_-CONFIG_} > In fact, Buildroot uses empty prefix instead of "BR2_". > In my understanding, Buildroot just has a convention where > symbols in Config.in files start with "BR2_". IMHO buildroot uses 'BR2_' prefix. That's what we want to use with merge_config.sh. Kind regards, Petr
On Tue, Oct 30, 2018 at 3:27 AM Petr Vorel <petr.vorel@gmail.com> wrote: > > Hi Masahiro, > > > Please send v2 with the commit log properly filled. > OK I'll do. > > > I will take my credit as Suggested-by. > Sure :). > > > > > +# Check the environment variable "CONFIG_" for the config option prefix. > > > > +# If unset, the default is "CONFIG_". > > > > +: ${CONFIG_=CONFIG_} > > > Although I guess this could be less cryptic: > > > CONFIG_="${CONFIG_:-CONFIG_}" > > > You should not use the colon > > because we want to allow the environment variable > > 'CONFIG_' being set as empty. > How about having default 'CONFIG_' and if it's empty change it to > '[A-Z0-9_]\+' ? No, I'd like to have the same rule for prefix. > I know, it's different from C definition in lkc.h, but IMHO it'd > make sense. But I guess you prefer your suggestion CONFIG_PREFIX=${CONFIG_-CONFIG_} Yeah, I prefer this. > > In fact, Buildroot uses empty prefix instead of "BR2_". > > In my understanding, Buildroot just has a convention where > > symbols in Config.in files start with "BR2_". > IMHO buildroot uses 'BR2_' prefix. That's what we want to use with > merge_config.sh. Yes, you can use 'BR2_' prefix for merge_config. I just wanted to point out that Buildroot's own patch specifies the prefix as empty wrt the kconfig binary. https://github.com/buildroot/buildroot/blob/2018.08.2/support/kconfig/patches/10-br-build-system.patch#L32 > > Kind regards, > Petr
diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh index 67d131447631..538222848bcf 100755 --- a/scripts/kconfig/merge_config.sh +++ b/scripts/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\}\([A-Z0-9_]\+_[a-zA-Z0-9_]*\)[= ].*/\2/p" TMP_FILE=$(mktemp ./.tmp.config.XXXXXXXXXX) echo "Using $INITFILE as base"
merge_config.sh expect prefix from kernel: CONFIG_. There some other projects using kconfig with different prefixes (e.g. buildroot: BR2_ prefix). While there could be some option to specify config prefix [1] [2], IMHO it's better to not specify prefix (expect just any prefix). [1] http://patchwork.ozlabs.org/patch/824051/ [2] https://patchwork.ozlabs.org/patch/988890/ Signed-off-by: Petr Vorel <petr.vorel@gmail.com> Suggested-by: Marcel Patzlaff <m.patzlaff@pilz.de> --- NOTE: I deliberately left prefix CONFIG_ in comment: # allnoconfig: Fills in any missing symbols with # CONFIG_* is not set Kind regards, Petr --- scripts/kconfig/merge_config.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)