| Message ID | 20170710204007.8402-2-arnout@mind.be |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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 >
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
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
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) +
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(+)