[v4.1 2/2] CI, pytest: Add a test for sandbox without LTO

[Tom Rini]

On Mon, Oct 23, 2023 at 12:05:34AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 21 Oct 2023 at 11:34, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sat, Oct 21, 2023 at 08:43:00AM -0700, Simon Glass wrote:
> > > On Fri, 20 Oct 2023 at 14:53, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > The primary motivation for having a sandbox without LTO build in CI is
> > > > to ensure that we don't have that option break. We now have the ability
> > > > to run tests of specific options being enabled/disabled, so drop the
> > > > parts of CI that build and test that configuration specifically and add
> > > > a build test instead. We still test that "NO_LTO=1" rather than editing
> > > > the config file works via the ftrace tests.
> > > >
> > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > ---
> > > > This creates a small bisectability gap in CI itself, but I think is more
> > > > reasonable than reworking the introduction of
> > > > test/py/tests/test_sandbox_opts.py
> > > >
> > > > Cc: Simon Glass <sjg at chromium.org>
> > > > ---
> > > >  .azure-pipelines.yml               |  3 ---
> > > >  .gitlab-ci.yml                     | 12 ------------
> > > >  test/py/tests/test_sandbox_opts.py | 10 ++++++++++
> > > >  3 files changed, 10 insertions(+), 15 deletions(-)
> > >
> > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > >
> > > Q below
> > >
> > > >
> > > > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > > > index 6f91553e8613..65533b36dde4 100644
> > > > --- a/.azure-pipelines.yml
> > > > +++ b/.azure-pipelines.yml
> > > > @@ -287,9 +287,6 @@ stages:
> > > >          sandbox64_clang:
> > > >            TEST_PY_BD: "sandbox64"
> > > >            OVERRIDE: "-O clang-16"
> > > > -        sandbox_nolto:
> > > > -          TEST_PY_BD: "sandbox"
> > > > -          BUILD_ENV: "NO_LTO=1"
> > > >          sandbox_spl:
> > > >            TEST_PY_BD: "sandbox_spl"
> > > >            TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl"
> > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > > > index 6decdfdee334..97c964fb8079 100644
> > > > --- a/.gitlab-ci.yml
> > > > +++ b/.gitlab-ci.yml
> > > > @@ -258,12 +258,6 @@ sandbox with clang test.py:
> > > >      OVERRIDE: "-O clang-16"
> > > >    <<: *buildman_and_testpy_dfn
> > > >
> > > > -sandbox without LTO test.py:
> > > > -  variables:
> > > > -    TEST_PY_BD: "sandbox"
> > > > -    BUILD_ENV: "NO_LTO=1"
> > > > -  <<: *buildman_and_testpy_dfn
> > > > -
> > > >  sandbox64 test.py:
> > > >    variables:
> > > >      TEST_PY_BD: "sandbox64"
> > > > @@ -275,12 +269,6 @@ sandbox64 with clang test.py:
> > > >      OVERRIDE: "-O clang-16"
> > > >    <<: *buildman_and_testpy_dfn
> > > >
> > > > -sandbox64 without LTO test.py:
> > > > -  variables:
> > > > -    TEST_PY_BD: "sandbox64"
> > > > -    BUILD_ENV: "NO_LTO=1"
> > > > -  <<: *buildman_and_testpy_dfn
> > > > -
> > > >  sandbox_spl test.py:
> > > >    variables:
> > > >      TEST_PY_BD: "sandbox_spl"
> > > > diff --git a/test/py/tests/test_sandbox_opts.py b/test/py/tests/test_sandbox_opts.py
> > > > index 91790b3374b4..422b43cb3bc1 100644
> > > > --- a/test/py/tests/test_sandbox_opts.py
> > > > +++ b/test/py/tests/test_sandbox_opts.py
> > > > @@ -18,3 +18,13 @@ def test_sandbox_cmdline(u_boot_console):
> > > >      out = util.run_and_log(
> > > >          cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > >                 '-a', '~CMDLINE', '-o', TMPDIR])
> > > > +
> > > > + at pytest.mark.slow
> > > > + at pytest.mark.boardspec('sandbox')
> > > > +def test_sandbox_lto(u_boot_console):
> > > > +    """Test building sandbox without CONFIG_LTO"""
> > > > +    cons = u_boot_console
> > > > +
> > > > +    out = util.run_and_log(
> > > > +        cons, ['./tools/buildman/buildman', '-m', '--board', 'sandbox',
> > > > +               '-a', '~LTO', '-o', TMPDIR])
> > >
> > > Don't you need sandbox64 here?
> >
> > No, I don't think it's providing further value to just build sandbox64
> > without LTO. I'm also not 100% sure this patch is really needed in that we're
> > trying to test for "did we disable LTO via make arguments" and in turn
> > only the ftrace test really catches that, as it will fail if we do have
> > LTO enabled, yes?
> 
> Really the test is to make sure that sandbox builds without LTO. With
> the build time being. For development, LTO serves no useful purpose
> and just triples the incremental build time.
> 
> I suggest we keep sandbox64 just to prevent a regression on non-LTO,
> but I suspect it is unlikely to happen in practice.

I don't follow you here. This adds the build time test for disabling
LTO, as that's at least an option for sandbox (other platforms require
it because otherwise we won't fit on the hardware). I'm mainly not sure
if this test is right in that what we really want to know is "does
NO_LTO=1" pass things as expected, and the ftrace test covers that.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231023/aaeb0aca/attachment.sig>