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

handle Ctrl-C explicitly in confirm prompt #11706

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ericmarkmartin
Copy link
Contributor

Summary

Resolves #11704

Replace uv-console's use of ctrlc::set_handler to exit the prompt with an explicit matching on the user inputting Ctrl-C. This is accomplished via console's read_key_raw, which is exactly like read_key except it returns an explicit value indicating a user entered CtrlC rather than implicitly raising SIGINT and returning an error.

Test Plan

Replace `uv-console`'s use of `ctrlc::set_handler` to exit the prompt with an explicit
matching on the user inputting Ctrl-C.
@ericmarkmartin
Copy link
Contributor Author

Took a brief stab at testing, but failed. Not sure how to test the behavior when we need the subprocess to be interactive in order to see the confirmation prompt. For reference, this is what I attempted to no avail (the snapshot looks like what happens if you answer the confirmation prompt with no).

#[test]
fn ctrl_c_install_confirmation_prompt() -> Result<()> {
    let context = TestContext::new("3.12");

    context.temp_dir.child("pyproject.toml").touch()?;

    let ctrl_c_file = context.temp_dir.child("ctrl_c");

    ctrl_c_file.write_binary(&[0x03])?;

    let file_handle = std::fs::File::open(ctrl_c_file)?;

    uv_snapshot!(context.filters(), context.pip_install()
        .arg("pyproject.toml") // triggers confirmation prompt because user probably wants `-r`
        .stdin(file_handle), @r###"
        "###
    );

    Ok(())
}

Copy link
Contributor Author

@ericmarkmartin ericmarkmartin left a comment

Choose a reason for hiding this comment

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

I've added a test that seems to work using pty, which I get out of wezterm's pty implementation. One thing to note is that the read out of the pty is non-blocking so if our confirmation prompt changes and becomes smaller, the test will hang, which isn't great. Still thinking of ways to fix that.

Part of making the test workable was a drive-by change so that the confirmation prompt respects the global color setting

@ericmarkmartin ericmarkmartin marked this pull request as draft February 24, 2025 03:29
@ericmarkmartin
Copy link
Contributor Author

Apparently the fix doesn't work on windows, so converting back to draft while I workshop a bit

@konstin
Copy link
Member

konstin commented Feb 24, 2025

Can you try forwarding the io error? The handler seems working, but i think the main thread panics too fast.

@ericmarkmartin
Copy link
Contributor Author

Can you try forwarding the io error? The handler seems working, but i think the main thread panics too fast.

Forwarding it out of where? I think the main thread panics because we forward it out of the confirm function and unwrap the containing Result, no?

@konstin
Copy link
Member

konstin commented Feb 25, 2025

Besides the behavior changes to signal handling, we would ideally not have an .unwrap() call at all, usually we can just propagate the error to the caller through a thiserror derive enum variant.

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.

Did you mean [...]? + ctr-c => thread 'main2' panicked
2 participants