Skip to content

features: terminate installs on ctrl-c and --fail-fast failures#15295

Merged
tldahlgren merged 2 commits intospack:developfrom
tldahlgren:features/support-cancelling-new-build
Jun 23, 2020
Merged

features: terminate installs on ctrl-c and --fail-fast failures#15295
tldahlgren merged 2 commits intospack:developfrom
tldahlgren:features/support-cancelling-new-build

Conversation

@tldahlgren
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren commented Mar 2, 2020 •
edited
Loading

Fixes #15182
Fixes #15684

This change restores the ability to terminate spack install using ctrl-c*.

It also adds support for a --fail-fast install option. Instead of proceeding with a "best effort" installation of packages associated with a spack install, spack install --fail-fast will terminate the installation process upon detection of an install failure (i.e., by the process or another, overlapping install process).

*Note that I am able to successfully terminate an installation using develop by entering ctrl-c twice in a row. This PR reduces the number of times it needs to be entered to one.

TODO

  • Address PR feedback
  • Add option to documentation

Follow-On Work

@tldahlgren tldahlgren requested a review from tgamblin March 2, 2020 20:53
@tldahlgren tldahlgren self-assigned this Mar 2, 2020
@tldahlgren tldahlgren force-pushed the features/support-cancelling-new-build branch from 9f1ca54 to f2ae3ab Compare March 2, 2020 22:22
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 3, 2020

@wortiz Does this fix PR fix your ctrl-c problem?

@wortiz
Copy link
Copy Markdown
Contributor

wortiz commented Mar 3, 2020

Yes it appears to allow ctrl-c to exit immediately for me

@scheibelp scheibelp requested review from scheibelp and removed request for tgamblin March 4, 2020 00:42
@scheibelp scheibelp self-assigned this Mar 4, 2020
@tldahlgren tldahlgren force-pushed the features/support-cancelling-new-build branch from 4818e87 to 188b3e3 Compare March 4, 2020 19:41
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 4, 2020

Split the two features into two separate commits.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 4, 2020

@scheibelp I'm done with the split into two commits and the tests passed.

@tldahlgren tldahlgren changed the title feature/terminate installs on ctrl-c and --fail-fast failures features: terminate installs on ctrl-c and --fail-fast failures Mar 5, 2020
scheibelp
scheibelp requested changes Mar 5, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

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

I have a question and a few requests

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 5, 2020

@scheibelp Given your feedback involves changes to each of the separate features/commits, I'll assume that I need to split one of them -- the smaller is ctrl-c -- into a separate PR in order to maintain a separate commit for each. Right?

@scheibelp
Copy link
Copy Markdown
Member

scheibelp commented Mar 5, 2020

I'll assume that I need to split one of them -- the smaller is ctrl-c -- into a separate PR in order to maintain a separate commit for each. Right?

You can do that. Another option is to start by just tacking on the fixes, then (assuming the commits can be grouped cleanly) you can rebase and treat the new commits as "fixup". Both this and extracting into another PR work fine.

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Mar 5, 2020

@tldahlgren: see https://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 5, 2020 •
edited
Loading

@tldahlgren: see https://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html

I've been using git rebase -i HEAD~<#> but guess I didn't pay enough attention as I was under the impression you could only affect the immediately preceding commit(s) versus "skip over" one.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 9, 2020 •
edited
Loading

@tldahlgren: see https://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html

I've been using git rebase -i HEAD~<#> but guess I didn't pay enough attention as I was under the impression you could only affect the immediately preceding commit(s) versus "skip over" one.

Git was telling me there were differences when I checked the branch out again so I did a git pull, which resulted in a strange mix of pre-rebase commits. So I ended up re-entering the changes while integrating scheibelp's feedback to restore a 2-commit version of the changes.

@tldahlgren tldahlgren closed this Mar 9, 2020
@tldahlgren tldahlgren force-pushed the features/support-cancelling-new-build branch from 188b3e3 to fd69994 Compare March 9, 2020 21:47
@tldahlgren tldahlgren reopened this Mar 9, 2020
@tldahlgren tldahlgren closed this Mar 9, 2020
@tldahlgren tldahlgren reopened this Mar 9, 2020
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 10, 2020

@scheibelp ping

@tldahlgren tldahlgren mentioned this pull request Mar 12, 2020
Closed
2 tasks
@tldahlgren tldahlgren force-pushed the features/support-cancelling-new-build branch 3 times, most recently from 44c5117 to 10bc92d Compare March 20, 2020 00:26
@adrienbernede
Copy link
Copy Markdown
Contributor

adrienbernede commented Jun 12, 2020

@tldahlgren I ran into this issue today. So I am supportive of your work in this PR.

Note: on MacOS, hitting ctrl-C twice in a row does not stop the installation, it has the same behavior as one hit and will go to the next install.

@adrienbernede
Copy link
Copy Markdown
Contributor

adrienbernede commented Jun 12, 2020

And I confirm that this PR works for me.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 22, 2020

Closing as part of re-triggering the tests.

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

tldahlgren commented Jun 22, 2020

Re-opening to trigger the tests.

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

tldahlgren commented Jun 22, 2020

@scheibelp @becker33 ping

@scheibelp
Copy link
Copy Markdown
Member

scheibelp commented Jun 22, 2020

Has #15295 (comment) been addressed yet? I haven't seen a comment on it.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 22, 2020

I will merge the last 2 commits once the tests pass.

@tldahlgren tldahlgren force-pushed the features/support-cancelling-new-build branch from d84534c to 182e96a Compare June 23, 2020 01:13
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Jun 23, 2020

@becker33 @scheibelp The PR tests passed so I rebased the third commit (test change) into the second. So, once the tests pass, this PR can be pushed to develop as 2 commits: one for ctrl-c and the other for --fail-fast.

@tldahlgren tldahlgren merged commit 96932d6 into spack:develop Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@becker33 becker33 becker33 left review comments

@scheibelp scheibelp scheibelp approved these changes

Assignees

@scheibelp scheibelp

@tldahlgren tldahlgren

Labels

impact-high radiuss

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feature: Add option to terminate spack install after first failure Spack continues installing packages after failure

7 participants

@tldahlgren @wortiz @scheibelp @tgamblin @adrienbernede @becker33 @adamjstewart