diff mbox

linux: Build and install kernel selftests

Message ID 1448409222-4510-2-git-send-email-cyrilbur@gmail.com
State Superseded
Headers show

Commit Message

Cyril Bur Nov. 24, 2015, 11:53 p.m. UTC 
This patch simply adds the ability to compile and install the kernel
selftests into the target.

This is likely to be a rarely used debugging/performance feature for
development and unlikely to be used in a production configuration.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 linux/Config.tools.in         | 12 ++++++++++++
 linux/linux-tool-selftests.mk | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 linux/linux-tool-selftests.mk

Comments

Yann E. MORIN Dec. 14, 2015, 10:18 p.m. UTC | #1 
Cyril, All,

Sorry for the delay, but I'm now looking at this patch...

On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:
> This patch simply adds the ability to compile and install the kernel
> selftests into the target.
> 
> This is likely to be a rarely used debugging/performance feature for
> development and unlikely to be used in a production configuration.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  linux/Config.tools.in         | 12 ++++++++++++
>  linux/linux-tool-selftests.mk | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>  create mode 100644 linux/linux-tool-selftests.mk
> 
> diff --git a/linux/Config.tools.in b/linux/Config.tools.in
> index 24ef8cd..68655dc 100644
> --- a/linux/Config.tools.in
> +++ b/linux/Config.tools.in
> @@ -26,4 +26,16 @@ config BR2_LINUX_KERNEL_TOOL_PERF
>  
>  	  https://perf.wiki.kernel.org/
>  
> +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> +	bool "selftests"

Since the .mk has:
    SELFTESTS_DEPENDENCIES = bash

then you must either depend on bash or select it here. I think a select
is better, so:

    depends on BR2_USE_MMU  # bash
    select BR2_PACKAGE_BASH

> +	help
> +	  Build and install (to /usr/lib/selftests) kernel selftests.

Why do you install in there?

I'm not sure what the kernel selftests are, but two options:
  - if they are executables, they should go into /usr/sbin
  - if they are libraries, they should go in /usr/lib

> +	  Use of this option implies you know the process using and compiling
> +	  the kernel selftests. The Makefile to build and install these is very
> +	  noisy and may appear to cause your build to fail for strange reasons.
> +
> +	  This is very much a use at your risk option and may not work for
> +	  every setup or every architecture.
> +
>  endmenu
> diff --git a/linux/linux-tool-selftests.mk b/linux/linux-tool-selftests.mk
> new file mode 100644
> index 0000000..573ba0c
> --- /dev/null
> +++ b/linux/linux-tool-selftests.mk
> @@ -0,0 +1,40 @@
> +################################################################################
> +#
> +# selftests
> +#
> +################################################################################
> +
> +LINUX_TOOLS += selftests

linux-tool infrastructure. Yeah! :-)

> +ifeq ($(KERNEL_ARCH),x86_64)
> +SELFTESTS_ARCH=x86
> +else
> +SELFTESTS_ARCH=$(KERNEL_ARCH)
> +endif

Not related to your patch, but I wonder if we should not make that a
common conditional, so that all tools do not have to repeat it.

Since there are only two such tool (with selftests) that do it, it can
be put aside for now, and we can revisit it later.

> +SELFTESTS_DEPENDENCIES = bash
> +
> +SELFTESTS_INSTALL_STAGING = YES

Why install it in staging?

> +SELFTESTS_MAKE_FLAGS = \
> +	$(LINUX_MAKE_FLAGS) \
> +	ARCH=$(SELFTESTS_ARCH)
> +
> +# O must be redefined here to overwrite the one used by Buildroot for
> +# out of tree build. We build the selftests in $(@D)/tools/selftests and
> +# not just $(@D) so that it isn't built in the root directory of the kernel
> +# sources.
> +define SELFTESTS_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE1) -C $(@D) headers_install

Headers are target dependent, so I think you also need to pass
$(SELFTESTS_MAKE_FLAGS) in there, no?

Why are headers needed?

> +	$(TARGET_MAKE_ENV) $(MAKE1) $(SELFTESTS_MAKE_FLAGS) \
> +		-C $(@D)/tools/testing/selftests O=$(@D)/tools/testing/selftests

