[Buildroot] [PATCH 1/2] support/testing: unbreak run-tests -l

[Buildroot] [PATCH 1/2] support/testing: unbreak run-tests -l

[Arnout Vandecappelle]

In commit b78b50465c20c1733753a8dd47945cf80c9155f8, the initialisation
of BRTest.builddir was moved to the __init__ function. However, it is
set based on BRTest.outputdir and that is only set when the -o argument
is given to run-tests. When called as "run-tests -l", there is no -o
argument so BRTest.outputdir remains unset.

To fix, keep BRTest.builddir at None when BRTest.outputdir is None.

While we're at it, drop the direct access to the class member. If a
subclass wishes to set outputdir to something else before calling
BRTest.__init__, they are free to do so.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 support/testing/infra/basetest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py
index 2a5c9ec939..1a082bb441 100644
--- a/support/testing/infra/basetest.py
+++ b/support/testing/infra/basetest.py
@@ -39,7 +39,7 @@ class BRTest(unittest.TestCase):
     def __init__(self, names):
         super(BRTest, self).__init__(names)
         self.testname = self.__class__.__name__
-        self.builddir = os.path.join(self.__class__.outputdir, self.testname)
+        self.builddir = self.outputdir and os.path.join(self.outputdir, self.testname)
         self.emulator = None
 
     def show_msg(self, msg):
-- 
2.13.2

[Buildroot] [PATCH 2/2] support/testing: add test of BR2_CCACHE with an external toolchain

[Arnout Vandecappelle]

We reuse TestExternalToolchainBuildrootuClibc and add ccache to its
configuration.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 support/testing/tests/toolchain/test_external.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/support/testing/tests/toolchain/test_external.py b/support/testing/tests/toolchain/test_external.py
index afb4bb0b50..57e2f11451 100644
--- a/support/testing/tests/toolchain/test_external.py
+++ b/support/testing/tests/toolchain/test_external.py
@@ -229,3 +229,15 @@ BR2_TOOLCHAIN_EXTERNAL_CXX=y
                            kernel="builtin",
                            options=["-initrd", img])
         self.emulator.login()
+
+class TestExternalToolchainCCache(TestExternalToolchainBuildrootuClibc):
+    extraconfig = \
+"""
+BR2_CCACHE=y
+BR2_CCACHE_DIR="{builddir}/ccache-dir"
+"""
+
+    def __init__(self, names):
+        super(TestExternalToolchainBuildrootuClibc, self).__init__(names)
+        self.config = self.config + self.extraconfig.format(builddir = self.builddir)
+
-- 
2.13.2

[Buildroot] [PATCH 2/2] support/testing: add test of BR2_CCACHE with an external toolchain

[Ricardo Martincoski]

Hello,

On Mon, Jul 10, 2017 at 05:40 PM, Arnout Vandecappelle (Essensium/Mind) wrote:

> We reuse TestExternalToolchainBuildrootuClibc and add ccache to its
> configuration.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
[snip]
> +        self.config = self.config + self.extraconfig.format(builddir = self.builddir)

3 nits:
- why not += ?
- usually there are no spaces around parameter equals
- line is too long (not that much)
from these I suggest to change to:
        self.config += self.extraconfig.format(builddir=self.builddir)

> +
> -- 

nit: This empty line at end of file is not needed.

with the 4 nits fixed
Reviewed-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

[the test passes, host-ccache is built either with or without ccache
 pre-installed in the host machine, the ccache dir is created inside the
 builddir and has valid data inside:
 $ cd ${builddir} && host/bin/ccache -s
 ...
 cache size                          13.1 MB
 max cache size                       5.0 GB]
Tested-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Regards,
Ricardo

[Buildroot] [PATCH 2/2] support/testing: add test of BR2_CCACHE with an external toolchain

[Arnout Vandecappelle]

On 15-07-17 05:07, Ricardo Martincoski wrote:
> Hello,
> 
> On Mon, Jul 10, 2017 at 05:40 PM, Arnout Vandecappelle (Essensium/Mind) wrote:
> 
>> We reuse TestExternalToolchainBuildrootuClibc and add ccache to its
>> configuration.
>>
>> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> [snip]
>> +        self.config = self.config + self.extraconfig.format(builddir = self.builddir)
> 
> 3 nits:
> - why not += ?

 Because config is initialized as a class member, and += would update the class
