search

rust-cli / human-panic

Panic messages for humans.
https://docs.rs/human-panic
Apache License 2.0
1.66k stars 65 forks source link

Optionally display the message in native windows for linux + windows #39

Closed rukai closed 6 years ago

rukai commented 6 years ago

This is a 🙋 feature

Description

GUI applications cant display the message on the terminal so I have added a native message box so they can use human-panic. The x11 implementation is based on the SDL ShowSimpleMessageBox source code: https://github.com/spurious/SDL-mirror/blob/master/src/video/x11/SDL_x11messagebox.c Non-ascii characters aren't working on the linux implementation, no idea whats up with that. Mac OS is left unimplemented as I don't have access to such a machine. Enabled by the gui feature

Scope

This may not fit it into the scope of this project, due to:

This feature is something I need, so if this change doesn't fit into this project I will need to maintain a fork.

Checklist

Semver Changes

Major change

Screenshots

Linux (i3 wm)

linuxmessagebox

Windows

windowsmessagebox

rukai commented 6 years ago

Looks like I am rustfmt'ing wrong. Will have to look into it tomorrow.

yoshuawuyts commented 6 years ago

I really like this conceptually! I think like @spacekookie said having this as a separate struct / module inside this repo would be great! I agree having this be opt-in would probably be preferred.

Something I'm not entirely convinced of is adding feature flags just for the gui. I feel it isn't great user experience; changing runtime behavior based on a setting inside Cargo.toml makes it quite hard to spot differences. Also it seems I'm not the only one that feels this way about feature flags.

epage commented 6 years ago

Something I'm not entirely convinced of is adding feature flags just for the gui. I feel it isn't great user experience; changing runtime behavior based on a setting inside Cargo.toml makes it quite hard to spot differences. Also it seems I'm not the only one that feels this way about feature flags.

imo adding extra dependencies for non-GUI people seems undesirable. If we agree on that, then its a matter of how to handle it.

Any others?

yoshuawuyts commented 6 years ago

@epage edge cases aside, I don't think that the amount of data that's stored on disk is ever a bottleneck. During compilation unused methods are removed anyway, making the resulting binary only include the code that's used.

So unless this is proving to be a tremendous amount of extra data (like >= 1gb extra stored on disk), I don't think we should shape the API around that constraint.

But to perhaps find a middleground: what if we have a feature flag that's enabled by default, we can get the best of both worlds? Both a convenient way of choosing which method to use, and a flag to trim any overhead for disk-constrained development environments.

spacekookie commented 6 years ago

@yoshuawuyts I don't think that letting a feature flag determine runtime behaviour is a bad thing at all. There are plenty of crates to do this and considering that only a small minority of people will want to use the GUI feature of this crate, making it anything other than opt-in would be irresponsible.

(EDIT: That being said, if you feel strongly about not letting feature flags influence runtime behaviour, we can keep the show_window option which can default to false to make it really explicit about what happens. Although that feels a bit redundant)

We don't know what application build pipelines exist out there and adding a dependency which is absolutely not installed on headless system runs the risk of breaking a lot of user setups.

In my opinion this is important when it comes to keeping this change a minor (1.1.0 instead of 2.0.0). And I'd feel very uncomfortable giving my OK to merging it otherwise.

rukai commented 6 years ago

Wow, I didn't expect the response to be so positive! :D Thanks for the feedback. I think I've addressed all the issues that have been brought up.

I've opted to use a gui feature to toggle the window on/off at compile time. I didn't want to enable it by default then toggle off because:

I have put no thought into the naming of the feature, feel free to bikeshed all you want, I dont really care what its called.

I've run out of time again for today, I'll have to look into rust-fmt tomorrow.

Edit: Oops just realized the cargo.toml is still always pulling in the x11+win deps will need to fix that tomorrow

spacekookie commented 6 years ago

Those changes look good!

Although did you know you can make an example depend a feature flag?

[[example]]
name = "window"
required-features = ["gui"]

This might be less overkill than adding a whole new test (which can't run on CI anyways because it's headless)

skade commented 6 years ago

