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

[Andres Salomon <dilinger@queued.net>]

On Sun, 27 Apr 2008 22:47:44 +0200
Sam Ravnborg <sam@ravnborg.org> wrote:

> 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.


Yeah, I don't really like the name either, but I wasn't sure what to call
it.  I want behavior that is similar to silentoldconfig, but not because
I want it to not be chatty, but because I want it to fail if there have
been changes and there's no tty (ie, !valid_stdin).  So basically,
silentdefoldconfig or something ridiculously long like that. :)

> 
> 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.

Do I?  I'm not sure what you mean by "interactively apply".  I want
to non-interactively apply the defconfig, and fail if prompting is
required (rather than just choosing default values).

Perhaps _silentoldconfig would've been a better name.  Actually, I'm
pretty sure that it is. 

> 
> 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?

Sounds like you're saying that you want:

make oldconfig V=1   (chatty, prompt if possible or fail)
make oldconfig V=0   (silentoldconfig, prompt if possible or fail)

make defconfig V=1   (chatty, use defaults)
make defconfig V=0   (silent, use defaults)

make i386_oldconfig V=1 (chatty, prompt if possible or fail)
make i386_oldconfig V=0 (silent, prompt if possible or fail)

make i386_defconfig V=1 (chatty, use defaults)
make i386_defconfig V=0 (silent, use defaults)

Does that sound right?  Would using the build system's verbose variable
work?  If so, what should the default be?




> 
> 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.


Eh?  There's already a printf, this just adds an additional printf.


> 
> >  
> >  	@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.

I'm not sure what you mean.  This isn't really preparation for this patch;
it's just ensuring that we can use '-o' and '-D' together without
running a check for .config.  Basically, if '-o' is specified but '-D'
is not, check for .config (and fail if it doesn't exist.  If '-o' and '-D'
are both specified, we don't care about .config.

> 
> >  			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.

I don't believe it fails, it uses a default config name.  I'm not sure
if it fails if _that_ file isn't found, though.  I can't make much
sense of the symbol stuff..

We definitely _do_ want to fail if conf_read(defconfig_file) can't find
the file, and we definitely don't want to fail if conf_read(NULL) can't
open the file.


> 
> 	Sam