member instead of the instance member like we want. As it happens, this test is
executed last so it doesn't matter, but if any test comes after it, it would get
the extraconfig as well...

 Cfr. the following test:

In [49]: class Foo:
    ...:     foo = [0]
    ...:

In [50]: class Foo1(Foo):
    ...:     def __init__(self):
    ...:         self.foo += [1]
    ...:

In [51]: Foo1().foo
Out[51]: [0, 1]

In [52]: Foo1().foo
Out[52]: [0, 1, 1]

In [53]: class Foo2(Foo):
    ...:     def __init__(self):
    ...:         self.foo = self.foo + [2]
    ...:

In [54]: Foo2().foo
Out[54]: [0, 1, 1, 2]

In [55]: Foo2().foo
Out[55]: [0, 1, 1, 2]


> - usually there are no spaces around parameter equals
> - line is too long (not that much)

 I'll take care of these, and check it with flake8 while I'm at it.

> from these I suggest to change to:
>         self.config += self.extraconfig.format(builddir=self.builddir)
> 
>> +
>> -- 
> 
> nit: This empty line at end of file is not needed.

 Hm, I usually don't make that kind of mistake :-)

> 
> with the 4 nits fixed
> Reviewed-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

 Can you confirm that this still stands if I don't do the +=, as long as the
line stays below 79 characters?


 Regards,
 Arnout

> 
> [the test passes, host-ccache is built either with or without ccache
>  pre-installed in the host machine, the ccache dir is created inside the
>  builddir and has valid data inside:
>  $ cd ${builddir} && host/bin/ccache -s
>  ...
>  cache size                          13.1 MB
>  max cache size                       5.0 GB]
> Tested-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> 
> Regards,
> Ricardo
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 2/2] support/testing: add test of BR2_CCACHE with an external toolchain

[Ricardo Martincoski]

Hello,

On Tue, Jul 18, 2017 at 06:38 AM, Arnout Vandecappelle wrote:

> On 15-07-17 05:07, Ricardo Martincoski wrote:
>> [snip]
>>> +        self.config = self.config + self.extraconfig.format(builddir = self.builddir)
>> 
>> 3 nits:
>> - why not += ?
> 
>  Because config is initialized as a class member, and += would update the class
> member instead of the instance member like we want. As it happens, this test is
> executed last so it doesn't matter, but if any test comes after it, it would get

From command line there is no guarantee on execution order when using -t.

In my tests without -t I observe the testcases being executed in a case
insensitive reverse alphabetical order, no matter if I move one test to another
place in the file.

$ support/testing/run-tests -d d -o o -k tests.toolchain.test_external
22:32:39 TestExternalToolchainSourceryArmv7       Starting
22:32:43 TestExternalToolchainSourceryArmv7       Cleaning up
.22:32:43 TestExternalToolchainSourceryArmv5       Starting
22:32:49 TestExternalToolchainSourceryArmv5       Cleaning up
.22:32:49 TestExternalToolchainSourceryArmv4       Starting
22:32:54 TestExternalToolchainSourceryArmv4       Cleaning up
.22:32:54 TestExternalToolchainLinaroArm           Starting
22:32:58 TestExternalToolchainLinaroArm           Cleaning up
.22:32:58 TestExternalToolchainCtngMusl            Starting
22:33:01 TestExternalToolchainCtngMusl            Cleaning up
.22:33:01 TestExternalToolchainCCache              Starting
22:33:04 TestExternalToolchainCCache              Cleaning up
.22:33:04 TestExternalToolchainBuildrootuClibc     Starting
22:33:08 TestExternalToolchainBuildrootuClibc     Cleaning up
.22:33:08 TestExternalToolchainBuildrootMusl       Starting
22:33:11 TestExternalToolchainBuildrootMusl       Cleaning up
.

But I guess you are talking about gitlab. I never used it.

> the extraconfig as well...
> 
>  Cfr. the following test:
> 
> In [49]: class Foo:
>     ...:     foo = [0]
>     ...:
> 
> In [50]: class Foo1(Foo):
>     ...:     def __init__(self):
>     ...:         self.foo += [1]
>     ...:
> 
> In [51]: Foo1().foo
> Out[51]: [0, 1]
> 
> In [52]: Foo1().foo
> Out[52]: [0, 1, 1]
> 
> In [53]: class Foo2(Foo):
>     ...:     def __init__(self):
>     ...:         self.foo = self.foo + [2]
>     ...:
> 
> In [54]: Foo2().foo
> Out[54]: [0, 1, 1, 2]
> 
> In [55]: Foo2().foo
> Out[55]: [0, 1, 1, 2]

