Skip to content

features: Add install failure tracking removal through spack clean#15314

Merged
becker33 merged 13 commits intospack:developfrom
tldahlgren:features/clean-install-failures
Jun 25, 2020
Merged

features: Add install failure tracking removal through spack clean#15314
becker33 merged 13 commits intospack:developfrom
tldahlgren:features/clean-install-failures

Conversation

@tldahlgren
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren commented Mar 3, 2020 •
edited
Loading

Note this PR addresses a feature a user asked about when #13100 was under testing.

The distributed build process automatically removes failure tracking information for relevant packages when spack install is executed. This has been shown to cause problems with environment installations from spack.yaml files -- see #15415 -- that led to the need to optionally retain failure tracking.

This PR adds the ability to explicitly remove install failure tracking data for a Spack instance through the spack clean command using either the -f or -a options.

TODO

  • Add option to documentation

Follow-On Work

@tldahlgren tldahlgren self-assigned this Mar 3, 2020
@tldahlgren tldahlgren force-pushed the features/clean-install-failures branch from f9895b0 to fca96a5 Compare March 4, 2020 01:51
@tldahlgren tldahlgren closed this Mar 4, 2020
@tldahlgren tldahlgren reopened this Mar 4, 2020
@tldahlgren tldahlgren closed this Mar 4, 2020
@tldahlgren tldahlgren reopened this Mar 4, 2020
@tldahlgren tldahlgren force-pushed the features/clean-install-failures branch from 15f0302 to b4d0773 Compare March 4, 2020 21:33
@tldahlgren tldahlgren changed the title Add install failure tracking removal through spack clean Feature: Add install failure tracking removal through spack clean Mar 4, 2020
@tldahlgren tldahlgren changed the title Feature: Add install failure tracking removal through spack clean features: Add install failure tracking removal through spack clean Mar 5, 2020
@tldahlgren tldahlgren requested review from alalazo and scheibelp March 5, 2020 00:51
@tldahlgren tldahlgren force-pushed the features/clean-install-failures branch from 0ea06a8 to 0f5d4ae Compare March 9, 2020 20:31
@tldahlgren tldahlgren mentioned this pull request Mar 13, 2020
Closed
2 tasks
@tgamblin tgamblin self-assigned this Mar 24, 2020
tgamblin
tgamblin previously requested changes Mar 25, 2020
View reviewed changes
lib/spack/spack/installer.py
"""
Remove all failure tracking markers for the Spack instance.
"""
spack.store.db.clear_all_failures()
Copy link
Copy Markdown
Member

@tgamblin tgamblin Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not opposed to this function but it does seem to indicate that the failure file management should get its own class.

Copy link
Copy Markdown
Contributor Author

@tldahlgren tldahlgren Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Where does the refactor fit within the priorities?

@tldahlgren tldahlgren force-pushed the features/clean-install-failures branch from 08ca9cb to 8e7c70a Compare June 22, 2020 18:59
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 22, 2020

FWIW. Merged develop since local testing was failing for me due to assertions related to missing provider entries in etc/spack/defaults/packages.yaml. Post-merge local tests then had issues with test_make_elf_binaries_relative (fixed in a separate PR) and test_clean_default.

@becker33 becker33 assigned becker33 and unassigned tgamblin Jun 23, 2020
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 23, 2020 •
edited
Loading

Note project and patch coverage diffs show two uncovered lines that were unchanged in by this PR.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 23, 2020

Interesting about the foreground/background test failing on MacOS (see below). I've seen this on other PRs as well.

=================================== FAILURES ===================================
________ test_foreground_background_output[test_fn1-termios_on_or_off1] ________

test_fn = <function mock_shell_v_v_no_termios at 0x10b296dd0>
capfd = <_pytest.capture.CaptureFixture object at 0x11ac22bd0>
termios_on_or_off = <function no_termios at 0x10b296b90>
tmpdir = local('/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_foreground_background_out1')

    @pytest.mark.skipif(not which("ps"), reason="requires ps utility")
    @pytest.mark.skipif(not termios, reason="requires termios support")
    @pytest.mark.parametrize('test_fn,termios_on_or_off', [
        (mock_shell_v_v, nullcontext),
        (mock_shell_v_v_no_termios, no_termios),
    ])
    def test_foreground_background_output(
            test_fn, capfd, termios_on_or_off, tmpdir):

@tldahlgren tldahlgren closed this Jun 23, 2020
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 23, 2020

Oops. That was supposed to be a comment, not closing the PR.

@tldahlgren tldahlgren reopened this Jun 23, 2020
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 24, 2020

@becker33 The coverage "issue" is with untouched code near the changes.

@becker33 becker33 dismissed tgamblin’s stale review June 24, 2020 22:22

All requests from this review have been satisfied

@becker33 becker33 merged commit 48d3e8d into spack:develop Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@becker33 becker33 becker33 approved these changes

@tgamblin tgamblin tgamblin left review comments

@adamjstewart adamjstewart Awaiting requested review from adamjstewart

@alalazo alalazo Awaiting requested review from alalazo

@scheibelp scheibelp Awaiting requested review from scheibelp

Assignees

@becker33 becker33

@tldahlgren tldahlgren

Labels

commands

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tldahlgren @tgamblin @becker33 @scheibelp