I prefer when the -C args are passed early (if possible first) because
it is then much obvious where the build is taking place:

    $(TARGET_MAKE_ENV) $(MAKE1) -C $(@D)/tools/testing/selftests \
        $(SELFTESTS_MAKE_FLAGS) \
        O=$(@D)/tools/testing/selftests

> +endef
> +
> +define SELFTESTS_INSTALL_STAGING_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE1) INSTALL_PATH=$(STAGING_DIR)/usr/lib/selftests $(SELFTESTS_MAKE_FLAGS) \
> +		-C $(@D)/tools/testing/selftests install

Line too long, split it to below ~72 chars; also, I prefer we keep the
generic flags early; probably you also have to repeat the O arg:

    $(TARGET_MAKE_ENV) $(MAKE1) -C $(@D)/tools/testing/selftests \
        $(SELFTESTS_MAKE_FLAGS) \
        O=$(@D)/tools/testing/selftests \
        INSTALL_PATH=$(STAGING_DIR)/usr/lib/selftests \
        install

> +endef
> +
> +define SELFTESTS_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE1) INSTALL_PATH=$(TARGET_DIR)/usr/lib/selftests $(SELFTESTS_MAKE_FLAGS) \
> +		-C $(@D)/tools/testing/selftests install

Ditto.

Regards,
Yann E. MORIN.

> +endef
> -- 
> 2.6.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Baruch Siach Dec. 15, 2015, 6:37 a.m. UTC | #2 
Hi Yann,

On Mon, Dec 14, 2015 at 11:18:46PM +0100, Yann E. MORIN wrote:
> On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:
> > +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> > +	bool "selftests"
> 
> Since the .mk has:
>     SELFTESTS_DEPENDENCIES = bash
> 
> then you must either depend on bash or select it here. I think a select
> is better, so:
> 
>     depends on BR2_USE_MMU  # bash
>     select BR2_PACKAGE_BASH

But BR2_PACKAGE_BASH depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS. Should 
BR2_LINUX_KERNEL_TOOL_SELFTESTS also depend on 
BR2_PACKAGE_BUSYBOX_SHOW_OTHERS?

baruch
Yann E. MORIN Dec. 15, 2015, 5:31 p.m. UTC | #3 
Baruch, Cyril, All,

On 2015-12-15 08:37 +0200, Baruch Siach spake thusly:
> On Mon, Dec 14, 2015 at 11:18:46PM +0100, Yann E. MORIN wrote:
> > On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:
> > > +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> > > +	bool "selftests"
> > 
> > Since the .mk has:
> >     SELFTESTS_DEPENDENCIES = bash
> > 
> > then you must either depend on bash or select it here. I think a select
> > is better, so:
> > 
> >     depends on BR2_USE_MMU  # bash
> >     select BR2_PACKAGE_BASH
> 
> But BR2_PACKAGE_BASH depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS.

Gah... I missed that... Thanks for spotting! :-)

> Should 
> BR2_LINUX_KERNEL_TOOL_SELFTESTS also depend on 
> BR2_PACKAGE_BUSYBOX_SHOW_OTHERS?

Yes. With a comment stating so, like:

    config BR2_LINUX_KERNEL_TOOL_SELFTESTS
        bool "selftests"
        depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS # bash
        depends on BR2_USE_MMU  # bash
        select BR2_PACKAGE_BASH

    comment "selftests needs BR2_PACKAGE_BUSYBOX_SHOW_OTHERS"
        depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS

(not so nice, but we already have it for openvmtools.)

Regards,
Yann E. MORIN.
Cyril Bur Dec. 17, 2015, 1:25 a.m. UTC | #4 
On Tue, 15 Dec 2015 18:31:17 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

Hello Everyone,

> Baruch, Cyril, All,
> 
> On 2015-12-15 08:37 +0200, Baruch Siach spake thusly:
> > On Mon, Dec 14, 2015 at 11:18:46PM +0100, Yann E. MORIN wrote:  
> > > On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:  
> > > > +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> > > > +	bool "selftests"  
> > > 
> > > Since the .mk has:
> > >     SELFTESTS_DEPENDENCIES = bash
> > > 
> > > then you must either depend on bash or select it here. I think a select
> > > is better, so:
> > > 
> > >     depends on BR2_USE_MMU  # bash
> > >     select BR2_PACKAGE_BASH  
> > 
> > But BR2_PACKAGE_BASH depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS.  
> 
> Gah... I missed that... Thanks for spotting! :-)
> 

