Merge branch 'abderrahim/cached-failure' into 'master'

Rework handling of cached failures

Closes #967

See merge request BuildStream/buildstream!1970

6 files changed, 45 insertions, 54 deletions

diff --git a/src/buildstream/_artifact.py b/src/buildstream/_artifact.py
index cf82a1636..ac33f041c 100644
--- a/src/buildstream/_artifact.py
+++ b/src/buildstream/_artifact.py
@@ -193,7 +193,6 @@ class Artifact:
         log_filename = context.messenger.get_log_filename()
         if log_filename:
             digest = self._cas.add_object(path=log_filename)
-            element._build_log_path = self._cas.objpath(digest)
             log = artifact.logs.add()
             log.name = os.path.basename(log_filename)
             log.digest.CopyFrom(digest)
diff --git a/src/buildstream/_exceptions.py b/src/buildstream/_exceptions.py
index 30fc26e71..755da3a9d 100644
--- a/src/buildstream/_exceptions.py
+++ b/src/buildstream/_exceptions.py
@@ -267,6 +267,14 @@ class AppError(BstError):
         super().__init__(message, detail=detail, domain=ErrorDomain.APP, reason=reason)
 
 
+# CachedFailure
+#
+# Raised from a child process within a job to indicate that the failure was cached
+#
+class CachedFailure(BstError):
+    pass
+
+
 # SkipJob
 #
 # Raised from a child process within a job when the job should be
diff --git a/src/buildstream/_scheduler/jobs/elementjob.py b/src/buildstream/_scheduler/jobs/elementjob.py
index 6e035be9c..683129506 100644
--- a/src/buildstream/_scheduler/jobs/elementjob.py
+++ b/src/buildstream/_scheduler/jobs/elementjob.py
@@ -16,9 +16,6 @@
 #  Author:
 #        Tristan Daniël Maat <tristan.maat@codethink.co.uk>
 #
-from ruamel import yaml
-
-from ..._message import MessageType
 
 from .job import Job, ChildJob
 
@@ -92,13 +89,6 @@ class ChildElementJob(ChildJob):
 
     def child_process(self):
 
-        # Print the element's environment at the beginning of any element's log file.
-        #
-        # This should probably be omitted for non-build tasks but it's harmless here
-        elt_env = self._element.get_environment()
-        env_dump = yaml.round_trip_dump(elt_env, default_flow_style=False, allow_unicode=True)
-        self.message(MessageType.LOG, "Build environment for element {}".format(self._element.name), detail=env_dump)
-
         # Run the action
         return self._action_cb(self._element)
 
diff --git a/src/buildstream/_scheduler/queues/buildqueue.py b/src/buildstream/_scheduler/queues/buildqueue.py
index 5cbd5af57..c7de0b880 100644
--- a/src/buildstream/_scheduler/queues/buildqueue.py
+++ b/src/buildstream/_scheduler/queues/buildqueue.py
@@ -18,11 +18,8 @@
 #        Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
 #        Jürg Billeter <juerg.billeter@codethink.co.uk>
 
-from datetime import timedelta
-
 from . import Queue, QueueStatus
 from ..resources import ResourceType
-from ..._message import MessageType
 from ..jobs import JobStatus
 
 
@@ -34,38 +31,6 @@ class BuildQueue(Queue):
     complete_name = "Built"
     resources = [ResourceType.PROCESS, ResourceType.CACHE]
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self._tried = set()
-
-    def enqueue(self, elts):
-        to_queue = []
-
-        for element in elts:
-            if not element._cached_failure() or element in self._tried:
-                to_queue.append(element)
-                continue
-
-            # XXX: Fix this, See https://mail.gnome.org/archives/buildstream-list/2018-September/msg00029.html
-            # Bypass queue processing entirely the first time it's tried.
-            self._tried.add(element)
-            _, description, detail = element._get_build_result()
-            logfile = element._get_build_log()
-            self._message(
-                element,
-                MessageType.FAIL,
-                description,
-                detail=detail,
-                action_name=self.action_name,
-                elapsed=timedelta(seconds=0),
-                logfile=logfile,
-            )
-            self._done_queue.append(element)
-            element_name = element._get_full_name()
-            self._task_group.add_failed_task(element_name)
-
-        return super().enqueue(to_queue)
-
     def get_process_func(self):
         return BuildQueue._assemble_element
 
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 547397e54..4c3f239a1 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -86,11 +86,12 @@ import string
 from typing import cast, TYPE_CHECKING, Any, Dict, Iterator, List, Optional, Set
 
 from pyroaring import BitMap  # pylint: disable=no-name-in-module