The same does not apply to strings, only to mutable objects.

But if i.e. for some reason we change the type of config from string to an array
of lines the += would need to be changed, while your code would still work.

[snip]
>> with the 4 nits fixed
>> Reviewed-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> 
>  Can you confirm that this still stands if I don't do the +=, as long as the
> line stays below 79 characters?

Sure. It still stands without += .

Regards,
Ricardo

[Buildroot] [PATCH 2/2] support/testing: add test of BR2_CCACHE with an external toolchain

[Arnout Vandecappelle]

On 19-07-17 04:02, Ricardo Martincoski wrote:
> Hello,
> 
> On Tue, Jul 18, 2017 at 06:38 AM, Arnout Vandecappelle wrote:
> 
>> On 15-07-17 05:07, Ricardo Martincoski wrote:
>>> [snip]
>>>> +        self.config = self.config + self.extraconfig.format(builddir = self.builddir)
>>> 3 nits:
>>> - why not += ?
>>  Because config is initialized as a class member, and += would update the class
>> member instead of the instance member like we want.
[snip]
> The same does not apply to strings, only to mutable objects.

 Right, I didn't think of that! I just learned to be wary of using += when the
left-hand side is a class member (or function argument with a default value), I
didn't consider mutability.


> But if i.e. for some reason we change the type of config from string to an array
> of lines the += would need to be changed, while your code would still work.

 However, with Yann's change in '[PATCHv3] support/tests: allow properly
indented config fragment', self.config will already have been updated to be an
instance member instead of a class member. Therefore, += should be fine.

 I'll respin with all your nits applied.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/2] support/testing: unbreak run-tests -l

[Yann E. MORIN]

Arnout, All,

On 2017-07-10 22:40 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
> In commit b78b50465c20c1733753a8dd47945cf80c9155f8, the initialisation
> of BRTest.builddir was moved to the __init__ function. However, it is
> set based on BRTest.outputdir and that is only set when the -o argument
> is given to run-tests. When called as "run-tests -l", there is no -o
> argument so BRTest.outputdir remains unset.
> 
> To fix, keep BRTest.builddir at None when BRTest.outputdir is None.
> 
> While we're at it, drop the direct access to the class member. If a
> subclass wishes to set outputdir to something else before calling
> BRTest.__init__, they are free to do so.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

Reported-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Thanks for the quick fixing! :-)

Regards,
Yann E. MORIN.

> ---
>  support/testing/infra/basetest.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py
> index 2a5c9ec939..1a082bb441 100644
> --- a/support/testing/infra/basetest.py
> +++ b/support/testing/infra/basetest.py
> @@ -39,7 +39,7 @@ class BRTest(unittest.TestCase):
>      def __init__(self, names):
>          super(BRTest, self).__init__(names)
>          self.testname = self.__class__.__name__
> -        self.builddir = os.path.join(self.__class__.outputdir, self.testname)
> +        self.builddir = self.outputdir and os.path.join(self.outputdir, self.testname)
>          self.emulator = None
>  
>      def show_msg(self, msg):
> -- 
> 2.13.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2] support/testing: unbreak run-tests -l

[Thomas Petazzoni]

Hello,

On Mon, 10 Jul 2017 22:40:06 +0200, Arnout Vandecappelle
(Essensium/Mind) wrote:
> In commit b78b50465c20c1733753a8dd47945cf80c9155f8, the initialisation
> of BRTest.builddir was moved to the __init__ function. However, it is
> set based on BRTest.outputdir and that is only set when the -o argument
> is given to run-tests. When called as "run-tests -l", there is no -o
> argument so BRTest.outputdir remains unset.
> 
> To fix, keep BRTest.builddir at None when BRTest.outputdir is None.
> 
> While we're at it, drop the direct access to the class member. If a
> subclass wishes to set outputdir to something else before calling
> BRTest.__init__, they are free to do so.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
>  support/testing/infra/basetest.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com