Ah I would totally have missed that too, thanks Yann and Baruch!

> > Should 
> > BR2_LINUX_KERNEL_TOOL_SELFTESTS also depend on 
> > BR2_PACKAGE_BUSYBOX_SHOW_OTHERS?  
> 
> Yes. With a comment stating so, like:
> 
>     config BR2_LINUX_KERNEL_TOOL_SELFTESTS
>         bool "selftests"
>         depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS # bash
>         depends on BR2_USE_MMU  # bash
>         select BR2_PACKAGE_BASH
> 
>     comment "selftests needs BR2_PACKAGE_BUSYBOX_SHOW_OTHERS"
>         depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> 
> (not so nice, but we already have it for openvmtools.)
> 

Doesn't matter, I don't think we'll ever get the selftests looking 'nice'
anyway but that is an improvement.

On the note of dependencies - when I wrote the patch I was in two minds about
dependencies. Either this does the bare minimum to get the tests in and if the
user wants more tests to work/pass then they'll need to add things, or the
selftests depends on as much as possible to give as many tests passing as
possible. Based off this and discussion with a colleague, it might be better
that ticking the option to just select everything, if you're building the
selftests into your image it's because you want to be able to run them.
I haven't gone through and determined what the biggest set is but will do and
post v2.

> Regards,
> Yann E. MORIN.
>
Cyril Bur Dec. 17, 2015, 1:45 a.m. UTC | #5 
On Mon, 14 Dec 2015 23:18:46 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Cyril, All,
> 
> Sorry for the delay, but I'm now looking at this patch...
> 

Hi Yann,

No worries, thanks for getting around to it and thanks for review.

> On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:
> > This patch simply adds the ability to compile and install the kernel
> > selftests into the target.
> > 
> > This is likely to be a rarely used debugging/performance feature for
> > development and unlikely to be used in a production configuration.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  linux/Config.tools.in         | 12 ++++++++++++
> >  linux/linux-tool-selftests.mk | 40 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >  create mode 100644 linux/linux-tool-selftests.mk
> > 
> > diff --git a/linux/Config.tools.in b/linux/Config.tools.in
> > index 24ef8cd..68655dc 100644
> > --- a/linux/Config.tools.in
> > +++ b/linux/Config.tools.in
> > @@ -26,4 +26,16 @@ config BR2_LINUX_KERNEL_TOOL_PERF
> >  
> >  	  https://perf.wiki.kernel.org/
> >  
> > +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> > +	bool "selftests"  
> 
> Since the .mk has:
>     SELFTESTS_DEPENDENCIES = bash
> 
> then you must either depend on bash or select it here. I think a select
> is better, so:
> 
>     depends on BR2_USE_MMU  # bash
>     select BR2_PACKAGE_BASH
> 
> > +	help
> > +	  Build and install (to /usr/lib/selftests) kernel selftests.  
> 
> Why do you install in there?
> 

I was really quite a bit random, don't have any preferred place myself.

> I'm not sure what the kernel selftests are, but two options:
>   - if they are executables, they should go into /usr/sbin
>   - if they are libraries, they should go in /usr/lib

So I think I initially put them in /bin for testing but its odd because they
have quite a directory structure to them, so /bin/selftests/x/y/z felt odd.
Theres a runtests script that expects that too otherwise all the binaries
*could* be dumped into /usr/sbin.

They aren't libraries either though I agree but then its not like the
executables really do anything, in a way they are more like a 'library of
tests'

Happy to defer to you on that one.

> 
> > +	  Use of this option implies you know the process using and compiling
> > +	  the kernel selftests. The Makefile to build and install these is very
> > +	  noisy and may appear to cause your build to fail for strange reasons.
> > +
> > +	  This is very much a use at your risk option and may not work for
> > +	  every setup or every architecture.
> > +
> >  endmenu
> > diff --git a/linux/linux-tool-selftests.mk b/linux/linux-tool-selftests.mk
> > new file mode 100644
> > index 0000000..573ba0c
> > --- /dev/null
> > +++ b/linux/linux-tool-selftests.mk
> > @@ -0,0 +1,40 @@
> > +################################################################################
> > +#
> > +# selftests
> > +#
> > +################################################################################
> > +
> > +LINUX_TOOLS += selftests  
> 
> linux-tool infrastructure. Yeah! :-)
> 

