Hi Tom,
On Mon, 23 Oct 2023 at 06:37, Tom Rini <tr...@konsulko.com> wrote:
>
> 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 <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.
>
> 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.
OK, I see. Well let's run with it and see how we go. I have wanted to
have these sorts of tests for a while, so we can check combinations
that are not in common use.
BTW buildman supports -L which disabled LTO using the NO_LTO=1 option
Regards,
Simon
