Skip to content
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

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

Googulator
Copy link
Collaborator

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

@Googulator Googulator force-pushed the debug-trap branch 3 times, most recently from 861e97d to 5ec0a86 Compare January 1, 2024 22:08
Copy link
Collaborator

@stikonas stikonas left a 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.

@Googulator
Copy link
Collaborator Author

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.

Copy link
Owner

@fosslinux fosslinux left a 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..

@Googulator
Copy link
Collaborator Author

I was thinking "not using Bash" (0) -> "using Bash #1" -> "using Bash #2", not necessarily a boolean.

What would be a better variable name for you?

@Googulator
Copy link
Collaborator Author

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).

@fosslinux
Copy link
Owner

What would be a better variable name for you?

Perhaps bash_rebuild?
It's not a huge deal though.

@Googulator
Copy link
Collaborator Author

"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.

@Googulator
Copy link
Collaborator Author

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.
@Googulator
Copy link
Collaborator Author

Googulator commented Jan 8, 2024

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.

@fosslinux fosslinux merged commit 31753cc into fosslinux:master Jan 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants