diff mbox series

[2/3] support/tests: allow top-level parallel builds

Message ID a04f80a248fb1452841a424e850a694419e3a6dd.1671873478.git.yann.morin.1998@free.fr
State Accepted
Headers show
Series support/testing: misc improvements (branch yem/runtime-test-ppd) | expand

Commit Message

Yann E. MORIN Dec. 24, 2022, 9:18 a.m. UTC
Running tests with top-level parallel builds can speed up running some
tests, expecially those that have a lot of packages like the systemd
init tests.

Trigger TLPB when the configuration enables per-package directories.

Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 support/testing/infra/basetest.py | 2 +-
 support/testing/infra/builder.py  | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Thomas Petazzoni Dec. 27, 2022, 8:45 p.m. UTC | #1
On Sat, 24 Dec 2022 10:18:12 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Running tests with top-level parallel builds can speed up running some
> tests, expecially those that have a lot of packages like the systemd
> init tests.
> 
> Trigger TLPB when the configuration enables per-package directories.
> 
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

I certainly support the idea of running some tests with TLPB. However,
this implementation makes a confusion between two different settings:

- The existing BRConfigTest.jlevel, which is set by run-tests -j, and
  used to define BR2_JLEVEL in the Buildroot configuration of each test
  case. This determines the number of parallel jobs used to build each
  independent package.

- Your new work, which uses "make -j" to do TLPB... but relies on the
  same above value, even though it's a completely different setting.

Is this expected?

Best regards,

Thomas
Yann E. MORIN Dec. 27, 2022, 8:53 p.m. UTC | #2
Thomas, All,

On 2022-12-27 21:45 +0100, Thomas Petazzoni via buildroot spake thusly:
> On Sat, 24 Dec 2022 10:18:12 +0100
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > Running tests with top-level parallel builds can speed up running some
> > tests, expecially those that have a lot of packages like the systemd
> > init tests.
> > 
> > Trigger TLPB when the configuration enables per-package directories.
> > 
> > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> I certainly support the idea of running some tests with TLPB. However,
> this implementation makes a confusion between two different settings:
> 
> - The existing BRConfigTest.jlevel, which is set by run-tests -j, and
>   used to define BR2_JLEVEL in the Buildroot configuration of each test
>   case. This determines the number of parallel jobs used to build each
>   independent package.
> 
> - Your new work, which uses "make -j" to do TLPB... but relies on the
>   same above value, even though it's a completely different setting.
> 
> Is this expected?

Yes, this is the intended behaviour, which I was explicitly seeking.

So, if one runs with PPD and TLPB (outside the run-time infra), one
would do something like:

    $ make -jN

This spawns a top-level make process that is parallel. In turn, in
rules, when we call $(MAKE), this is the magic that tells make that
it is recursive, but that it should use the jobserver from the calling
process.

So, in this case, the BR2_JLEVEL is unused by whatever uses the make
jobserver; only the number of jobs in the top-level jobserver is
meaningful, i.e. whatever we pass as -jN.

The exception, of course, is whatever uses BR2_JLEVEL but does not talk
to the jobserver, but this is mostly a few packages (scons, waf et al.).
Even ninja packages do talk to the top-level jobserver, now that we use
the ninja fork that knows to talk to it.

So, yes, using top-level -jN with the same value as BR2_JLEVEL is
exactly what I intended to do.

Unless I totally missed something...

Regards,
Yann E. MORIN.
Yann E. MORIN Dec. 27, 2022, 8:55 p.m. UTC | #3
Thomas, All,

On 2022-12-27 21:53 +0100, Yann E. MORIN spake thusly:
> On 2022-12-27 21:45 +0100, Thomas Petazzoni via buildroot spake thusly:
> > On Sat, 24 Dec 2022 10:18:12 +0100
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > > Running tests with top-level parallel builds can speed up running some
> > > tests, expecially those that have a lot of packages like the systemd
> > > init tests.
> > > 
> > > Trigger TLPB when the configuration enables per-package directories.
> > > 
> > > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > 
> > I certainly support the idea of running some tests with TLPB. However,
> > this implementation makes a confusion between two different settings:
> > 
> > - The existing BRConfigTest.jlevel, which is set by run-tests -j, and
> >   used to define BR2_JLEVEL in the Buildroot configuration of each test
> >   case. This determines the number of parallel jobs used to build each
> >   independent package.
> > 
> > - Your new work, which uses "make -j" to do TLPB... but relies on the
> >   same above value, even though it's a completely different setting.
> > 
> > Is this expected?
> 
> Yes, this is the intended behaviour, which I was explicitly seeking.
> 
> So, if one runs with PPD and TLPB (outside the run-time infra), one
> would do something like:
> 
>     $ make -jN
> 
> This spawns a top-level make process that is parallel. In turn, in
> rules, when we call $(MAKE), this is the magic that tells make that
> it is recursive, but that it should use the jobserver from the calling
> process.
> 
> So, in this case, the BR2_JLEVEL is unused by whatever uses the make
> jobserver; only the number of jobs in the top-level jobserver is
> meaningful, i.e. whatever we pass as -jN.

Slight correction: BR2_JLEVEL does have an actual effect, but only if its
value is lower than the one we pass as -jN.

So, in practice, we want to have BR2_JLEVEL equal to the top-level -jN.

Regards,
Yann E. MORIN.

> The exception, of course, is whatever uses BR2_JLEVEL but does not talk
> to the jobserver, but this is mostly a few packages (scons, waf et al.).
> Even ninja packages do talk to the top-level jobserver, now that we use
> the ninja fork that knows to talk to it.
> 
> So, yes, using top-level -jN with the same value as BR2_JLEVEL is
> exactly what I intended to do.
> 
> Unless I totally missed something...
> 
> Regards,
> Yann E. MORIN.
> 
> -- 
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
Thomas Petazzoni Feb. 7, 2023, 8:39 a.m. UTC | #4
On Sat, 24 Dec 2022 10:18:12 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Running tests with top-level parallel builds can speed up running some
> tests, expecially those that have a lot of packages like the systemd
> init tests.
> 
> Trigger TLPB when the configuration enables per-package directories.
> 
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  support/testing/infra/basetest.py | 2 +-
>  support/testing/infra/builder.py  | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)

Thanks to the additional explanation you provided, I understood better
the idea, and therefore applied the patch. I updated the commit log a
little bit to summarize the explanation you gave, and applied. Thanks!

Thomas
diff mbox series

Patch

diff --git a/support/testing/infra/basetest.py b/support/testing/infra/basetest.py
index 45bcd4c2e2..670c7213d6 100644
--- a/support/testing/infra/basetest.py
+++ b/support/testing/infra/basetest.py
@@ -52,7 +52,7 @@  class BRConfigTest(unittest.TestCase):
 
     def setUp(self):
         self.show_msg("Starting")
-        self.b = Builder(self.config, self.builddir, self.logtofile)
+        self.b = Builder(self.config, self.builddir, self.logtofile, self.jlevel)
 
         if not self.keepbuilds:
             self.b.delete()
diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
index 922a707220..a2abb9ed89 100644
--- a/support/testing/infra/builder.py
+++ b/support/testing/infra/builder.py
@@ -6,11 +6,12 @@  import infra
 
 
 class Builder(object):
-    def __init__(self, config, builddir, logtofile):
+    def __init__(self, config, builddir, logtofile, jlevel=None):
         self.config = '\n'.join([line.lstrip() for line in
                                  config.splitlines()]) + '\n'
         self.builddir = builddir
         self.logfile = infra.open_log_file(builddir, "build", logtofile)
+        self.jlevel = jlevel
 
     def is_defconfig_valid(self, configfile, defconfig):
         """Check if the .config is contains all lines present in the defconfig."""
@@ -87,6 +88,8 @@  class Builder(object):
         env.update(make_extra_env)
 
         cmd = ["make", "-C", self.builddir]
+        if "BR2_PER_PACKAGE_DIRECTORIES=y" in self.config.splitlines() and self.jlevel:
+            cmd.append(f"-j{self.jlevel}")
         cmd += make_extra_opts
 
         ret = subprocess.call(cmd, stdout=self.logfile, stderr=self.logfile,