Re: [PATCH 4/4] kconfig: add *_silentdefconfig feature for config targets

[Sam Ravnborg <sam@ravnborg.org>]

On Fri, Apr 25, 2008 at 10:35:30PM -0400, Andres Salomon wrote:
> 
> Being able to run 'silentoldconfig' with an existing .config has been
> immensely useful, especially for automated builds.  If the kernel code
> changes in an incompatible manner without the associated .config being
> updated, the build will fail and call attention to the need for an update.
> 
> AFAICT, there is nothing similar when using *_defconfig; one must copy
> a .config manually, and then run silentoldconfig.  Simply running the
> associated _defconfig will quietly update the config (which may silently
> drop config options).  This patch adds a *_silentdefconfig target, with
> semantics similar to silentoldconfig.  It will take the defconfig from
> arch/$(SRCARCH)/configs/$x_defconfig, check for changes, and if there are
> none, write out a .config.  If there have been changes and stdin is
> valid, it will prompt for updates.  If there have been changes and
> stdin is not valid, it will bail out with an error.

I like what you achieve by this patchset.
But I do not agree on the naming you chose.

We have today:
oldconfig	=> very chatty
silentoldconfig	=> Asks only relevant questions
defconfig	=> silent

[I plan one day to make oldconfig behave like
silentoldconfig and drop the chatty mode]

And I see why you went for the name *_silentdefconfig
But in reality what we want to say is that we want to
interactively apply the _defconfig.

So if we could come up with something where we told
that we want to interactively use i386_defconfig
then the users would hopefully be less confused.

I have considered a few way to do so:

a) make I=1 i386_defconfig
b) make i_i386_defconfig
c) make ii386_defconfig
d) make i386_config

And none of these are actually good.
Any better ideas here?

See a few comments below.

	Sam


> 
> Signed-off-by: Andres Salomon <dilinger@debian.org>
> ---
>  Makefile                 |    4 ++++
>  scripts/kconfig/Makefile |    3 +++
>  scripts/kconfig/conf.c   |   13 +++++++++++--
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e77149e..c264f7f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1225,6 +1225,10 @@ help:
>  		$(foreach b, $(boards), \
>  		printf "  %-24s - Build for %s\\n" $(b) $(subst _defconfig,,$(b));) \
>  		echo '')
> +	@$(if $(boards), \
> +		$(foreach b, $(boards), \
> +		printf "  %-24s - Quiet Build for %s\\n" $(subst _defconfig,_silentdefconfig,$(b)) $(subst _defconfig,,$(b));) \
> +		echo '')
This is the first time we use printf in the top-level Makefile.
Most likely because I never use printf in my shell scripts
so I guess this is not a problem.

>  
>  	@echo  '  make V=0|1 [targets] 0 => quiet build (default), 1 => verbose build'
>  	@echo  '  make V=2   [targets] 2 => give reason for rebuild of target'
> diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
> index ce7d754..19ba562 100644
> --- a/scripts/kconfig/Makefile
> +++ b/scripts/kconfig/Makefile
> @@ -72,6 +72,9 @@ endif
>  %_defconfig: $(obj)/conf
>  	$(Q)$< -d -D arch/$(SRCARCH)/configs/$@ $(Kconfig)
>  
> +%_silentdefconfig: $(obj)/conf
> +	$(Q)$< -s -o -D arch/$(SRCARCH)/configs/$(subst _silentdefconfig,_defconfig,$@) $(Kconfig)
> +
>  # Help text used by make help
>  help:
>  	@echo  '  config	  - Update current config utilising a line-oriented program'
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 9a27638..264eee9 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -558,7 +558,8 @@ int main(int ac, char **av)
>  		}
>  		break;
>  	case ask_new:
> -		if (silent_mode && stat(".config", &tmpstat)) {
> +		if (!defconfig_file && silent_mode &&
> +				stat(".config", &tmpstat)) {

This belong in a preparation patch. We should handle this
also if we do not do so from the Makefile.

>  			printf(_("***\n"
>  				"*** You have not yet configured your kernel!\n"
>  				"*** (missing kernel .config file)\n"
> @@ -570,7 +571,15 @@ int main(int ac, char **av)
>  		}
>  		/* fall through */
>  	case ask_all:
> -		conf_read(NULL);
> +		if (defconfig_file) {
> +			if (conf_read(defconfig_file)) {
> +				printf(_("***\n*** Can't find default "
> +					 "configuration \"%s\"!\n***\n"),
> +					 defconfig_file);
> +				exit(1);
> +			}
> +		} else
> +			conf_read(NULL);

Does conf_read() fail if we use the NULL argument?
I assume not so the above code can be simplified and
should also be in the same preparational patch as the change above.

	Sam