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(-) 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 '' 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 '' 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-70-g09d2