scripts/kconfig/merge_config: don't redefine 'y' to 'm'
diff mbox series

Message ID 20181102114119.23859-1-anders.roxell@linaro.org
State New
Headers show
Series
  • scripts/kconfig/merge_config: don't redefine 'y' to 'm'
Related show

Commit Message

Anders Roxell Nov. 2, 2018, 11:41 a.m. UTC
In today's merge_config.sh the order of the config fragment files dictates
the output of a config option. With this approach we will get different
.config files depending on the order of the config fragment files.
Adding a switch to add precedence for builtin over modules, this will
make the .config file the same

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 scripts/kconfig/merge_config.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Masahiro Yamada Nov. 5, 2018, 8:34 a.m. UTC | #1
Hi Anders,

On Fri, Nov 2, 2018 at 8:41 PM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> In today's merge_config.sh the order of the config fragment files dictates
> the output of a config option. With this approach we will get different
> .config files depending on the order of the config fragment files.
> Adding a switch to add precedence for builtin over modules, this will
> make the .config file the same
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---

I think this patch makes sense.

Just in case, could you please provide me the context of the discussion?

Does the real problem exist in the kernel tree,
or for local fragment files?


Thanks.



>  scripts/kconfig/merge_config.sh | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index da66e7742282..902a60b45614 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -32,6 +32,7 @@ usage() {
>         echo "  -m    only merge the fragments, do not execute the make command"
>         echo "  -n    use allnoconfig instead of alldefconfig"
>         echo "  -r    list redundant entries when merging fragments"
> +       echo "  -y    make builtin have precedence over modules"
>         echo "  -O    dir to put generated output files.  Consider setting \$KCONFIG_CONFIG instead."
>         echo
>         echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ environment variable."
> @@ -40,6 +41,7 @@ usage() {
>  RUNMAKE=true
>  ALLTARGET=alldefconfig
>  WARNREDUN=false
> +BUILTIN=false
>  OUTPUT=.
>  CONFIG_PREFIX=${CONFIG_-CONFIG_}
>
> @@ -64,6 +66,11 @@ while true; do
>                 shift
>                 continue
>                 ;;
> +       "-y")
> +               BUILTIN=true
> +               shift
> +               continue
> +               ;;
>         "-O")
>                 if [ -d $2 ];then
>                         OUTPUT=$(echo $2 | sed 's/\/*$//')
> @@ -122,7 +129,13 @@ for MERGE_FILE in $MERGE_LIST ; do
>                 grep -q -w $CFG $TMP_FILE || continue
>                 PREV_VAL=$(grep -w $CFG $TMP_FILE)
>                 NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> -               if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> +               if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then
> +                       echo Value of $CFG is \'y\' and we don\'t want to redefine the fragment $MERGE_FILE:
> +                       echo Previous  value: $PREV_VAL
> +                       echo New value:       $NEW_VAL
> +                       echo Will use previous value.
> +                       echo
> +               elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
>                         echo Value of $CFG is redefined by fragment $MERGE_FILE:
>                         echo Previous  value: $PREV_VAL
>                         echo New value:       $NEW_VAL
> --
> 2.11.0
>
Anders Roxell Nov. 6, 2018, 1:57 p.m. UTC | #2
On Mon, 5 Nov 2018 at 09:35, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Hi Anders,
>
> On Fri, Nov 2, 2018 at 8:41 PM Anders Roxell <anders.roxell@linaro.org> wrote:
> >
> > In today's merge_config.sh the order of the config fragment files dictates
> > the output of a config option. With this approach we will get different
> > .config files depending on the order of the config fragment files.
> > Adding a switch to add precedence for builtin over modules, this will
> > make the .config file the same
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > ---
>
> I think this patch makes sense.
>
> Just in case, could you please provide me the context of the discussion?

For instance we don't want to force X86 from DRM=y to DRM=m, when
enabling selftest, that would surely break somebody's setup and you also
don't want to force ARM64 from DRM=m to DRM=y, that seems
unnecessary for a big subsystem like DRM.

>
> Does the real problem exist in the kernel tree,

Not that I'm aware about.

Cheers,
Anders

