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()"

[Ulf Magnusson]

On Tue, Feb 13, 2018 at 1:35 PM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>:
>> > >> 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@xxxxxxxxxxxxx>
>> > >>>      Reviewed-by: Ulf Magnusson <ulfalizer@xxxxxxxxx>
>> > >>>
>> > >>> 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

Shouldn't be a problem to back this one out either if it turns out to
cause massive amounts of pain in practice I guess, even if it's the
Debian tools doing something weird.

Good to look into what it is they're doing in any case.

Cheers,
Ulf