I thought doing this was going to be a massive headache and then I found
linux-tool infrastructure and it's exactly what I need, long live buildroot! :)

> > +ifeq ($(KERNEL_ARCH),x86_64)
> > +SELFTESTS_ARCH=x86
> > +else
> > +SELFTESTS_ARCH=$(KERNEL_ARCH)
> > +endif  
> 
> Not related to your patch, but I wonder if we should not make that a
> common conditional, so that all tools do not have to repeat it.
> 
> Since there are only two such tool (with selftests) that do it, it can
> be put aside for now, and we can revisit it later.
> 

That does make sense, totally agreed. I'll ponder too :)

> > +SELFTESTS_DEPENDENCIES = bash
> > +
> > +SELFTESTS_INSTALL_STAGING = YES  
> 
> Why install it in staging?
> 

I actually hadn't initially but I did get to a point in my work where I had
to objdump a test binary. So, since I needed it, it stayed.

> > +SELFTESTS_MAKE_FLAGS = \
> > +	$(LINUX_MAKE_FLAGS) \
> > +	ARCH=$(SELFTESTS_ARCH)
> > +
> > +# O must be redefined here to overwrite the one used by Buildroot for
> > +# out of tree build. We build the selftests in $(@D)/tools/selftests and
> > +# not just $(@D) so that it isn't built in the root directory of the kernel
> > +# sources.
> > +define SELFTESTS_BUILD_CMDS
> > +	$(TARGET_MAKE_ENV) $(MAKE1) -C $(@D) headers_install  
> 
> Headers are target dependent, so I think you also need to pass
> $(SELFTESTS_MAKE_FLAGS) in there, no?
> 

Quite possible that you're right, this might explain somethings I was seeing.
Thanks.

> Why are headers needed?
> 

Unfortunately a few of the Makefiles do:
CFLAGS += -I../../../../usr/include/

There are other ways around the problem but this was the easiest.

> > +	$(TARGET_MAKE_ENV) $(MAKE1) $(SELFTESTS_MAKE_FLAGS) \
> > +		-C $(@D)/tools/testing/selftests O=$(@D)/tools/testing/selftests  
> 
> I prefer when the -C args are passed early (if possible first) because
> it is then much obvious where the build is taking place:
> 
>     $(TARGET_MAKE_ENV) $(MAKE1) -C $(@D)/tools/testing/selftests \
>         $(SELFTESTS_MAKE_FLAGS) \
>         O=$(@D)/tools/testing/selftests
> 

Sure, will fix.

> > +endef
> > +
> > +define SELFTESTS_INSTALL_STAGING_CMDS
> > +	$(TARGET_MAKE_ENV) $(MAKE1) INSTALL_PATH=$(STAGING_DIR)/usr/lib/selftests $(SELFTESTS_MAKE_FLAGS) \
> > +		-C $(@D)/tools/testing/selftests install  
> 
> Line too long, split it to below ~72 chars; also, I prefer we keep the
> generic flags early; probably you also have to repeat the O arg:
> 
>     $(TARGET_MAKE_ENV) $(MAKE1) -C $(@D)/tools/testing/selftests \
>         $(SELFTESTS_MAKE_FLAGS) \
>         O=$(@D)/tools/testing/selftests \
>         INSTALL_PATH=$(STAGING_DIR)/usr/lib/selftests \
>         install
> 
Thanks!


> > +endef
> > +
> > +define SELFTESTS_INSTALL_TARGET_CMDS
> > +	$(TARGET_MAKE_ENV) $(MAKE1) INSTALL_PATH=$(TARGET_DIR)/usr/lib/selftests $(SELFTESTS_MAKE_FLAGS) \
> > +		-C $(@D)/tools/testing/selftests install  
> 
> Ditto.
> 
> Regards,
> Yann E. MORIN.
> 

Thanks for the review,

Cyril.

> > +endef
> > -- 
> > 2.6.2
> > 
> > _______________________________________________
> > buildroot mailing list
> > buildroot@busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot  
>
Yann E. MORIN Dec. 17, 2015, 6:07 p.m. UTC | #6 
Cyril, All,

