From 7c6b253d89cb4e91c2683d150e46da00a6085695 Mon Sep 17 00:00:00 2001
From: Jordan Galby <67924449+Jordan474@users.noreply.github.com>
Date: Tue, 9 Nov 2021 16:47:32 +0100
Subject: Fix log-format reporter ignoring install errors (#25961)

When running `spack install --log-format junit|cdash ...`, install
errors were ignored. This made spack continue building dependents of
failed install, ignoring `--fail-fast`, and exit 0 at the end.
---
 lib/spack/spack/report.py           | 34 +++++++++++++++++---------------
 lib/spack/spack/test/cmd/install.py | 39 ++++++++++++++++++++++++-------------
 lib/spack/spack/test/cmd/test.py    |  6 ++++--
 3 files changed, 48 insertions(+), 31 deletions(-)

(limited to 'lib')

diff --git a/lib/spack/spack/report.py b/lib/spack/spack/report.py
index 4685321bc8..83bb89e27f 100644
--- a/lib/spack/spack/report.py
+++ b/lib/spack/spack/report.py
@@ -151,6 +151,22 @@ class InfoCollector(object):
                     'installed_from_binary_cache': False
                 }
 
+                # Append the package to the correct spec report. In some
+                # cases it may happen that a spec that is asked to be
+                # installed explicitly will also be installed as a
+                # dependency of another spec. In this case append to both
+                # spec reports.
+                for s in llnl.util.lang.dedupe([pkg.spec.root, pkg.spec]):
+                    name = name_fmt.format(s.name, s.dag_hash(length=7))
+                    try:
+                        item = next((
+                            x for x in self.specs
+                            if x['name'] == name
+                        ))
+                        item['packages'].append(package)
+                    except StopIteration:
+                        pass
+
                 start_time = time.time()
                 value = None
                 try:
@@ -170,6 +186,7 @@ class InfoCollector(object):
                     package['stdout'] = fetch_log(pkg, do_fn, self.dir)
                     package['stdout'] += package['message']
                     package['exception'] = e.traceback
+                    raise
 
                 except (Exception, BaseException) as e:
                     # Everything else is an error (the installation
@@ -178,26 +195,11 @@ class InfoCollector(object):
                     package['stdout'] = fetch_log(pkg, do_fn, self.dir)
                     package['message'] = str(e) or 'Unknown error'
                     package['exception'] = traceback.format_exc()
+                    raise
 
                 finally:
                     package['elapsed_time'] = time.time() - start_time
 
-                # Append the package to the correct spec report. In some
-                # cases it may happen that a spec that is asked to be
-                # installed explicitly will also be installed as a
-                # dependency of another spec. In this case append to both
-                # spec reports.
-                for s in llnl.util.lang.dedupe([pkg.spec.root, pkg.spec]):
-                    name = name_fmt.format(s.name, s.dag_hash(length=7))
-                    try:
-                        item = next((
-                            x for x in self.specs
-                            if x['name'] == name
-                        ))
-                        item['packages'].append(package)
-                    except StopIteration:
-                        pass
-
                 return value
 
             return wrapper
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index b0b10d1287..4fa022d168 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -393,9 +393,14 @@ def test_junit_output_with_failures(tmpdir, exc_typename, msg):
             '--log-format=junit', '--log-file=test.xml',
             'raiser',
             'exc_type={0}'.format(exc_typename),
-            'msg="{0}"'.format(msg)
+            'msg="{0}"'.format(msg),
+            fail_on_error=False,
         )
 
+    assert isinstance(install.error, spack.build_environment.ChildError)
+    assert install.error.name == exc_typename
+    assert install.error.pkg.name == 'raiser'
+
     files = tmpdir.listdir()
     filename = tmpdir.join('test.xml')
     assert filename in files
@@ -407,18 +412,22 @@ def test_junit_output_with_failures(tmpdir, exc_typename, msg):
     assert 'failures="1"' in content
     assert 'errors="0"' in content
 
