Hi Tom,
On Sat, 21 Oct 2023 at 11:34, Tom Rini <tr...@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 <tr...@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 <tr...@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 <s...@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 <s...@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])
> > > +
> > > +@pytest.mark.slow
> > > +@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.
Regards,
Simon
