Return 1 if any remote-build failed

[Hook25]

• Have you followed the guidelines for contributing?
• Have you signed the CLA?
• Have you successfully run make lint?

Description

When using snapcraft remote-build in CI, it would be useful to get a failure when any of the remote build fails. This is not the behaviour that we currently have (namely, even if some build fails, the return code is 0 and the workflow is marked as ok. See: core16-failure). Currently we are resorting to counting artifacts, but this process is fragile and error prone (For instance, we have discovered that the snap counting mechanism was introduced here but not here, leading to some failures not being detected)

This PR introduces a new flag to remote-build: --exit-fail-on-any-failure. When the flag is given, the remote build command will return 1 if any build fails, if the flag is not given it will just produce a debug message signaling the failure.

Bug

Fixes: #4142

Testing

I can't run the following due to this exception:

• Have you successfully run pytest tests/unit?

(venv)  snapcraft (return_1_remote_builds_failed) >  pytest tests/unit
ImportError while loading conftest '/home/h25/prj/canonical/snapcraft/tests/conftest.py'.
tests/conftest.py:21: in <module>
    import xdg.BaseDirectory
E   ModuleNotFoundError: No module named 'xdg.BaseDirectory'

---

[mr-cal]

Looking good, thanks! I think this still needs:

1. Unit test updates (some are now failing)
2. A new unit test that confirms a SnapcraftError is raised when one of the builds fail
3. A new unit test that confirms a Snapcraft ERror is not raised when the builds succeed
4. The same changes made in snapcraft_legacy's remote build to return true/false

[Hook25]

I'm afraid I will need some guidance to patch the legacy runner. If it is a straightforward thing I can do it. I don't really know what the legacy launcher is used for/what modifying it leads to, so please give me some input for that as well.

If it works as I think it does, for our specific need (and the issue I have claimed to fix in this PR) this patch should be enough and it should cover most CI/CD needs as I suppose that most users use the latest version of snapcraft to trigger the remote build.

[codecov-commenter]

Codecov Report

Merging #4400 (6ef729f) into main (0e9ea29) will decrease coverage by 0.02%.
The diff coverage is 96.15%.

❗ Current head 6ef729f differs from pull request most recent head 75de3ae. Consider uploading reports for the commit 75de3ae to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #4400      +/-   ##
==========================================
- Coverage   89.18%   89.17%   -0.02%     
==========================================
  Files         321      321              
  Lines       21658    21616      -42     
==========================================
- Hits        19315    19275      -40     
+ Misses       2343     2341       -2     

Files	Coverage Δ
snapcraft/remote/launchpad.py	87.54% <100.00%> (-1.48%)	⬇️
snapcraft/remote/remote_builder.py	100.00% <100.00%> (ø)
snapcraft/commands/remote.py	98.20% <94.73%> (-0.60%)	⬇️

... and 7 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[mr-cal]

I'm afraid I will need some guidance to patch the legacy runner. If it is a straightforward thing I can do it. I don't really know what the legacy launcher is used for/what modifying it leads to, so please give me some input for that as well.

If it works as I think it does, for our specific need (and the issue I have claimed to fix in this PR) this patch should be enough and it should cover most CI/CD needs as I suppose that most users use the latest version of snapcraft to trigger the remote build.

Sure, if you don't mind I'd appreciate it. It should be straightforward as the code is extremely similar and slightly simpler.

There are two places where monitor_build is called (here and here), which should look identical (here).

I think you can re-use your same design of monitor_build() returning a bool then raising an error.

The main reason I want to have both the new and fallback remote-builders to have a consistent behavior is because the fallback remote-builder is still in use. In fact, it's the only builder available on the latest/stable channel. The new remote-build is currently only available on latest/edge and is planned to be released to stable with snapcraft 8.

[mr-cal]

Thanks, @Hook25!

I think you will need to touch this file:

--- a/snapcraft/legacy_cli.py
+++ b/snapcraft/legacy_cli.py
@@ -29,7 +29,7 @@ _LIB_NAMES = ("craft_parts", "craft_providers", "craft_store", "snapcraft.remote
 _ORIGINAL_LIB_NAME_LOG_LEVEL: Dict[str, int] = {}


-def run_legacy(err: Optional[Exception] = None):
+def run_legacy(err: Optional[Exception] = None) -> bool:
     """Run legacy implementation."""
     # Reset the libraries to their original log level
     for lib_name in _LIB_NAMES:
@@ -43,4 +43,4 @@ def run_legacy(err: Optional[Exception] = None):
         emit.trace(f"run legacy implementation: {err!s}")
     emit.ended_ok()

-    legacy.legacy_run()
+    return legacy.legacy_run()

and if you don't mind adding two more unit tests for the legacy runner:

@pytest.mark.parametrize(
    "create_snapcraft_yaml", LEGACY_BASES | {"core22"}, indirect=True
)
@pytest.mark.usefixtures("create_snapcraft_yaml", "mock_argv", "mock_confirm")
def test_remote_build_legacy_ok(mock_run_legacy):
    """Successful legacy remote builds should have a return code of 0."""
    mock_run_legacy.return_value = True

    return_value = cli.run()

    assert return_value == 0


@pytest.mark.parametrize(
    "create_snapcraft_yaml", LEGACY_BASES | {"core22"}, indirect=True
)
@pytest.mark.usefixtures("create_snapcraft_yaml", "mock_argv", "mock_confirm")
def test_remote_build_legacy_failure(mock_run_legacy):
    """Unsuccessful legacy remote builds should have a return code of 1."""
    mock_run_legacy.return_value = False

    return_value = cli.run()

    assert return_value == 1

Finally, we will need some unit tests in tests/legacy/unit/remote/test_launchpad.py to ensure monitor_build() is working as expected, but those are slower to write with the existing unit.TestCase design. I can help you with those if you'd like.

[sergiusens]

thanks for this, an alternative implementation made its way in so this is now superseded