+    # Nothing should have succeeded
+    assert 'tests="0"' not in content
+    assert 'failures="0"' not in content
+
     # We want to have both stdout and stderr
     assert '<system-out>' in content
     assert msg in content
 
 
 @pytest.mark.disable_clean_stage_check
-@pytest.mark.parametrize('exc_typename,msg', [
-    ('RuntimeError', 'something weird happened'),
-    ('KeyboardInterrupt', 'Ctrl-C strikes again')
+@pytest.mark.parametrize('exc_typename,expected_exc,msg', [
+    ('RuntimeError', spack.installer.InstallError, 'something weird happened'),
+    ('KeyboardInterrupt', KeyboardInterrupt, 'Ctrl-C strikes again')
 ])
 def test_junit_output_with_errors(
-        exc_typename, msg,
+        exc_typename, expected_exc, msg,
         mock_packages, mock_archive, mock_fetch, install_mockery,
         config, tmpdir, monkeypatch):
 
@@ -429,11 +438,11 @@ def test_junit_output_with_errors(
     monkeypatch.setattr(spack.installer.PackageInstaller, '_install_task',
                         just_throw)
 
-    # TODO: Why does junit output capture appear to swallow the exception
-    # TODO: as evidenced by the two failing packages getting tagged as
-    # TODO: installed?
     with tmpdir.as_cwd():
-        install('--log-format=junit', '--log-file=test.xml', 'libdwarf')
+        install('--log-format=junit', '--log-file=test.xml', 'libdwarf',
+                fail_on_error=False)
+
+    assert isinstance(install.error, expected_exc)
 
     files = tmpdir.listdir()
     filename = tmpdir.join('test.xml')
@@ -441,10 +450,14 @@ def test_junit_output_with_errors(
 
     content = filename.open().read()
 
-    # Count failures and errors correctly: libdwarf _and_ libelf
-    assert 'tests="2"' in content
+    # Only libelf error is reported (through libdwarf root spec). libdwarf
+    # install is skipped and it is not an error.
+    assert 'tests="1"' in content
     assert 'failures="0"' in content
-    assert 'errors="2"' in content
+    assert 'errors="1"' in content
+
+    # Nothing should have succeeded
+    assert 'errors="0"' not in content
 
     # We want to have both stdout and stderr
     assert '<system-out>' in content
@@ -877,7 +890,7 @@ def test_install_help_cdash(capsys):
 
 
 @pytest.mark.disable_clean_stage_check
-def test_cdash_auth_token(tmpdir, install_mockery, capfd):
+def test_cdash_auth_token(tmpdir, mock_fetch, install_mockery, capfd):
     # capfd interferes with Spack's capturing
     with tmpdir.as_cwd():
         with capfd.disabled():
diff --git a/lib/spack/spack/test/cmd/test.py b/lib/spack/spack/test/cmd/test.py
index df9fb35db1..9ee117b281 100644
--- a/lib/spack/spack/test/cmd/test.py
+++ b/lib/spack/spack/test/cmd/test.py
@@ -133,7 +133,8 @@ def test_junit_output_with_failures(tmpdir, mock_test_stage, pkg_name, msgs):
     with tmpdir.as_cwd():
         spack_test('run',
                    '--log-format=junit', '--log-file=test.xml',
-                   pkg_name)
+                   pkg_name,
+                   fail_on_error=False)
 
     files = tmpdir.listdir()
     filename = tmpdir.join('test.xml')
@@ -160,7 +161,8 @@ def test_cdash_output_test_error(
         spack_test('run',
                    '--log-format=cdash',
                    '--log-file=cdash_reports',
-                   'test-error')
+                   'test-error',
+                   fail_on_error=False)
         report_dir = tmpdir.join('cdash_reports')
         print(tmpdir.listdir())
         assert report_dir in tmpdir.listdir()
-- 
cgit v1.2.3-60-g2f50