On 2015-12-17 12:45 +1100, Cyril Bur spake thusly:
> On Mon, 14 Dec 2015 23:18:46 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:
> > > This patch simply adds the ability to compile and install the kernel
> > > selftests into the target.
> > > 
> > > This is likely to be a rarely used debugging/performance feature for
> > > development and unlikely to be used in a production configuration.
> > > 
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > ---
> > >  linux/Config.tools.in         | 12 ++++++++++++
> > >  linux/linux-tool-selftests.mk | 40 ++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 52 insertions(+)
> > >  create mode 100644 linux/linux-tool-selftests.mk
> > > 
> > > diff --git a/linux/Config.tools.in b/linux/Config.tools.in
> > > index 24ef8cd..68655dc 100644
> > > --- a/linux/Config.tools.in
> > > +++ b/linux/Config.tools.in
> > > @@ -26,4 +26,16 @@ config BR2_LINUX_KERNEL_TOOL_PERF
> > >  
> > >  	  https://perf.wiki.kernel.org/
> > >  
> > > +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> > > +	bool "selftests"  
> > 
> > Since the .mk has:
> >     SELFTESTS_DEPENDENCIES = bash
> > 
> > then you must either depend on bash or select it here. I think a select
> > is better, so:
> > 
> >     depends on BR2_USE_MMU  # bash
> >     select BR2_PACKAGE_BASH
> > 
> > > +	help
> > > +	  Build and install (to /usr/lib/selftests) kernel selftests.  
> > 
> > Why do you install in there?
> > 
> 
> I was really quite a bit random, don't have any preferred place myself.
> 
> > I'm not sure what the kernel selftests are, but two options:
> >   - if they are executables, they should go into /usr/sbin
> >   - if they are libraries, they should go in /usr/lib
> 
> So I think I initially put them in /bin for testing but its odd because they
> have quite a directory structure to them, so /bin/selftests/x/y/z felt odd.
> Theres a runtests script that expects that too otherwise all the binaries
> *could* be dumped into /usr/sbin.
> 
> They aren't libraries either though I agree but then its not like the
> executables really do anything, in a way they are more like a 'library of
> tests'
> 
> Happy to defer to you on that one.

OK, your explanations is good for me. Still, the directory is not
completely to my taste, since it does not refer to the kernel. What
about either of:
    /usr/lib/linux-selftests
    /usr/lib/kselftests

My preference would be for the second, but if you have a better idea,
feel free to improvise! ;-)

That should probably be explained in the commit log.

[--SNIP--]
> > > +ifeq ($(KERNEL_ARCH),x86_64)
> > > +SELFTESTS_ARCH=x86
> > > +else
> > > +SELFTESTS_ARCH=$(KERNEL_ARCH)
> > > +endif  
> > 
> > Not related to your patch, but I wonder if we should not make that a
> > common conditional, so that all tools do not have to repeat it.
> > 
> > Since there are only two such tool (with selftests) that do it, it can
> > be put aside for now, and we can revisit it later.
> That does make sense, totally agreed. I'll ponder too :)

Really, just let that as-is for now. Two packages doing exactly the same
think is OK. We try to commonalise stuff when a lot of packages want to
do the same thing. 2 is not "a lot". ;-)

> > > +SELFTESTS_DEPENDENCIES = bash
> > > +
> > > +SELFTESTS_INSTALL_STAGING = YES  
> > 
> > Why install it in staging?
> 
> I actually hadn't initially but I did get to a point in my work where I had
> to objdump a test binary. So, since I needed it, it stayed.

But can't you objdump the binaries that are installed in target/ instead?

> > > +SELFTESTS_MAKE_FLAGS = \
> > > +	$(LINUX_MAKE_FLAGS) \
> > > +	ARCH=$(SELFTESTS_ARCH)
> > > +
> > > +# O must be redefined here to overwrite the one used by Buildroot for
> > > +# out of tree build. We build the selftests in $(@D)/tools/selftests and
> > > +# not just $(@D) so that it isn't built in the root directory of the kernel
> > > +# sources.
> > > +define SELFTESTS_BUILD_CMDS
> > > +	$(TARGET_MAKE_ENV) $(MAKE1) -C $(@D) headers_install  
[--SNIP--]
> > Why are headers needed?
> 
> Unfortunately a few of the Makefiles do:
> CFLAGS += -I../../../../usr/include/
> 
> There are other ways around the problem but this was the easiest.