> or for local fragment files?
>
>
> Thanks.
>
>
>
> >  scripts/kconfig/merge_config.sh | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> > index da66e7742282..902a60b45614 100755
> > --- a/scripts/kconfig/merge_config.sh
> > +++ b/scripts/kconfig/merge_config.sh
> > @@ -32,6 +32,7 @@ usage() {
> >         echo "  -m    only merge the fragments, do not execute the make command"
> >         echo "  -n    use allnoconfig instead of alldefconfig"
> >         echo "  -r    list redundant entries when merging fragments"
> > +       echo "  -y    make builtin have precedence over modules"
> >         echo "  -O    dir to put generated output files.  Consider setting \$KCONFIG_CONFIG instead."
> >         echo
> >         echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ environment variable."
> > @@ -40,6 +41,7 @@ usage() {
> >  RUNMAKE=true
> >  ALLTARGET=alldefconfig
> >  WARNREDUN=false
> > +BUILTIN=false
> >  OUTPUT=.
> >  CONFIG_PREFIX=${CONFIG_-CONFIG_}
> >
> > @@ -64,6 +66,11 @@ while true; do
> >                 shift
> >                 continue
> >                 ;;
> > +       "-y")
> > +               BUILTIN=true
> > +               shift
> > +               continue
> > +               ;;
> >         "-O")
> >                 if [ -d $2 ];then
> >                         OUTPUT=$(echo $2 | sed 's/\/*$//')
> > @@ -122,7 +129,13 @@ for MERGE_FILE in $MERGE_LIST ; do
> >                 grep -q -w $CFG $TMP_FILE || continue
> >                 PREV_VAL=$(grep -w $CFG $TMP_FILE)
> >                 NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> > -               if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> > +               if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then
> > +                       echo Value of $CFG is \'y\' and we don\'t want to redefine the fragment $MERGE_FILE:
> > +                       echo Previous  value: $PREV_VAL
> > +                       echo New value:       $NEW_VAL
> > +                       echo Will use previous value.
> > +                       echo
> > +               elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> >                         echo Value of $CFG is redefined by fragment $MERGE_FILE:
> >                         echo Previous  value: $PREV_VAL
> >                         echo New value:       $NEW_VAL
> > --
> > 2.11.0
> >
>
>
> --
> Best Regards
> Masahiro Yamada
Darren Hart Nov. 7, 2018, 6:47 p.m. UTC | #3
On Tue, Nov 06, 2018 at 02:57:40PM +0100, Anders Roxell wrote:
> On Mon, 5 Nov 2018 at 09:35, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > Hi Anders,
> >
> > On Fri, Nov 2, 2018 at 8:41 PM Anders Roxell <anders.roxell@linaro.org> wrote:
> > >
> > > In today's merge_config.sh the order of the config fragment files dictates
> > > the output of a config option. With this approach we will get different
> > > .config files depending on the order of the config fragment files.
> > > Adding a switch to add precedence for builtin over modules, this will
> > > make the .config file the same
> > >
> > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > > ---
> >
> > I think this patch makes sense.
> >
> > Just in case, could you please provide me the context of the discussion?
> 
> For instance we don't want to force X86 from DRM=y to DRM=m, when
> enabling selftest, that would surely break somebody's setup and you also
> don't want to force ARM64 from DRM=m to DRM=y, that seems
> unnecessary for a big subsystem like DRM.
> 
> >
> > Does the real problem exist in the kernel tree,
> 
> Not that I'm aware about.
> 
> Cheers,
> Anders
> 
> > or for local fragment files?

This is really about ordering of fragment files and not "making a symbol
'less'", I think were Arnd's words.

So, doing something like:

$ make arm64-selftest.config drm.config

where arm64-selftest.config defines DRM=y and drm.config defines DRM=m, the
result should be "DRM=y".

So the first step is to make merge_config.sh support it. A follow up step would
be to integrate this into the kernel Makefile system if we determine this is the
correct behavior, or to provide a parameter if we don't want to change the
default behavior.
Darren Hart Nov. 7, 2018, 7:36 p.m. UTC | #4
On Fri, Nov 02, 2018 at 12:41:19PM +0100, Anders Roxell wrote:
> In today's merge_config.sh the order of the config fragment files dictates
> the output of a config option. With this approach we will get different
> .config files depending on the order of the config fragment files.
> Adding a switch to add precedence for builtin over modules, this will
> make the .config file the same
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

Thanks for the patch Anders!

