-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support interactive issue resolution prompts #386
Conversation
861e97d
to
5ec0a86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me. But do traps work in old bash? I remember having some issues early in the history of live-bootstrap.
Traps do work when used like this; the normal interactive prompt doesn't work, but this one does. I have hit it a few times during development already, and used the trap prompt successfully to fix the issue and continue bootstrapping. Maybe EXIT traps don't work (that's what was used in live-bootstrap before the simplify refactor), but ERR ones do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps change the variable name for using_bash... given it's not a boolean anymore.
Otherwise, OK with this implementation. In the future I think I'd like to support auto-interactivity in the case where bootstrap.cfg doesn't exist in the initial filesystem, but that would come later..
Just a quick heads up - I've just discovered a bug in this. When -i is given, checksum failures after switching to the first Bash don't trigger a drop to the trap as intended, but only print a warning. Fortunately, the fix should be easy ("set -E" needs to be set to propagate the trap to functions). |
Perhaps |
"bash_rebuild" is slightly off semantically, as 0 means "no bash", 1 means "initial build of bash" (= no rebuilds), and 2 is "2nd build of bash") (= 1 rebuild). "bash_build" works, however. |
Pushed the rename to bash_build. |
This adds a new flag, -i / --interactive, which enables opening a Bash prompt whenever something goes wrong in the bootstrap. This is highly useful when developing or debugging live-bootstrap, but it needs to be off by default, for use in automated processes. In the future, asking for variables at runtime could (and perhaps should) also be gated behind this flag.
Ouch, the "set -E" fix causes a trap in the autogen build EDIT: this appears to be a legit issue in the autogen bootstrap that we previously missed! I suggest that we merge this PR in the current state (unless there are other outstanding issues), and then fix the underlying issue in autogen separately, especially since this doesn't break existing bootstrap paths, only affecting the new "-i" option. |
This adds a new flag, -i / --interactive, which enables opening a
Bash prompt whenever something goes wrong in the bootstrap. This is
highly useful when developing or debugging live-bootstrap, but it
needs to be off by default, for use in automated processes.
In the future, asking for variables at runtime could (and perhaps
should) also be gated behind this flag.
Depends on #385