OK, your solution is acceptable. But please add a comment just above,
tha texplains why this is needed, like so;

    # Lots of selftests use hard-coded CFLAGS to find the headers
    # like:  CFLAGS += -I../../../../usr/include/
    # So we insall them locally *inside* the kernel source tree
    make blabla headers_install

Basically, add a comment whenever what you do, or the reason why you do
it, is not obvious. And it also probably warrants some explanations in
the commit log as well.

Regards,
Yann E. MORIN.
Cyril Bur Dec. 21, 2015, 5:44 a.m. UTC | #7 
On Thu, 17 Dec 2015 19:07:35 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Cyril, All,
> 

Hi Yann,

Totally agreed will address everything you mention. I should have mentioned a
few in more detail, my bad for not going over it in more detail before posting,
you're correct that a few things in there are actually non obvious.

Just one more thing below,

> On 2015-12-17 12:45 +1100, Cyril Bur spake thusly:
> > On Mon, 14 Dec 2015 23:18:46 +0100
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:  
> > > On 2015-11-25 10:53 +1100, Cyril Bur spake thusly:  
> > > > This patch simply adds the ability to compile and install the kernel
> > > > selftests into the target.
> > > > 
> > > > This is likely to be a rarely used debugging/performance feature for
> > > > development and unlikely to be used in a production configuration.
> > > > 
> > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > > ---
> > > >  linux/Config.tools.in         | 12 ++++++++++++
> > > >  linux/linux-tool-selftests.mk | 40 ++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 52 insertions(+)
> > > >  create mode 100644 linux/linux-tool-selftests.mk
> > > > 
> > > > diff --git a/linux/Config.tools.in b/linux/Config.tools.in
> > > > index 24ef8cd..68655dc 100644
> > > > --- a/linux/Config.tools.in
> > > > +++ b/linux/Config.tools.in
> > > > @@ -26,4 +26,16 @@ config BR2_LINUX_KERNEL_TOOL_PERF
> > > >  
> > > >  	  https://perf.wiki.kernel.org/
> > > >  
> > > > +config BR2_LINUX_KERNEL_TOOL_SELFTESTS
> > > > +	bool "selftests"    
> > > 
> > > Since the .mk has:
> > >     SELFTESTS_DEPENDENCIES = bash
> > > 
> > > then you must either depend on bash or select it here. I think a select
> > > is better, so:
> > > 
> > >     depends on BR2_USE_MMU  # bash
> > >     select BR2_PACKAGE_BASH
> > >   
> > > > +	help
> > > > +	  Build and install (to /usr/lib/selftests) kernel selftests.    
> > > 
> > > Why do you install in there?
> > >   
> > 
> > I was really quite a bit random, don't have any preferred place myself.
> >   
> > > I'm not sure what the kernel selftests are, but two options:
> > >   - if they are executables, they should go into /usr/sbin
> > >   - if they are libraries, they should go in /usr/lib  
> > 
> > So I think I initially put them in /bin for testing but its odd because they
> > have quite a directory structure to them, so /bin/selftests/x/y/z felt odd.
> > Theres a runtests script that expects that too otherwise all the binaries
> > *could* be dumped into /usr/sbin.
> > 
> > They aren't libraries either though I agree but then its not like the
> > executables really do anything, in a way they are more like a 'library of
> > tests'
> > 
> > Happy to defer to you on that one.  
> 
> OK, your explanations is good for me. Still, the directory is not
> completely to my taste, since it does not refer to the kernel. What
> about either of:
>     /usr/lib/linux-selftests
>     /usr/lib/kselftests
> 
> My preference would be for the second, but if you have a better idea,
> feel free to improvise! ;-)
> 
> That should probably be explained in the commit log.
> 
> [--SNIP--]
> > > > +ifeq ($(KERNEL_ARCH),x86_64)
> > > > +SELFTESTS_ARCH=x86
> > > > +else
> > > > +SELFTESTS_ARCH=$(KERNEL_ARCH)
> > > > +endif    
> > > 
> > > Not related to your patch, but I wonder if we should not make that a
> > > common conditional, so that all tools do not have to repeat it.
> > > 
> > > Since there are only two such tool (with selftests) that do it, it can
> > > be put aside for now, and we can revisit it later.  
> > That does make sense, totally agreed. I'll ponder too :)  
> 
> Really, just let that as-is for now. Two packages doing exactly the same
> think is OK. We try to commonalise stuff when a lot of packages want to
> do the same thing. 2 is not "a lot". ;-)
> 
> > > > +SELFTESTS_DEPENDENCIES = bash
> > > > +
> > > > +SELFTESTS_INSTALL_STAGING = YES    
> > > 
> > > Why install it in staging?  
> > 
> > I actually hadn't initially but I did get to a point in my work where I had
> > to objdump a test binary. So, since I needed it, it stayed.  
> 
> But can't you objdump the binaries that are installed in target/ instead?
> 