> ---
>  scripts/kconfig/merge_config.sh | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> index da66e7742282..902a60b45614 100755
> --- a/scripts/kconfig/merge_config.sh
> +++ b/scripts/kconfig/merge_config.sh
> @@ -32,6 +32,7 @@ usage() {
>  	echo "  -m    only merge the fragments, do not execute the make command"
>  	echo "  -n    use allnoconfig instead of alldefconfig"
>  	echo "  -r    list redundant entries when merging fragments"
> +	echo "  -y    make builtin have precedence over modules"
>  	echo "  -O    dir to put generated output files.  Consider setting \$KCONFIG_CONFIG instead."
>  	echo
>  	echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ environment variable."
> @@ -40,6 +41,7 @@ usage() {
>  RUNMAKE=true
>  ALLTARGET=alldefconfig
>  WARNREDUN=false
> +BUILTIN=false
>  OUTPUT=.
>  CONFIG_PREFIX=${CONFIG_-CONFIG_}
>  
> @@ -64,6 +66,11 @@ while true; do
>  		shift
>  		continue
>  		;;
> +	"-y")
> +		BUILTIN=true
> +		shift
> +		continue
> +		;;
>  	"-O")
>  		if [ -d $2 ];then
>  			OUTPUT=$(echo $2 | sed 's/\/*$//')
> @@ -122,7 +129,13 @@ for MERGE_FILE in $MERGE_LIST ; do
>  		grep -q -w $CFG $TMP_FILE || continue
>  		PREV_VAL=$(grep -w $CFG $TMP_FILE)
>  		NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> -		if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> +		if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then

I think we can avoid the rather lengthy/forky "echo | grep" mechanism
with POSIX substring parameter expansion (should work in bash, dash,
etc.).

http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02

We can also be more consistent with the other tests in the script, consider:

		if [ "$BUILTIN" = "true" ] && [ "${NEW_VAL#CONFIG_*=}" = "m" ] && [ "${PREV_VAL#CONFIG_*=}" = "y" ]; then


> +			echo Value of $CFG is \'y\' and we don\'t want to redefine the fragment $MERGE_FILE:

I think we can drop the above and perhaps update the last line with...

> +			echo Previous  value: $PREV_VAL
> +			echo New value:       $NEW_VAL
> +			echo Will use previous value.

echo "-y passed, will not demote y to m"

Or something along those lines.

> +			echo
> +		elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
>  			echo Value of $CFG is redefined by fragment $MERGE_FILE:
>  			echo Previous  value: $PREV_VAL
>  			echo New value:       $NEW_VAL
> -- 
> 2.11.0
> 
>
Anders Roxell Nov. 8, 2018, 7:43 p.m. UTC | #5
On Wed, 7 Nov 2018 at 20:36, Darren Hart <dvhart@infradead.org> wrote:
>
> On Fri, Nov 02, 2018 at 12:41:19PM +0100, Anders Roxell wrote:
> > In today's merge_config.sh the order of the config fragment files dictates
> > the output of a config option. With this approach we will get different
> > .config files depending on the order of the config fragment files.
> > Adding a switch to add precedence for builtin over modules, this will
> > make the .config file the same
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>
> Thanks for the patch Anders!
>
> > ---
> >  scripts/kconfig/merge_config.sh | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
> > index da66e7742282..902a60b45614 100755
> > --- a/scripts/kconfig/merge_config.sh
> > +++ b/scripts/kconfig/merge_config.sh
> > @@ -32,6 +32,7 @@ usage() {
> >       echo "  -m    only merge the fragments, do not execute the make command"
> >       echo "  -n    use allnoconfig instead of alldefconfig"
> >       echo "  -r    list redundant entries when merging fragments"
> > +     echo "  -y    make builtin have precedence over modules"
> >       echo "  -O    dir to put generated output files.  Consider setting \$KCONFIG_CONFIG instead."
> >       echo
> >       echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ environment variable."
> > @@ -40,6 +41,7 @@ usage() {
> >  RUNMAKE=true
> >  ALLTARGET=alldefconfig
> >  WARNREDUN=false
> > +BUILTIN=false
> >  OUTPUT=.
> >  CONFIG_PREFIX=${CONFIG_-CONFIG_}
> >
> > @@ -64,6 +66,11 @@ while true; do
> >               shift
> >               continue
> >               ;;
> > +     "-y")
> > +             BUILTIN=true
> > +             shift
> > +             continue
> > +             ;;
> >       "-O")
> >               if [ -d $2 ];then
> >                       OUTPUT=$(echo $2 | sed 's/\/*$//')
> > @@ -122,7 +129,13 @@ for MERGE_FILE in $MERGE_LIST ; do
> >               grep -q -w $CFG $TMP_FILE || continue
> >               PREV_VAL=$(grep -w $CFG $TMP_FILE)
> >               NEW_VAL=$(grep -w $CFG $MERGE_FILE)
> > -             if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> > +             if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then
>
> I think we can avoid the rather lengthy/forky "echo | grep" mechanism
> with POSIX substring parameter expansion (should work in bash, dash,
> etc.).
>
> http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02
>
> We can also be more consistent with the other tests in the script, consider:
>
>                 if [ "$BUILTIN" = "true" ] && [ "${NEW_VAL#CONFIG_*=}" = "m" ] && [ "${PREV_VAL#CONFIG_*=}" = "y" ]; then
>
>
> > +                     echo Value of $CFG is \'y\' and we don\'t want to redefine the fragment $MERGE_FILE:
>
> I think we can drop the above and perhaps update the last line with...
>
> > +                     echo Previous  value: $PREV_VAL
> > +                     echo New value:       $NEW_VAL
> > +                     echo Will use previous value.
>
> echo "-y passed, will not demote y to m"
>
> Or something along those lines.

