This is an automated email from the ASF dual-hosted git repository. tvb pushed a commit to branch tristan/implement-retry-failed in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit 33346bd87967d2cdc95ba5ba9f0d5b684dac3c12 Author: Tristan van Berkom <[email protected]> AuthorDate: Mon Jul 10 22:25:09 2023 +0900 _messenger.py: Use datestampts instead of PID to differentiate log files Log filenames have traditionally been differentiated by PID in the off chance that the same cache key is built twice, this however is unreliable, as I can see in some tests we consistently get the same PID twice in a row (depending on the system behavior of course). This commit fixes the cachedfail.py integration test to test correctly, while the functionality was indeed working correctly, the test was passing only due to the log file being generated twice in a row with the same filename. In order to test this properly, we do the following: * Add a new INFO message when discarding a failed build artifact * Add a new testing utility Result.get_discarded_elements() --- src/buildstream/_messenger.py | 7 +++++-- src/buildstream/_testing/runcli.py | 7 +++++++ src/buildstream/element.py | 7 +++++++ tests/integration/cachedfail.py | 24 ++++++++---------------- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/buildstream/_messenger.py b/src/buildstream/_messenger.py index ba2556d88..ef87e26f6 100644 --- a/src/buildstream/_messenger.py +++ b/src/buildstream/_messenger.py @@ -411,8 +411,11 @@ class Messenger: assert not hasattr(self._locals, "log_filename") or self._locals.log_filename is None # Create the fully qualified logfile in the log directory, - # appending the pid and .log extension at the end. - self._locals.log_filename = os.path.join(logdir, "{}.{}.log".format(filename, os.getpid())) + # appending the timestamp and .log extension at the end. + timestamp = datetime.datetime.now() + self._locals.log_filename = os.path.join( + logdir, "{}.{}.log".format(filename, timestamp.strftime("%Y%m%d-%H%M%S")) + ) # Ensure the directory exists first directory = os.path.dirname(self._locals.log_filename) diff --git a/src/buildstream/_testing/runcli.py b/src/buildstream/_testing/runcli.py index 02c3edd1f..8daaf08b6 100644 --- a/src/buildstream/_testing/runcli.py +++ b/src/buildstream/_testing/runcli.py @@ -235,6 +235,13 @@ class Result: return list(pulled) + def get_discarded_elements(self): + discarded = re.findall(r"\[\s*(?:main|pull):(\S+)\s*\]\s*INFO\s*Discarded failed build", self.stderr) + if discarded is None: + return [] + + return list(discarded) + class Cli: def __init__(self, directory, verbose=True, default_options=None): diff --git a/src/buildstream/element.py b/src/buildstream/element.py index 1a97308e6..2970073de 100644 --- a/src/buildstream/element.py +++ b/src/buildstream/element.py @@ -1893,6 +1893,13 @@ class Element(Plugin): # equal to the resolved strict key. # if artifact.strong_key != self.__strict_cache_key: + + self.info( + "Discarded failed build", + detail="Discarded '{}'\n".format(artifact.strong_key) + + "in non strict mode because intermediate dependencies may have changed.", + ) + artifact = Artifact( self, context, diff --git a/tests/integration/cachedfail.py b/tests/integration/cachedfail.py index 9a0f2bb02..fb03a8a07 100644 --- a/tests/integration/cachedfail.py +++ b/tests/integration/cachedfail.py @@ -16,7 +16,6 @@ # pylint: disable=redefined-outer-name import os -import glob from contextlib import ExitStack import pytest @@ -263,8 +262,8 @@ def test_host_tools_errors_are_not_cached(cli, datafiles, tmp_path): @pytest.mark.datafiles(DATA_DIR) @pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox") @pytest.mark.parametrize("use_share", (False, True), ids=["local-cache", "pull-failed-artifact"]) [email protected]("retry", (True, False), ids=["retry", "no-retry"]) -def test_nonstrict_retry_failed(cli, tmpdir, datafiles, use_share, retry): [email protected]("success", (True, False), ids=["success", "no-success"]) +def test_nonstrict_retry_failed(cli, tmpdir, datafiles, use_share, success): project = str(datafiles) intermediate_path = os.path.join(project, "elements", "intermediate.bst") dep_path = os.path.join(project, "elements", "dep.bst") @@ -341,29 +340,22 @@ def test_nonstrict_retry_failed(cli, tmpdir, datafiles, use_share, retry): # Assert that the failed build has been removed assert cli.get_element_state(project, "target.bst") == "buildable" - # Regenerate the dependency so that the target would succeed to build, if the - # test is configured to test a retry - if retry: + # Regenerate the dependency so that the target would succeed to build + if success: dep = generate_dep("foo", "intermediate.bst") _yaml.roundtrip_dump(dep, dep_path) # Even though we are in non-strict mode, the failed build should be retried result = cli.run(project=project, args=["build", "target.bst"]) - # If we did not modify the cache key, we want to assert that we did not - # in fact attempt to rebuild the failed artifact. + # Because the intermediate.bst is changed, the failed target.bst will be + # retried unconditionally, assert that it gets discarded. # - # Since the UX is very similar, we'll distinguish this by counting the number of - # build logs which were produced. - # - logdir = os.path.join(cli.directory, "logs", "test", "target") - build_logs = glob.glob("{}/*-build.*.log".format(logdir)) - if retry: + assert "target.bst" in result.get_discarded_elements() + if success: result.assert_success() - assert len(build_logs) == 2 else: result.assert_main_error(ErrorDomain.STREAM, None) - assert len(build_logs) == 1 if use_share: # Assert that we did indeed go through the motions of downloading the failed