I think I tried that initially but IIRC they get stripped when put into target/
which proved annoying when objdumping.

Thanks,

Cyril

> > > > +SELFTESTS_MAKE_FLAGS = \
> > > > +	$(LINUX_MAKE_FLAGS) \
> > > > +	ARCH=$(SELFTESTS_ARCH)
> > > > +
> > > > +# O must be redefined here to overwrite the one used by Buildroot for
> > > > +# out of tree build. We build the selftests in $(@D)/tools/selftests and
> > > > +# not just $(@D) so that it isn't built in the root directory of the kernel
> > > > +# sources.
> > > > +define SELFTESTS_BUILD_CMDS
> > > > +	$(TARGET_MAKE_ENV) $(MAKE1) -C $(@D) headers_install    
> [--SNIP--]
> > > Why are headers needed?  
> > 
> > Unfortunately a few of the Makefiles do:
> > CFLAGS += -I../../../../usr/include/
> > 
> > There are other ways around the problem but this was the easiest.  
> 
> OK, your solution is acceptable. But please add a comment just above,
> tha texplains why this is needed, like so;
> 
>     # Lots of selftests use hard-coded CFLAGS to find the headers
>     # like:  CFLAGS += -I../../../../usr/include/
>     # So we insall them locally *inside* the kernel source tree
>     make blabla headers_install
> 
> Basically, add a comment whenever what you do, or the reason why you do
> it, is not obvious. And it also probably warrants some explanations in
> the commit log as well.
> 
> Regards,
> Yann E. MORIN.
>
Yann E. MORIN Dec. 21, 2015, 9:25 a.m. UTC | #8 
Cyril, All,

On 2015-12-21 16:44 +1100, Cyril Bur spake thusly:
> On Thu, 17 Dec 2015 19:07:35 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> Totally agreed will address everything you mention. I should have mentioned a
> few in more detail, my bad for not going over it in more detail before posting,
> you're correct that a few things in there are actually non obvious.

No problem! Things just happen to be obvious when you actually do the
job, because everything is fresh in your memory. Patch reviews are there
to eventually highlight that fact. So, really, that's not a problem.

[--SNIP--]
> > > > > +SELFTESTS_INSTALL_STAGING = YES    
> > > > Why install it in staging?  
> > > I actually hadn't initially but I did get to a point in my work where I had
> > > to objdump a test binary. So, since I needed it, it stayed.  
> > But can't you objdump the binaries that are installed in target/ instead?
> 
> I think I tried that initially but IIRC they get stripped when put into target/
> which proved annoying when objdumping.

But then, the 'original' binaries are still present in the build
directory, where they are definitely not stripped (at least, not by
Builroot). Isn't that enough?

Staging is really for libraries and headers that are to be used by
another package. Kernel selftests are not headers, they are not
libraries, so nothing will include/link them. So, I still believe
there's no reason to put them in staging.

Regards,
Yann E. MORIN.
Cyril Bur Dec. 21, 2015, 10:18 p.m. UTC | #9 
On Mon, 21 Dec 2015 10:25:13 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Cyril, All,
> 
> On 2015-12-21 16:44 +1100, Cyril Bur spake thusly:
> > On Thu, 17 Dec 2015 19:07:35 +0100
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > Totally agreed will address everything you mention. I should have mentioned a
> > few in more detail, my bad for not going over it in more detail before posting,
> > you're correct that a few things in there are actually non obvious.  
> 
> No problem! Things just happen to be obvious when you actually do the
> job, because everything is fresh in your memory. Patch reviews are there
> to eventually highlight that fact. So, really, that's not a problem.
> 
> [--SNIP--]
> > > > > > +SELFTESTS_INSTALL_STAGING = YES      
> > > > > Why install it in staging?    
> > > > I actually hadn't initially but I did get to a point in my work where I had
> > > > to objdump a test binary. So, since I needed it, it stayed.    
> > > But can't you objdump the binaries that are installed in target/ instead?  
> > 
> > I think I tried that initially but IIRC they get stripped when put into target/
> > which proved annoying when objdumping.  
> 
> But then, the 'original' binaries are still present in the build
> directory, where they are definitely not stripped (at least, not by
> Builroot). Isn't that enough?
> 