Thank you for your input, I'll update and send a v2 shortly.

Cheers,
Anders

>
> > +                     echo
> > +             elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
> >                       echo Value of $CFG is redefined by fragment $MERGE_FILE:
> >                       echo Previous  value: $PREV_VAL
> >                       echo New value:       $NEW_VAL
> > --
> > 2.11.0
> >
> >
>
> --
> Darren Hart
> VMware Open Source Technology Center

Patch
diff mbox series

diff --git a/scripts/kconfig/merge_config.sh b/scripts/kconfig/merge_config.sh
index da66e7742282..902a60b45614 100755
--- a/scripts/kconfig/merge_config.sh
+++ b/scripts/kconfig/merge_config.sh
@@ -32,6 +32,7 @@  usage() {
 	echo "  -m    only merge the fragments, do not execute the make command"
 	echo "  -n    use allnoconfig instead of alldefconfig"
 	echo "  -r    list redundant entries when merging fragments"
+	echo "  -y    make builtin have precedence over modules"
 	echo "  -O    dir to put generated output files.  Consider setting \$KCONFIG_CONFIG instead."
 	echo
 	echo "Used prefix: '$CONFIG_PREFIX'. You can redefine it with \$CONFIG_ environment variable."
@@ -40,6 +41,7 @@  usage() {
 RUNMAKE=true
 ALLTARGET=alldefconfig
 WARNREDUN=false
+BUILTIN=false
 OUTPUT=.
 CONFIG_PREFIX=${CONFIG_-CONFIG_}
 
@@ -64,6 +66,11 @@  while true; do
 		shift
 		continue
 		;;
+	"-y")
+		BUILTIN=true
+		shift
+		continue
+		;;
 	"-O")
 		if [ -d $2 ];then
 			OUTPUT=$(echo $2 | sed 's/\/*$//')
@@ -122,7 +129,13 @@  for MERGE_FILE in $MERGE_LIST ; do
 		grep -q -w $CFG $TMP_FILE || continue
 		PREV_VAL=$(grep -w $CFG $TMP_FILE)
 		NEW_VAL=$(grep -w $CFG $MERGE_FILE)
-		if [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
+		if test "$BUILTIN" = "true" && echo $PREV_VAL |grep -Eq '^\w+=y' && echo $NEW_VAL |grep -Eq '^\w+=m' ; then
+			echo Value of $CFG is \'y\' and we don\'t want to redefine the fragment $MERGE_FILE:
+			echo Previous  value: $PREV_VAL
+			echo New value:       $NEW_VAL
+			echo Will use previous value.
+			echo
+		elif [ "x$PREV_VAL" != "x$NEW_VAL" ] ; then
 			echo Value of $CFG is redefined by fragment $MERGE_FILE:
 			echo Previous  value: $PREV_VAL
 			echo New value:       $NEW_VAL