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

Message ID 20170710204007.8402-2-arnout@mind.be
State Changes Requested
Headers show

Commit Message

Arnout Vandecappelle July 10, 2017, 8:40 p.m.
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(+)

Comments

Ricardo Martincoski July 15, 2017, 3:07 a.m. | #1
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
Arnout Vandecappelle July 18, 2017, 9:38 a.m. | #2
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
>
Ricardo Martincoski July 19, 2017, 2:02 a.m. | #3
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
Arnout Vandecappelle July 19, 2017, 11:49 a.m. | #4
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

Patch

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)
+