+from ruamel import yaml
 
 from . import _yaml
 from ._variables import Variables
 from ._versions import BST_CORE_ARTIFACT_VERSION
-from ._exceptions import BstError, LoadError, ImplError, SourceCacheError
+from ._exceptions import BstError, LoadError, ImplError, SourceCacheError, CachedFailure
 from .exceptions import ErrorDomain, LoadErrorReason
 from .utils import FileListResult, BST_ARBITRARY_TIMESTAMP
 from . import utils
@@ -255,7 +256,6 @@ class Element(Plugin):
         self.__required = False  # Whether the artifact is required in the current session
         self.__artifact_files_required = False  # Whether artifact files are required in the local cache
         self.__build_result = None  # The result of assembling this Element (success, description, detail)
-        self._build_log_path = None  # The path of the build log for this Element
         # Artifact class for direct artifact composite interaction
         self.__artifact = None  # type: Optional[Artifact]
         self.__strict_artifact = None  # Artifact for strict cache key
@@ -1467,7 +1467,7 @@ class Element(Plugin):
             self.__artifact
             and
             # And we're not cached yet
-            not self._cached()
+            not self._cached_success()
         )
 
     # __schedule_assembly_when_necessary():
@@ -1508,7 +1508,6 @@ class Element(Plugin):
     def _assemble_done(self, successful):
         assert self.__assemble_scheduled
 
-        self.__assemble_scheduled = False
         self.__assemble_done = True
 
         self.__strict_artifact.reset_cached()
@@ -1557,9 +1556,27 @@ class Element(Plugin):
     #
     def _assemble(self):
 
+        # Only do this the first time around (i.e. __assemble_done is False)
+        # to allow for retrying the job
+        if self._cached_failure() and not self.__assemble_done:
+            with self._output_file() as output_file:
+                for log_path in self.__artifact.get_logs():
+                    with open(log_path) as log_file:
+                        output_file.write(log_file.read())
+
+            _, description, detail = self._get_build_result()
+            e = CachedFailure(description, detail=detail)
+            # Shelling into a sandbox is useful to debug this error
+            e.sandbox = True
+            raise e
+
         # Assert call ordering
         assert not self._cached_success()
 
+        # Print the environment at the beginning of the log file.
+        env_dump = yaml.round_trip_dump(self.get_environment(), default_flow_style=False, allow_unicode=True)
+        self.log("Build environment for element {}".format(self.name), detail=env_dump)
+
         context = self._get_context()
         with self._output_file() as output_file:
 
@@ -1680,9 +1697,6 @@ class Element(Plugin):
 
         return artifact_size
 
-    def _get_build_log(self):
-        return self._build_log_path
-
     # _fetch_done()
     #
     # Indicates that fetching the sources for this element has been done.
diff --git a/tests/integration/interactive_build.py b/tests/integration/interactive_build.py
index c0b087126..285cb86f8 100644
--- a/tests/integration/interactive_build.py
+++ b/tests/integration/interactive_build.py
@@ -42,7 +42,7 @@ def build_session(datafiles, element_name):
 @pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.parametrize("element_name", ["interactive/failed-build.bst"])
-@pytest.mark.parametrize("choice", ["continue", "quit", "terminate", "retry"])
+@pytest.mark.parametrize("choice", ["continue", "quit", "terminate"])
 def test_failed_build_quit(element_name, build_session, choice):
     build_session.expect_exact("Choice: [continue]:", timeout=PEXPECT_TIMEOUT_LONG)
     build_session.sendline(choice)
@@ -55,6 +55,21 @@ def test_failed_build_quit(element_name, build_session, choice):
 @pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
 @pytest.mark.datafiles(DATA_DIR)
 @pytest.mark.parametrize("element_name", ["interactive/failed-build.bst"])
+def test_failed_build_retry(element_name, build_session):
+    build_session.expect_exact("Choice: [continue]:", timeout=PEXPECT_TIMEOUT_LONG)
+    build_session.sendline("retry")
+
+    build_session.expect_exact("Choice: [continue]:", timeout=PEXPECT_TIMEOUT_LONG)
+    build_session.sendline("quit")
+
+    build_session.expect_exact(pexpect.EOF)
+    build_session.close()
+    assert build_session.exitstatus == 255
+
+
+@pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.parametrize("element_name", ["interactive/failed-build.bst"])
 def test_failed_build_log(element_name, build_session):
     build_session.expect_exact("Choice: [continue]:", timeout=PEXPECT_TIMEOUT_LONG)
     build_session.sendline("log")