Merge branch 'jennis/fix_artifact_log' into 'master'

Fix artifact log

See merge request BuildStream/buildstream!1533

6 files changed, 52 insertions, 25 deletions

diff --git a/src/buildstream/_artifact.py b/src/buildstream/_artifact.py
index 5499b0dab..ff5bae4a1 100644
--- a/src/buildstream/_artifact.py
+++ b/src/buildstream/_artifact.py
@@ -89,6 +89,22 @@ class Artifact():
 
         return CasBasedDirectory(self._cas, digest=buildtree_digest)
 
+    # get_logs():
+    #
+    # Get the paths of the artifact's logs
+    #
+    # Returns:
+    #    (list): A list of object paths
+    #
+    def get_logs(self):
+        artifact = self._get_proto()
+
+        logfile_paths = []
+        for logfile in artifact.logs:
+            logfile_paths.append(self._cas.objpath(logfile.digest))
+
+        return logfile_paths
+
     # get_extract_key():
     #
     # Get the key used to extract the artifact
@@ -385,7 +401,7 @@ class Artifact():
         artifact = self._get_proto()
 
         for logfile in artifact.logs:
-            if not self._cas.contains(logfile.digest.hash):
+            if not self._cas.contains_file(logfile.digest):
                 return False
 
         return True
@@ -425,7 +441,7 @@ class Artifact():
 
         return self._proto
 
-    # _get_artifact_field()
+    # _get_field_digest()
     #
     # Returns:
     #     (Digest): Digest of field specified
diff --git a/src/buildstream/_cas/cascache.py b/src/buildstream/_cas/cascache.py
index b5089e36c..ff16480b1 100644
--- a/src/buildstream/_cas/cascache.py
+++ b/src/buildstream/_cas/cascache.py
@@ -116,9 +116,21 @@ class CASCache():
         # This assumes that the repository doesn't have any dangling pointers
         return os.path.exists(refpath)
 
+    # contains_file():
+    #
+    # Check whether a digest corresponds to a file which exists in CAS
+    #
+    # Args:
+    #     digest (Digest): The file digest to check
+    #
+    # Returns: True if the file is in the cache, False otherwise
+    #
+    def contains_file(self, digest):
+        return os.path.exists(self.objpath(digest))
+
     # contains_directory():
     #
-    # Check whether the specified directory and subdirecotires are in the cache,
+    # Check whether the specified directory and subdirectories are in the cache,
     # i.e non dangling.
     #
     # Args:
diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index 81ee4ad83..220d10477 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -1,8 +1,6 @@
 import os
 import sys
-from contextlib import ExitStack
 from functools import partial
-from tempfile import TemporaryDirectory
 import fcntl
 
 import click
@@ -1196,23 +1194,13 @@ def artifact_log(app, artifacts):
     # they are not somehow escaped.
 
     with app.initialized():
-        logsdirs = app.stream.artifact_log(artifacts)
-
-        with ExitStack() as stack:
-            extractdirs = []
-            for logsdir in logsdirs:
-                # NOTE: If reading the logs feels unresponsive, here would be a good place
-                # to provide progress information.
-                td = stack.enter_context(TemporaryDirectory())
-                logsdir.export_files(td, can_link=True)
-                extractdirs.append(td)
-
-            for extractdir in extractdirs:
-                for log in (os.path.join(extractdir, log) for log in os.listdir(extractdir)):
-                    # NOTE: Should click gain the ability to pass files to the pager this can be optimised.
-                    with open(log) as f:
-                        data = f.read()
-                        click.echo_via_pager(data)
+        log_file_paths = app.stream.artifact_log(artifacts)
+
+        for log in log_file_paths:
+            with open(log) as f:
+                data = f.read()
+
+            click.echo_via_pager(data)
 
 
 ###################################################################
diff --git a/src/buildstream/_stream.py b/src/buildstream/_stream.py
index 410b601b8..36610434b 100644
--- a/src/buildstream/_stream.py
+++ b/src/buildstream/_stream.py
@@ -603,7 +603,7 @@ class Stream():
         # Return list of Element and/or ArtifactElement objects
         target_objects = self.load_selection(targets, selection=PipelineSelection.NONE, load_refs=True)
 
-        logsdirs = []
+        log_file_paths = []
         for obj in target_objects:
             ref = obj.get_artifact_name()
             if not obj._cached():
@@ -613,9 +613,9 @@ class Stream():
                 self._message(MessageType.WARN, "{} is cached without log files".format(ref))
                 continue
 
-            logsdirs.append(self._artifacts.get_artifact_logs(ref))
+            log_file_paths.extend(obj.get_logs())
 
-        return logsdirs
+        return log_file_paths
 
     # artifact_delete()
     #
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 208bc35b2..240e8ce7d 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -925,6 +925,14 @@ class Element(Plugin):
         self.__batch_prepare_assemble_flags = flags
         self.__batch_prepare_assemble_collect = collect
 
+    def get_logs(self):
+        """Obtain a list of log file paths
+
+        Returns:
+           (list): A list of log file paths
+        """
+        return self.__artifact.get_logs()
+
     #############################################################
     #            Private Methods used in BuildStream            #
     #############################################################
diff --git a/tests/frontend/artifact.py b/tests/frontend/artifact.py
index eb187a168..177be8c30 100644
--- a/tests/frontend/artifact.py
+++ b/tests/frontend/artifact.py
@@ -56,6 +56,9 @@ def test_artifact_log(cli, datafiles):
     assert result.exit_code == 0
     log = result.output
 
+    # Assert that there actually was a log file
+    assert log != ''
+
     # Read the log via the key
     result = cli.run(project=project, args=['artifact', 'log', 'test/target/' + key])
     assert result.exit_code == 0