Doh, you're right of course they'll be in the linux build dir, I'll change that
too - I'll be sure to comment that one, even I totally forgot that haha.

Thanks

> Staging is really for libraries and headers that are to be used by
> another package. Kernel selftests are not headers, they are not
> libraries, so nothing will include/link them. So, I still believe
> there's no reason to put them in staging.
> 
> Regards,
> Yann E. MORIN.
>
Yann E. MORIN Dec. 21, 2015, 10:25 p.m. UTC | #10 
Cyril, All,

On 2015-12-22 09:18 +1100, Cyril Bur spake thusly:
> On Mon, 21 Dec 2015 10:25:13 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
[--SNIP--]
> > But then, the 'original' binaries are still present in the build
> > directory, where they are definitely not stripped (at least, not by
> > Builroot). Isn't that enough?
> Doh, you're right of course they'll be in the linux build dir, I'll change that
> too - I'll be sure to comment that one, even I totally forgot that haha.

If you were planning on adding a comment like "if you need to objdump
the binaries, they are still present unstripped in the build directory"
then you should not: it's the case for all and every packages; there's
nothing special about this for the kselftests.

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/linux/Config.tools.in b/linux/Config.tools.in
index 24ef8cd..68655dc 100644
--- a/linux/Config.tools.in
+++ b/linux/Config.tools.in
@@ -26,4 +26,16 @@  config BR2_LINUX_KERNEL_TOOL_PERF
 
 	  https://perf.wiki.kernel.org/
 
+config BR2_LINUX_KERNEL_TOOL_SELFTESTS
+	bool "selftests"
+	help
+	  Build and install (to /usr/lib/selftests) kernel selftests.
+
+	  Use of this option implies you know the process using and compiling
+	  the kernel selftests. The Makefile to build and install these is very
+	  noisy and may appear to cause your build to fail for strange reasons.
+
+	  This is very much a use at your risk option and may not work for
+	  every setup or every architecture.
+
 endmenu
diff --git a/linux/linux-tool-selftests.mk b/linux/linux-tool-selftests.mk
new file mode 100644
index 0000000..573ba0c
--- /dev/null
+++ b/linux/linux-tool-selftests.mk
@@ -0,0 +1,40 @@ 
+################################################################################
+#
+# selftests
+#
+################################################################################
+
+LINUX_TOOLS += selftests
+
+ifeq ($(KERNEL_ARCH),x86_64)
+SELFTESTS_ARCH=x86
+else
+SELFTESTS_ARCH=$(KERNEL_ARCH)
+endif
+
+SELFTESTS_DEPENDENCIES = bash
+
+SELFTESTS_INSTALL_STAGING = YES
+SELFTESTS_MAKE_FLAGS = \
+	$(LINUX_MAKE_FLAGS) \
+	ARCH=$(SELFTESTS_ARCH)
+
+# O must be redefined here to overwrite the one used by Buildroot for
+# out of tree build. We build the selftests in $(@D)/tools/selftests and
+# not just $(@D) so that it isn't built in the root directory of the kernel
+# sources.
+define SELFTESTS_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE1) -C $(@D) headers_install
+	$(TARGET_MAKE_ENV) $(MAKE1) $(SELFTESTS_MAKE_FLAGS) \
+		-C $(@D)/tools/testing/selftests O=$(@D)/tools/testing/selftests
+endef
+
+define SELFTESTS_INSTALL_STAGING_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE1) INSTALL_PATH=$(STAGING_DIR)/usr/lib/selftests $(SELFTESTS_MAKE_FLAGS) \
+		-C $(@D)/tools/testing/selftests install
+endef
+
+define SELFTESTS_INSTALL_TARGET_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE1) INSTALL_PATH=$(TARGET_DIR)/usr/lib/selftests $(SELFTESTS_MAKE_FLAGS) \
+		-C $(@D)/tools/testing/selftests install
+endef