Date:   Tue, 13 Feb 2018 13:35:10 +0100
From:   Ulf Magnusson <ulfalizer@...il.com>
To:     Sander Eikelenboom <linux@...elenboom.it>
Cc:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Woody Suwalski <terraluna977@...il.com>, srivasta@...ian.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Linux 4.16-rc1: regression bisected, Debian kernel package tool
 make-kpkg stalls indefinitely during kernel build due to commit "kconfig:
 remove check_stdin()"

On Tue, Feb 13, 2018 at 12:33:24PM +0100, Ulf Magnusson wrote:
> On Tue, Feb 13, 2018 at 11:00:49AM +0100, Sander Eikelenboom wrote:
> > On 13/02/18 05:09, Masahiro Yamada wrote:
> > > 2018-02-13 12:00 GMT+09:00 Woody Suwalski <terraluna977@...il.com>:
> > >> Sander Eikelenboom wrote:
> > >>>
> > >>> L.S.,
> > >>>
> > >>> The Debian kernel-package tool make-kpkg for easy building of upstream
> > >>> kernels on Debian fails with linux 4.16-rc1.
> > >>>
> > >>> The tool (perl script) while invoked with:
> > >>>      make-kpkg --initrd --append_to_version -20180212 kernel_image
> > >>>
> > >>> On a git tree with a .config from the previous kernel release, so new
> > >>> KConfig questions have to be asked on new or changed options.
> > >>>
> > >>> The script stalls indefinitely while it seems to be excuting:
> > >>>      exec make kpkg_version=13.018+nmu1 -f
> > >>> /usr/share/kernel-package/ruleset/minimal.mk debian
> > >>> APPEND_TO_VERSION=-t440s-20180212  INITRD=YES
> > >>>
> > >>> After using ctrl-c to break out it, i get:
> > >>>     ^CFailed to create a ./debian directory: No such file or directory at
> > >>> /usr/bin/make-kpkg line 970.
> > >>>
> > >>> Bisection turned up as culprit:
> > >>>      commit d2a04648a5dbc3d1d043b35257364f0197d4d868
> > >>>      kconfig: remove check_stdin()
> > >>>           Except silentoldconfig, valid_stdin is 1, so check_stdin() is
> > >>> no-op.
> > >>>           oldconfig and silentoldconfig work almost in the same way except
> > >>> that
> > >>>      the latter generates additional files under include/.  Both ask users
> > >>>      for input for new symbols.
> > >>>           I do not know why only silentoldconfig requires stdio be tty.
> > >>>             $ rm -f .config; touch .config
> > >>>        $ yes "" | make oldconfig > stdout
> > >>>        $ rm -f .config; touch .config
> > >>>        $ yes "" | make silentoldconfig > stdout
> > >>>        make[1]: *** [silentoldconfig] Error 1
> > >>>        make: *** [silentoldconfig] Error 2
> > >>>        $ tail -n 4 stdout
> > >>>        Console input/output is redirected. Run 'make oldconfig' to update
> > >>> configuration.
> > >>>             scripts/kconfig/Makefile:40: recipe for target
> > >>> 'silentoldconfig' failed
> > >>>        Makefile:507: recipe for target 'silentoldconfig' failed
> > >>>           Redirection is useful, for example, for testing where we want to
> > >>> give
> > >>>      particular key inputs from a test file, then check the result.
> > >>>           Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> > >>>      Reviewed-by: Ulf Magnusson <ulfalizer@...il.com>
> > >>>
> > >>> Reverting this specific commit makes make-kpkg work again as usual.
> > >>>
> > >>> Version of the kernel-package used:
> > >>> ii  kernel-package
> > >>> 13.018+nmu1
> > >>>
> > >>>
> > >>> I also cc'ed the Debian developer who maintains the kernel-package
> > >>> package: Manoj Srivastava
> > >>>
> > >>> --
> > >>> Sander
> > >>>
> > >> I have noticed today the same - the kernel-build blockage was in (as I
> > >> recall)
> > >> srcipts/kconfig/conf -s --silentoldconfig Kbuild
> > >>
> > >> I have bypassed it by regenerating the .config "by hand"...
> > > 
> > > 
> > > silentoldconfig asks you values for new symbols.
> > > So, you must answer questions to proceed.
> > 
> > I know, but it stalls before asking the questions.
> >  
> > > 
> > > How does 'make-kpkg' handle silentoldconfig?
> > > 
> > > Re-direct stdio, then make it forcibly fail?
> > 
> > I don't know, it is a bunch of perl and shell scripts that gets invoked, not the most easy to comprehend if you are not familiar with them. I'm just a user of the tool.
> > 
> > So i would have to defer that question to the Debian package maintainer, hopefully he will chime in.
> > 
> > --
> > Sander
> > 
> > > 
> > > 
> > > 
> > 
> 
> Hello,
> 
> What the commit does is to make silentoldconfig not immediately exit(1)
> when both of the following apply:
> 
>     1. stdin is from something that's not a terminal
> 
>     2. New symbols are prompted for
> 
> All the outputs (.config and the include/generated/ and include/config/
> trees) are generated after the point where silentoldconfig would
> previously exit(1), afaics, so the effects of the commit can be
> summarized as follows:
> 
>     * If no new symbols appear (that would be prompted for), the
>       behavior is exactly the same as before.
>       
>       (check_stdin() never seems to get called unless a value would
>       actually be prompted for, which makes sense.)
> 
>     * If new symbols appear and stdin is a tty, the behavior is also
>       exactly the same as before.
> 
> And finally, the only case where the behavior differs:
> 
>     * If new symbols appear and stdin is not a tty, the
>       new behavior is to prompt (to expect e.g. "n"/"m"/"y" from
>       wherever stdin is coming from).
> 
>       The old behavior was to exit(1) without generating any output
>       files.
> 
> Hopefully that clarifies it.
> 
> The intent of the commit was just to clean up the code a bit, since
> there doesn't seem to be a good reason to bail just because stdin
> happens to not be a tty (plus it's inconsistent with the way oldconfig
> works, which never bailed).
> 
> silentoldconfig is something of an internal implementation detail at
> this point by the way. It's basically oldconfig + generate the
> configuration stuff in include/generated/ and include/config/ that
> actually gets used during the build. Normally it's called automatically
> as needed when building the kernel.
> 
> See https://www.spinics.net/lists/kernel/msg2720574.html for some more
> details re. silentoldconfig.
> 
> Cheers,
> Ulf

By the way: If you can figure out a reason for why that stdin check was
there before while looking into the Debian tool, then please tell us. 

The problem with uncommented mystery code like that is that it easily
looks pointless and is at risk of getting changed or removed, even if
there was a good reason for it (which I still suspect there wasn't
though).

</soapbox>

Cheers,
Ulf

Powered by blists - more mailing lists