diff mbox

[-next,2/2] kbuild: fix for updated LZ4 tool with the new streaming format

Message ID 1367829775-4434-2-git-send-email-kyungsik.lee@lge.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kyungsik Lee May 6, 2013, 8:42 a.m. UTC
LZ4 has been updated with LZ4 Streaming Format specification(v1.3).
lz4demo is replaced by lz4c. lz4c supports both the new streaming and
legacy format with -l option.

This patch makes use of lz4c to support legacy format which is
used for LZ4 De/compression in the linux kernel.

Link: https://code.google.com/p/lz4/source/checkout
Signed-off-by: Kyungsik Lee <kyungsik.lee@lge.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Florian Fainelli <florian@openwrt.org>
Cc: Yann Collet <yann.collet.73@gmail.com>
Cc: Chanho Min <chanho.min@lge.com>
---
 scripts/Makefile.lib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Borislav Petkov May 6, 2013, 9:51 a.m. UTC | #1
On Mon, May 06, 2013 at 05:42:55PM +0900, Kyungsik Lee wrote:
> LZ4 has been updated with LZ4 Streaming Format specification(v1.3).
> lz4demo is replaced by lz4c. lz4c supports both the new streaming and
> legacy format with -l option.
> 
> This patch makes use of lz4c to support legacy format which is
> used for LZ4 De/compression in the linux kernel.
> 
> Link: https://code.google.com/p/lz4/source/checkout
> Signed-off-by: Kyungsik Lee <kyungsik.lee@lge.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Russell King <rmk@arm.linux.org.uk>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Florian Fainelli <florian@openwrt.org>
> Cc: Yann Collet <yann.collet.73@gmail.com>
> Cc: Chanho Min <chanho.min@lge.com>
> ---
>  scripts/Makefile.lib | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index a0ab6d7..c9bfbb0 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -313,7 +313,7 @@ cmd_lzo = (cat $(filter-out FORCE,$^) | \
>  
>  quiet_cmd_lz4 = LZ4     $@
>  cmd_lz4 = (cat $(filter-out FORCE,$^) | \
> -	lz4demo -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> +	lz4c -l -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \

You probably want to check for the presence of lz4c on the system and
bail with an informative message if absent.
Geert Uytterhoeven July 10, 2013, 8:12 a.m. UTC | #2
On Mon, May 6, 2013 at 11:51 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, May 06, 2013 at 05:42:55PM +0900, Kyungsik Lee wrote:
>> LZ4 has been updated with LZ4 Streaming Format specification(v1.3).
>> lz4demo is replaced by lz4c. lz4c supports both the new streaming and
>> legacy format with -l option.
>>
>> This patch makes use of lz4c to support legacy format which is
>> used for LZ4 De/compression in the linux kernel.
>>
>> Link: https://code.google.com/p/lz4/source/checkout
>> Signed-off-by: Kyungsik Lee <kyungsik.lee@lge.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Russell King <rmk@arm.linux.org.uk>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Florian Fainelli <florian@openwrt.org>
>> Cc: Yann Collet <yann.collet.73@gmail.com>
>> Cc: Chanho Min <chanho.min@lge.com>
>> ---
>>  scripts/Makefile.lib | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index a0ab6d7..c9bfbb0 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -313,7 +313,7 @@ cmd_lzo = (cat $(filter-out FORCE,$^) | \
>>
>>  quiet_cmd_lz4 = LZ4     $@
>>  cmd_lz4 = (cat $(filter-out FORCE,$^) | \
>> -     lz4demo -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
>> +     lz4c -l -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
>
> You probably want to check for the presence of lz4c on the system and
> bail with an informative message if absent.

Yep, x86_64-randconfig
(http://kisskb.ellerman.id.au/kisskb/buildresult/9110794/):

  LZ4     arch/x86/boot/compressed/vmlinux.bin.lz4
/bin/sh: lz4c: command not found

Also, several of the blackfin builds started failing due to a
compression-related
issue, e.g. http://kisskb.ellerman.id.au/kisskb/buildresult/9101322/:

  UIMAGE  arch/blackfin/boot/uImage.lzma

Invalid Compression Type - valid names are: none, bzip2, gzip
Usage: /usr/local/bin/mkimage -l image
          -l ==> list image header information
       /usr/local/bin/mkimage [-x] -A arch -O os -T type -C comp -a
addr -e ep -n name -d data_file[:data_file...] image
          -A ==> set architecture to 'arch'
          -O ==> set operating system to 'os'
          -T ==> set image type to 'type'
          -C ==> set compression type 'comp'
          -a ==> set load address to 'addr' (hex)
          -e ==> set entry point to 'ep' (hex)
          -n ==> set image name to 'name'
          -d ==> use image data from 'datafile'
          -x ==> set XIP (execute in place)
make[2]: *** [arch/blackfin/boot/uImage.lzma] Error 1

May be unrelated, though.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Borislav Petkov July 10, 2013, 9:36 a.m. UTC | #3
On Wed, Jul 10, 2013 at 10:12:46AM +0200, Geert Uytterhoeven wrote:
> >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> >> index a0ab6d7..c9bfbb0 100644
> >> --- a/scripts/Makefile.lib
> >> +++ b/scripts/Makefile.lib
> >> @@ -313,7 +313,7 @@ cmd_lzo = (cat $(filter-out FORCE,$^) | \
> >>
> >>  quiet_cmd_lz4 = LZ4     $@
> >>  cmd_lz4 = (cat $(filter-out FORCE,$^) | \
> >> -     lz4demo -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> >> +     lz4c -l -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> >
> > You probably want to check for the presence of lz4c on the system and
> > bail with an informative message if absent.
> 
> Yep, x86_64-randconfig
> (http://kisskb.ellerman.id.au/kisskb/buildresult/9110794/):
> 
>   LZ4     arch/x86/boot/compressed/vmlinux.bin.lz4
> /bin/sh: lz4c: command not found

Sure, it had to be 2 months and to hit upstream for there still to be no
fix. Geez...

Anyone reading this and willing to try his skills with an
introductory-level patch, feel free to address this. I'll help testing
and upstreaming it.

Thanks.
Ingo Molnar July 12, 2013, 7:56 a.m. UTC | #4
* Kyungsik Lee <kyungsik.lee@lge.com> wrote:

> LZ4 has been updated with LZ4 Streaming Format specification(v1.3).
> lz4demo is replaced by lz4c. lz4c supports both the new streaming and
> legacy format with -l option.
> 
> This patch makes use of lz4c to support legacy format which is
> used for LZ4 De/compression in the linux kernel.
> 
> Link: https://code.google.com/p/lz4/source/checkout
> Signed-off-by: Kyungsik Lee <kyungsik.lee@lge.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Russell King <rmk@arm.linux.org.uk>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Florian Fainelli <florian@openwrt.org>
> Cc: Yann Collet <yann.collet.73@gmail.com>
> Cc: Chanho Min <chanho.min@lge.com>
> ---
>  scripts/Makefile.lib | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index a0ab6d7..c9bfbb0 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -313,7 +313,7 @@ cmd_lzo = (cat $(filter-out FORCE,$^) | \
>  
>  quiet_cmd_lz4 = LZ4     $@
>  cmd_lz4 = (cat $(filter-out FORCE,$^) | \
> -	lz4demo -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> +	lz4c -l -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
>  	(rm -f $@ ; false)

None of the major distros I tried has the lz4 or lz4c binary available, 
and this breaks randconfig builds:

  /bin/sh: lz4c: command not found

If:

 CONFIG_HAVE_KERNEL_LZ4=y
 CONFIG_KERNEL_LZ4=y
 CONFIG_RD_LZ4=y
 CONFIG_LZ4_DECOMPRESS=y
 CONFIG_DECOMPRESS_LZ4=y

If a utility is not widely available yet and if the utility is not in the 
kernel proper, could you please at least make sure that randconfig does 
not stumble over non-buildable kernels?

Thanks,

	Ingo
Borislav Petkov July 12, 2013, 8:01 a.m. UTC | #5
On Fri, Jul 12, 2013 at 09:56:02AM +0200, Ingo Molnar wrote:
> If a utility is not widely available yet and if the utility is not in
> the kernel proper, could you please at least make sure that randconfig
> does not stumble over non-buildable kernels?

Two months ago I've complained about this and other people have burned
themselves too in the meantime...
Ingo Molnar July 12, 2013, 10:13 a.m. UTC | #6
* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jul 12, 2013 at 09:56:02AM +0200, Ingo Molnar wrote:
> >
> > If a utility is not widely available yet and if the utility is not in 
> > the kernel proper, could you please at least make sure that randconfig 
> > does not stumble over non-buildable kernels?
> 
> Two months ago I've complained about this and other people have burned 
> themselves too in the meantime...

and now it is upstream already, via -mm:

   e76e1fdfa8f8 lib: add support for LZ4-compressed kernel


Thanks,

	Ingo
Borislav Petkov July 12, 2013, 10:23 a.m. UTC | #7
On Fri, Jul 12, 2013 at 12:13:19PM +0200, Ingo Molnar wrote:
> and now it is upstream already, via -mm:
> 
>    e76e1fdfa8f8 lib: add support for LZ4-compressed kernel

Sure. Let's see if it manages to get released b0rked like it is right
now.
Andrew Morton July 12, 2013, 11:18 a.m. UTC | #8
On Fri, 12 Jul 2013 12:23:26 +0200 Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jul 12, 2013 at 12:13:19PM +0200, Ingo Molnar wrote:
> > and now it is upstream already, via -mm:
> > 
> >    e76e1fdfa8f8 lib: add support for LZ4-compressed kernel
> 
> Sure. Let's see if it manages to get released b0rked like it is right
> now.

Well y'know, if whining fixed bugs then there would be no problem here
at all.

This problem came up briefly and I thought it had been adequately
addressed.  Now a month or two later we find out who are the people who
can't be bothered testing -next, but think it terribly important that
others do so.
Ingo Molnar July 12, 2013, 12:06 p.m. UTC | #9
* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 12 Jul 2013 12:23:26 +0200 Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Fri, Jul 12, 2013 at 12:13:19PM +0200, Ingo Molnar wrote:
> > > and now it is upstream already, via -mm:
> > > 
> > >    e76e1fdfa8f8 lib: add support for LZ4-compressed kernel
> > 
> > Sure. Let's see if it manages to get released b0rked like it is right
> > now.
> 
> Well y'know, if whining fixed bugs then there would be no problem here 
> at all.
> 
> This problem came up briefly and I thought it had been adequately 
> addressed.  Now a month or two later we find out who are the people who 
> can't be bothered testing -next, but think it terribly important that 
> others do so.

At least with my limited resources -next is unusable for my randconfig 
testing. I can randconfig test the fresh new bugs I introduce plus the 
bugs that slip upstream - which naturally peak during the merge window.

Anyway, I hacked it around locally, sh*t happens, it should be easy enough 
to fix.

Thanks,

	Ingo
Borislav Petkov July 12, 2013, 12:13 p.m. UTC | #10
On Fri, Jul 12, 2013 at 04:18:10AM -0700, Andrew Morton wrote:
> Well y'know, if whining fixed bugs then there would be no problem here
> at all.

I know. I decided to whine this time and not try to fix it because
attempting to fix every fallout I'm seeing in the tree with randconfigs
is a full-time job and the *actual* job doesn't see any progress.

And in all fairness, if someone were reporting an issue against my code,
I'll try to do my very best to fix it as quickly aas possible, instead
of ignoring it.

So actually, the right thing to do in such cases is back out half-cooked
stuff from the tree until it gets fixed. That'll give the authors the
right incentive. But this is just me.

> This problem came up briefly and I thought it had been adequately
> addressed. Now a month or two later we find out who are the people who
> can't be bothered testing -next, but think it terribly important that
> others do so.

Yeah, if everyone would put their money where their mouth is...
Florian Fainelli July 12, 2013, 12:34 p.m. UTC | #11
Hello,

2013/7/12 Borislav Petkov <bp@alien8.de>:
> On Fri, Jul 12, 2013 at 04:18:10AM -0700, Andrew Morton wrote:
>> Well y'know, if whining fixed bugs then there would be no problem here
>> at all.
>
> I know. I decided to whine this time and not try to fix it because
> attempting to fix every fallout I'm seeing in the tree with randconfigs
> is a full-time job and the *actual* job doesn't see any progress.
>
> And in all fairness, if someone were reporting an issue against my code,
> I'll try to do my very best to fix it as quickly aas possible, instead
> of ignoring it.
>
> So actually, the right thing to do in such cases is back out half-cooked
> stuff from the tree until it gets fixed. That'll give the authors the
> right incentive. But this is just me.
>
>> This problem came up briefly and I thought it had been adequately
>> addressed. Now a month or two later we find out who are the people who
>> can't be bothered testing -next, but think it terribly important that
>> others do so.
>
> Yeah, if everyone would put their money where their mouth is...

Shall we provide a shell script wrapper for all of these compressor
commands which would test for the utility presence and issue a BIG FAT
WARNING if not, but still create the file not to make the kernel build
fail? I guess we could probably do this in the Makefile directly as
well.
--
Florian
Borislav Petkov July 12, 2013, 12:46 p.m. UTC | #12
On Fri, Jul 12, 2013 at 01:34:38PM +0100, Florian Fainelli wrote:
> Shall we provide a shell script wrapper for all of these compressor
> commands which would test for the utility presence and issue a BIG FAT
> WARNING if not, but still create the file not to make the kernel build
> fail? I guess we could probably do this in the Makefile directly as
> well.

I don't know how that would work with the main kernel Makefile
but we do those kinds of tests in perf before we build it so
that we can save ourselves the time spent on a failed build:
2209001fd895e8932ae2c85bfca233758234499a.

This should probably work if you add a prerequisite target which runs
early in the build process and checks for all the userspace tools
required for the build.

Thanks.
Andrew Morton July 15, 2013, 10:03 p.m. UTC | #13
On Fri, 12 Jul 2013 09:56:02 +0200 Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Kyungsik Lee <kyungsik.lee@lge.com> wrote:
> >  quiet_cmd_lz4 = LZ4     $@
> >  cmd_lz4 = (cat $(filter-out FORCE,$^) | \
> > -	lz4demo -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> > +	lz4c -l -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> >  	(rm -f $@ ; false)
> 
> None of the major distros I tried has the lz4 or lz4c binary available, 
> and this breaks randconfig builds:
> 
>   /bin/sh: lz4c: command not found
> 
> If:
> 
>  CONFIG_HAVE_KERNEL_LZ4=y
>  CONFIG_KERNEL_LZ4=y
>  CONFIG_RD_LZ4=y
>  CONFIG_LZ4_DECOMPRESS=y
>  CONFIG_DECOMPRESS_LZ4=y
> 
> If a utility is not widely available yet and if the utility is not in the 
> kernel proper, could you please at least make sure that randconfig does 
> not stumble over non-buildable kernels?

I don't know how to do this.  Any suggestions?

It has to be done at `make config' time.  We'd need to probe for the
presence of lz4c and then....  what?

Is there any precedent for this?

I don't think we can just ignore the absence of lz4c - the user has
selected a config which his system cannot build.  The problem lies
within randconfig itself.
H. Peter Anvin July 15, 2013, 10:08 p.m. UTC | #14
On 07/15/2013 03:03 PM, Andrew Morton wrote:
> 
> I don't know how to do this.  Any suggestions?
> 
> It has to be done at `make config' time.  We'd need to probe for the
> presence of lz4c and then....  what?
> 
> Is there any precedent for this?
> 
> I don't think we can just ignore the absence of lz4c - the user has
> selected a config which his system cannot build.  The problem lies
> within randconfig itself.
> 

We keep running over the need to be able to have kconfig run tests on
the build system (for toolchain support or for optional tools needed);
running them in the Makefiles (i.e. at Kbuild time) is simply too late.

	-hpa
Andrew Morton July 16, 2013, 7:47 a.m. UTC | #15
On Mon, 15 Jul 2013 15:08:20 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:

> On 07/15/2013 03:03 PM, Andrew Morton wrote:
> > 
> > I don't know how to do this.  Any suggestions?
> > 
> > It has to be done at `make config' time.  We'd need to probe for the
> > presence of lz4c and then....  what?
> > 
> > Is there any precedent for this?
> > 
> > I don't think we can just ignore the absence of lz4c - the user has
> > selected a config which his system cannot build.  The problem lies
> > within randconfig itself.
> > 
> 
> We keep running over the need to be able to have kconfig run tests on
> the build system (for toolchain support or for optional tools needed);
> running them in the Makefiles (i.e. at Kbuild time) is simply too late.
> 

Would it make sense to extend Kconfig's `depends'?

	depends on $(shell-command)

I don't know how practical that would be to implement...
Andrew Morton July 16, 2013, 7:56 a.m. UTC | #16
On Tue, 16 Jul 2013 00:47:27 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 15 Jul 2013 15:08:20 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:
> 
> > On 07/15/2013 03:03 PM, Andrew Morton wrote:
> > > 
> > > I don't know how to do this.  Any suggestions?
> > > 
> > > It has to be done at `make config' time.  We'd need to probe for the
> > > presence of lz4c and then....  what?
> > > 
> > > Is there any precedent for this?
> > > 
> > > I don't think we can just ignore the absence of lz4c - the user has
> > > selected a config which his system cannot build.  The problem lies
> > > within randconfig itself.
> > > 
> > 
> > We keep running over the need to be able to have kconfig run tests on
> > the build system (for toolchain support or for optional tools needed);
> > running them in the Makefiles (i.e. at Kbuild time) is simply too late.
> > 
> 
> Would it make sense to extend Kconfig's `depends'?
> 
> 	depends on $(shell-command)
> 
> I don't know how practical that would be to implement...

Or, easier and faster, run some front-end script which generates
once-off Kconfig symbols.

	if [ -x /bin/lz4c ]
	then
		echo CONFIG_HAVE_LZ4C
	fi

then munge the output of that script into the Kconfig run and do

	depends on HAVE_LZ4C
Yann E. MORIN July 16, 2013, 8:08 a.m. UTC | #17
Andrew, All,

On Tuesday 16 July 2013 09:56:11 Andrew Morton wrote:
> On Tue, 16 Jul 2013 00:47:27 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Mon, 15 Jul 2013 15:08:20 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:
> > > On 07/15/2013 03:03 PM, Andrew Morton wrote:
[--SNIP--]
> > > We keep running over the need to be able to have kconfig run tests on
> > > the build system (for toolchain support or for optional tools needed);
> > > running them in the Makefiles (i.e. at Kbuild time) is simply too late.
> > > 
> > 
> > Would it make sense to extend Kconfig's `depends'?
> > 
> > 	depends on $(shell-command)
> > 
> > I don't know how practical that would be to implement...

I'm afraid this will get rather ugly and not trivial.

Can we mix 'depends on SYMBOL' and 'depends on $(command)' ?
Can we mix both in a boolean expression such as 'depends on SYMBOL
&& $(command)' ?

What would be the condition for evaluating the dependency rule? Evaluation
at Kconfig read-time might not be enough, given this construct:

    config FOO
        depends on $(foo)
    comment "'foo' is missing, please install it"
        depends on !$(foo)

Also, I believe Kconfig should stay a config-only language, without
much esoteric features.

> Or, easier and faster, run some front-end script which generates
> once-off Kconfig symbols.
> 
> 	if [ -x /bin/lz4c ]
> 	then
> 		echo CONFIG_HAVE_LZ4C
> 	fi
> 
> then munge the output of that script into the Kconfig run and do
> 
> 	depends on HAVE_LZ4C

Yes, this is a better solution.

For what it's worth, this is what I'm doing in crosstool-NG: a script
checks for optional pre-requisites, spits out a Kconfig blob which is
included by the top-level Kconfig file.

Here is a snippet of generated Kconfig blob:
    config HAVE_XZ
        def_bool y
    config HAVE_LZMA
        bool

Which means we do have 'xz', but not 'lzma'. This is relatively trivial
to do, so I'll tackle this this evening when I'm back home (unless
someone beats me to it).

Regards,
Yann E. MORIN.
Andrew Morton July 16, 2013, 8:27 a.m. UTC | #18
On Tue, 16 Jul 2013 10:08:07 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > Or, easier and faster, run some front-end script which generates
> > once-off Kconfig symbols.
> > 
> > 	if [ -x /bin/lz4c ]
> > 	then
> > 		echo CONFIG_HAVE_LZ4C
> > 	fi
> > 
> > then munge the output of that script into the Kconfig run and do
> > 
> > 	depends on HAVE_LZ4C
> 
> Yes, this is a better solution.
> 
> For what it's worth, this is what I'm doing in crosstool-NG: a script
> checks for optional pre-requisites, spits out a Kconfig blob which is
> included by the top-level Kconfig file.
> 
> Here is a snippet of generated Kconfig blob:
>     config HAVE_XZ
>         def_bool y
>     config HAVE_LZMA
>         bool
> 
> Which means we do have 'xz', but not 'lzma'. This is relatively trivial
> to do, so I'll tackle this this evening when I'm back home (unless
> someone beats me to it).

Cool, thanks.

It seems a bit inefficient to be evaluating commands in a Kconfig
setting where doesn't need that done.  For example, if someone has
selected LZO compression then they don't need to probe for lz4c.

That example sounds hard to solve in a simple fashion, but what about
the case where the config system is not going to look at an entire
subsystem?  Suppose for example, drivers/media/firewire wants to probe
for some executable, but the user hasn't selected firewire at all.

What I'm angling at is, rather than a single global front-end script,
can we embed the scripts in some fashion within the various Kconfig
files?  Say,

	script ./some-script.sh

and the config system will only evaluate that command if it is working
on that Kconfig file.  Obviously that requires a multiple-pass thing.

It's late, but you see what I mean ;)
Borislav Petkov July 16, 2013, 9:05 a.m. UTC | #19
On Tue, Jul 16, 2013 at 01:27:56AM -0700, Andrew Morton wrote:
> What I'm angling at is, rather than a single global front-end script,
> can we embed the scripts in some fashion within the various Kconfig
> files?  Say,
> 
> 	script ./some-script.sh
> 
> and the config system will only evaluate that command if it is working
> on that Kconfig file.  Obviously that requires a multiple-pass thing.

What's wrong with simply grepping the .config we've just created for
*enabled* symbols which require userspace support, check for the
presence of said support and bail out if none?

I.e., those steps:

1. make <whatever>config
2. make => A prereq. target runs the shell script.
Yann E. MORIN July 16, 2013, 9:12 a.m. UTC | #20
Borislav, All,

On Tuesday 16 July 2013 11:05:46 Borislav Petkov wrote:
> On Tue, Jul 16, 2013 at 01:27:56AM -0700, Andrew Morton wrote:
> > What I'm angling at is, rather than a single global front-end script,
> > can we embed the scripts in some fashion within the various Kconfig
> > files?  Say,
> > 
> > 	script ./some-script.sh
> > 
> > and the config system will only evaluate that command if it is working
> > on that Kconfig file.  Obviously that requires a multiple-pass thing.
> 
> What's wrong with simply grepping the .config we've just created for
> *enabled* symbols which require userspace support, check for the
> presence of said support and bail out if none?

The goal is to avoid generating a non-buildable kernel in the first place.
This is very usefull for automatic check harness that run randconfig, for
example (but may have more use-cases).

> I.e., those steps:
> 1. make <whatever>config
> 2. make => A prereq. target runs the shell script.

This is too late, since this will bail out, and will give a false-positive
failure.

While if we were to check for _known_ needed tools before calling into
Kconfig, we can simply hide non-available config options, and even inform
the user *why* the option is not available.

Regards,
Yann E. MORIN.
Florian Fainelli July 16, 2013, 9:13 a.m. UTC | #21
2013/7/16 Andrew Morton <akpm@linux-foundation.org>:
> On Tue, 16 Jul 2013 00:47:27 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> On Mon, 15 Jul 2013 15:08:20 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:
>>
>> > On 07/15/2013 03:03 PM, Andrew Morton wrote:
>> > >
>> > > I don't know how to do this.  Any suggestions?
>> > >
>> > > It has to be done at `make config' time.  We'd need to probe for the
>> > > presence of lz4c and then....  what?
>> > >
>> > > Is there any precedent for this?
>> > >
>> > > I don't think we can just ignore the absence of lz4c - the user has
>> > > selected a config which his system cannot build.  The problem lies
>> > > within randconfig itself.
>> > >
>> >
>> > We keep running over the need to be able to have kconfig run tests on
>> > the build system (for toolchain support or for optional tools needed);
>> > running them in the Makefiles (i.e. at Kbuild time) is simply too late.
>> >
>>
>> Would it make sense to extend Kconfig's `depends'?
>>
>>       depends on $(shell-command)
>>
>> I don't know how practical that would be to implement...
>
> Or, easier and faster, run some front-end script which generates
> once-off Kconfig symbols.
>
>         if [ -x /bin/lz4c ]
>         then
>                 echo CONFIG_HAVE_LZ4C
>         fi
>
> then munge the output of that script into the Kconfig run and do
>
>         depends on HAVE_LZ4C

That does sound nice to avoid the build error, but will eventually
make it harder to diagnose why the kernel has not been compressed with
the specific compression tool, some kind of warning should also be
emitted maybe?
--
Florian
Borislav Petkov July 16, 2013, 9:22 a.m. UTC | #22
On Tue, Jul 16, 2013 at 11:12:17AM +0200, Yann E. MORIN wrote:
> The goal is to avoid generating a non-buildable kernel in the first place.

I know, that's why I'm proposing to fail the build with the script run
as one of the first things by make.

> > I.e., those steps:
> > 1. make <whatever>config
> > 2. make => A prereq. target runs the shell script.
> 
> This is too late, since this will bail out, and will give a false-positive
> failure.

Huh? This will tell you: "you've selected X, but I don't have toolX
needed for the build".

You install the tool and run make again. Checks pass, the build
continues.

> While if we were to check for _known_ needed tools before calling into
> Kconfig,

How would you do that? How do you know what the user is going to select
*before* she even selects it?

You need to have a prepared .config to act upon.
Yann E. MORIN July 16, 2013, 9:25 a.m. UTC | #23
Florian, All,

On Tuesday 16 July 2013 11:13:24 Florian Fainelli wrote:
> 2013/7/16 Andrew Morton <akpm@linux-foundation.org>:
> > On Tue, 16 Jul 2013 00:47:27 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> >> On Mon, 15 Jul 2013 15:08:20 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:
> >>
> >> > On 07/15/2013 03:03 PM, Andrew Morton wrote:
> >> > >
> >> > > I don't know how to do this.  Any suggestions?
> >> > >
> >> > > It has to be done at `make config' time.  We'd need to probe for the
> >> > > presence of lz4c and then....  what?
> >> > >
> >> > > Is there any precedent for this?
> >> > >
> >> > > I don't think we can just ignore the absence of lz4c - the user has
> >> > > selected a config which his system cannot build.  The problem lies
> >> > > within randconfig itself.
> >> > >
> >> >
> >> > We keep running over the need to be able to have kconfig run tests on
> >> > the build system (for toolchain support or for optional tools needed);
> >> > running them in the Makefiles (i.e. at Kbuild time) is simply too late.
> >> >
> >>
> >> Would it make sense to extend Kconfig's `depends'?
> >>
> >>       depends on $(shell-command)
> >>
> >> I don't know how practical that would be to implement...
> >
> > Or, easier and faster, run some front-end script which generates
> > once-off Kconfig symbols.
> >
> >         if [ -x /bin/lz4c ]
> >         then
> >                 echo CONFIG_HAVE_LZ4C
> >         fi
> >
> > then munge the output of that script into the Kconfig run and do
> >
> >         depends on HAVE_LZ4C
> 
> That does sound nice to avoid the build error, but will eventually
> make it harder to diagnose why the kernel has not been compressed with
> the specific compression tool, some kind of warning should also be
> emitted maybe?

Either a warning at check-time, or a comment in Kconfig, such as:
    config COMPRESS_LZ4
        depends on HAVE_LZ4
    comment "'lz4' missing, LZ4 compression not available"
        depends on !HAVE_LZ4

Either way is fine with me. ;-)

Regards,
Yann E. MORIN.
Yann E. MORIN July 16, 2013, 9:32 a.m. UTC | #24
Borislav, All,

On Tuesday 16 July 2013 11:22:42 Borislav Petkov wrote:
> On Tue, Jul 16, 2013 at 11:12:17AM +0200, Yann E. MORIN wrote:
> > The goal is to avoid generating a non-buildable kernel in the first place.
> 
> I know, that's why I'm proposing to fail the build with the script run
> as one of the first things by make.

No, this is already too late: the given .config *is* already broken and
unbuildable.

> > > I.e., those steps:
> > > 1. make <whatever>config
> > > 2. make => A prereq. target runs the shell script.
> > 
> > This is too late, since this will bail out, and will give a false-positive
> > failure.
> 
> Huh? This will tell you: "you've selected X, but I don't have toolX
> needed for the build".
> 
> You install the tool and run make again. Checks pass, the build
> continues.

That does not work in two cases:
  - older distros that do not have the tool packaged
  - automatic test-harness that run thousands of randconfig a day

In the first case, this /could/ be overcome by the user compiling and
installing the package manually, but is mostly undoable in enterprise
environment with shared build machines, where each user would have to
install the same tool(s) again and again.

In the second case, this would yield a lot of false-positive in daily
reports.

Your solution mostly works with minimum overhead for a human user
interactively building a kernel, not with automated test harnesses.

> > While if we were to check for _known_ needed tools before calling into
> > Kconfig,
> 
> How would you do that? How do you know what the user is going to select
> *before* she even selects it?

The idea is to *avoid* the user being able to select unavailable options.

> You need to have a prepared .config to act upon.

The idea is to be always have a .config that is buildable with the current
toolset of the system, especially for esoteric and/or recent tools that are
not packaged and/or installed by default by the distros.

Regards,
Yann E. MORIN.
Borislav Petkov July 16, 2013, 9:38 a.m. UTC | #25
On Tue, Jul 16, 2013 at 11:32:39AM +0200, Yann E. MORIN wrote:
> No, this is already too late: the given .config *is* already broken and
> unbuildable.

Yes, and we don't build it because we fail the build.

> That does not work in two cases:
>   - older distros that do not have the tool packaged

This point is moot - distro kernels are built by the distro people.

>   - automatic test-harness that run thousands of randconfig a day

randconfigs are not guaranteed to build successfully.

> The idea is to be always have a .config that is buildable with the
> current toolset of the system, especially for esoteric and/or recent
> tools that are not packaged and/or installed by default by the
> distros.

How are you going to tell the user running make <something>config to
install the missing tools?
Yann E. MORIN July 16, 2013, 9:45 a.m. UTC | #26
Borislav, All,

On Tuesday 16 July 2013 11:38:20 Borislav Petkov wrote:
> On Tue, Jul 16, 2013 at 11:32:39AM +0200, Yann E. MORIN wrote:
> > That does not work in two cases:
> >   - older distros that do not have the tool packaged
> 
> This point is moot - distro kernels are built by the distro people.

It's not about building distro kernels. It's about building the current
kernel on a distro, for example for cross-compilation.

> >   - automatic test-harness that run thousands of randconfig a day
> randconfigs are not guaranteed to build successfully.

Well, mostly agreed. However, there was a request (in this thread) that
using such new tools that are not yet widely available in distros, does
not break randconfig.

This is a *new* requirement.

> > The idea is to be always have a .config that is buildable with the
> > current toolset of the system, especially for esoteric and/or recent
> > tools that are not packaged and/or installed by default by the
> > distros.
> How are you going to tell the user running make <something>config to
> install the missing tools?

As discussed with Florian in this thread, either one of:
  - a warning at check time
  - a comment in Kconfig

which both are easy enough to achieve. As I said earlier in this thread,
I've been using a similar scheme in crosstool-NG (which uses Kconfig as
its configuration infrastructure), and it has been proven effective
over the past feew years.

Regards,
Yann E. MORIN.
Borislav Petkov July 16, 2013, 9:59 a.m. UTC | #27
On Tue, Jul 16, 2013 at 11:45:27AM +0200, Yann E. MORIN wrote:
> As discussed with Florian in this thread, either one of:
>   - a warning at check time
>   - a comment in Kconfig

Whatever you do, make sure to make it quite clear to users what she
needs to do.

What I really have a problem with is options being hidden just because I
haven't selected something they depend on and having to run around the
Kconfig maze to find what I also need to select in order for the option
to appear.

Thanks.
Florian Fainelli July 16, 2013, 10:04 a.m. UTC | #28
2013/7/16 Borislav Petkov <bp@alien8.de>:
> On Tue, Jul 16, 2013 at 11:45:27AM +0200, Yann E. MORIN wrote:
>> As discussed with Florian in this thread, either one of:
>>   - a warning at check time
>>   - a comment in Kconfig
>
> Whatever you do, make sure to make it quite clear to users what she
> needs to do.
>
> What I really have a problem with is options being hidden just because I
> haven't selected something they depend on and having to run around the
> Kconfig maze to find what I also need to select in order for the option
> to appear.

Yes, me too, which is why I suggested the warning, as in "a warning
printed to stdout/stderr" rather than a Kconfig comment which will be
easy to be ignored.
--
Florian
Sam Ravnborg July 16, 2013, 6:04 p.m. UTC | #29
On Tue, Jul 16, 2013 at 10:08:07AM +0200, Yann E. MORIN wrote:
> Andrew, All,
> 
> On Tuesday 16 July 2013 09:56:11 Andrew Morton wrote:
> > On Tue, 16 Jul 2013 00:47:27 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> > > On Mon, 15 Jul 2013 15:08:20 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:
> > > > On 07/15/2013 03:03 PM, Andrew Morton wrote:
> [--SNIP--]
> > > > We keep running over the need to be able to have kconfig run tests on
> > > > the build system (for toolchain support or for optional tools needed);
> > > > running them in the Makefiles (i.e. at Kbuild time) is simply too late.
> > > > 
> > > 
> > > Would it make sense to extend Kconfig's `depends'?
> > > 
> > > 	depends on $(shell-command)
> > > 
> > > I don't know how practical that would be to implement...
> 
> I'm afraid this will get rather ugly and not trivial.
> 
> Can we mix 'depends on SYMBOL' and 'depends on $(command)' ?
> Can we mix both in a boolean expression such as 'depends on SYMBOL
> && $(command)' ?
> 
> What would be the condition for evaluating the dependency rule? Evaluation
> at Kconfig read-time might not be enough, given this construct:
> 
>     config FOO
>         depends on $(foo)
>     comment "'foo' is missing, please install it"
>         depends on !$(foo)
> 
> Also, I believe Kconfig should stay a config-only language, without
> much esoteric features.

We could extend the symbol option part to retreive values from a binary.
Something like this:

config FOOBAR
        bool
        option exec="true"

FOOBAR would assume the value "y" if the command true has exit code == 0, otherwise "n".
And similar conversions for other types.

This only extendt Kconfig slightly - using an already present method to import
external values.

The drawback I see with this approach is that we may execute a lot of small programs
where the value is never used.
Implementing lazy evaluation of a symbol value will not be easy I think.

	Sam
diff mbox

Patch

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index a0ab6d7..c9bfbb0 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -313,7 +313,7 @@  cmd_lzo = (cat $(filter-out FORCE,$^) | \
 
 quiet_cmd_lz4 = LZ4     $@
 cmd_lz4 = (cat $(filter-out FORCE,$^) | \
-	lz4demo -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
+	lz4c -l -c1 stdin stdout && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
 	(rm -f $@ ; false)
 
 # U-Boot mkimage