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@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@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@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@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@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.