I think this case is similar to #7. My comment here: https://github.com/rust-clique/human-panic/issues/7#issuecomment-382132348

You are currently in a panicing program, so running any large amount of code might lead to interesting problems. Anything leading to another panic with abort the program immediately. For example, a panic might crash a thread and the thread owner is set up to handle panics - and abort will interact with that handler.

I would recommend forwarding to an external program in all cases. This program could handle automated submission, display of a window, etc, and - that's the best feature - can panic by itself, a condition that human_panic can then handle and log.

rukai commented 6 years ago

I'm not sure what the issue with the abort is? The window display occurs after the regular functionality (dump to file + message to stderr). Is it because it wont free some sort of OS resource used in windowing? I don't really know much about this sort of thing.

The issue with an external program is it seems annoying to setup for the devs. Would the setup for that look like:

Also would using catch_unwind instead of set_hook avoid this issue?

rukai commented 6 years ago

Turns out the rustfmt issue was because I did cargo install rustfmt ages ago and forgot about it, which was overriding the modern rustfmt-preview component I was trying to use.

All feedback except skade's feedback has been addressed.

skade commented 6 years ago

I'm not sure what the issue with the abort is? The window display occurs after the regular functionality (dump to file + message to stderr). Is it because it wont free some sort of OS resource used in windowing? I don't really know much about this sort of thing.

The issue with abort is that the program immediately quits. No cleanup will happen and no further panic handling will take place, including the handling that the host program has set up. (including potential catch_panic).

The issue with an external program is it seems annoying to setup for the devs.

You can ship a reference implementation, that can be amended to the developers needs. If - for example - the form and function of your feature doesn't match the developers expectations, they'd have to fork human_panic instead, which I'd call more annoying. This is the reason why I've drawn a parallel to the automated submission feature, which has a similar orientation.

Would the setup for that look like

  • bat/sh script to run the real program, then the human-panic-follow-up-helper program
  • only call the human-panic-follow-up-helper program if the real program returns an error value
  • human-panic-helper program checks the tmp folder for the most recent panic dump

The rough setup for this is that the crashing program does nothing else but execute the error handling program with a defined set of parameters instead. It only has to be in path.

human-panic-helper --dump-file /tmp/1232313.dump

Also would using catch_unwind instead of set_hook avoid this issue?

Using catch_panic has wider implications, it breaks catching assumptions of clients and requires the user to wrap all code paths with it, more specifically, it will not catch panics that are caught before the human_panic handler runs.

killercup commented 6 years ago

First off, thank you @rukai for wanting to upstream your solution! Sorry for taking a week to write this comment, this totally slipped through the cracks.

I'm pretty surprised to see this as a feature for human-panic, to be honest. Not that I disagree with the idea per-se, but GUI programming is usually quite different from CLI apps (what this crate currently targets) and there are way more frameworks and differences between OSs to consider.

Another potential issue I can see is that people might expect this crate -- if it were to add OS-specific GUI features -- to also use the OS-specific error reporting. And there are a lot of implementations of that. But all this does right now is to write a log file (whose path is rather cryptic and hard to find by hand).

Thus, I agree with @skade that a pluggable interface might be more useful than the current solution.

rukai commented 6 years ago

What are your thoughts on if I instead submit a PR that checks for an external program in the same directory named something like human-panic-helper. If it exists run it, if it doesn't exist then close.

rukai commented 6 years ago

Thinking about this some more, could running an external program result in privilege escalation? i.e. if the program is run in a directory without a helper program, then if a user has write access to the directory the program is in but not the actual program. Then they can create the helper program and then their code will be executed on a panic? Or is write access such a strong requirement that this doesn't matter?

rukai commented 6 years ago

First off, thanks for all the time put into responding to this PR. I wouldn't have been able to get to this point without your feedback. I'm going to go ahead and close this PR as I believe both creating a window and running external programs is out of scope of human-panic.

Different projects will have different needs for this sort of thing. So I think once a project gets to the point of running external programs, the project is better off building their own panic handler.

In case someone stumbles upon this PR and wants to implement a gui panic handler or external handler for their project: