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

rukai · 2018-07-11T14:10:45Z

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:

Extending the scope to GUI applications.
Lots of unsafe code which is especially undesirable in a panic handler.

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

Checklist

tests pass
documentation is changed or added

Semver Changes

Major change

Screenshots

Linux (i3 wm)

Windows

…ror message window

rukai · 2018-07-11T14:17:13Z

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

spacekookie

Hey there and thank you very much for this PR!

It's not something I would have thought of as the application area for human-panic (especially because it was created for CLI applications 😉) but a great application nonetheless.

I've marked a few things in my review I'll want to see changed before I'd feel comfortable merging it though.

One thing I noticed while looking at the linux-x11 window code is that you spend a lot of it trying to split the strings in a good way. I'm wondering if there aren't crates to do this sort of work and what it would take to use them. Even if it means refactoring the rest of human-panic to use it too.

spacekookie · 2018-07-11T17:13:36Z

Cargo.toml

+
+[target.'cfg(target_os = "windows")'.dependencies]
+winapi = { version = "0.3", features = ["winuser"] }
+
 [features]


I would absolutely want to make either of these an optional dependency. We can't have CLI applications that run headless depend on X11. Adding a feature called gui (or something along those lines 🚲 🏚️ ...) will make the feature opt-in which is a much safer option here

spacekookie · 2018-07-11T17:15:48Z

README.md


 When an error eventually occurs, you probably will want to know about it. So
-instead of just providing an error message on the command line, we can create a
-call to action for people to submit a report.
+instead of just providing an error message on the command line or the GUI


I would re-phrase to

"[...] instead of crashing without notice or printing a confusing error message [...]"

Rust does by default print an error message but it's not super helpful. I've been meaning to change the wording here for a while anyways 😉

spacekookie · 2018-07-11T17:16:47Z

examples/window.rs

+    version: env!("CARGO_PKG_VERSION").into(),
+    authors: "ルカス".into(),
+    homepage: "https://github.com/rust-clique/human-panic/issues".into(),
+    create_window: true,


Also something I'm noticing: if we have an gui compile feature we don't need to pass this anymore. The feature becomes an opt-in at the dependency level which is much neater IMO.

spacekookie · 2018-07-11T17:17:06Z

src/lib.rs

 //!
 //! When an error eventually occurs, you probably will want to know about it. So
-//! instead of just providing an error message on the command line, we can create a
-//! call to action for people to submit a report.
+//! instead of just providing an error message on the command line or the GUI


Same change as in README

spacekookie · 2018-07-11T17:17:26Z

src/lib.rs

+//!
+//! ## GUI Applications
+//! GUI applications don't have a terminal to output errors to.
+//! The errors printed to the terminal can optionally be displayed in a message box window.


Same as above

What are you referring to here?

Oh nvm, I thought there had been another change in the README we needed to duplicate in the crate docs.

spacekookie · 2018-07-11T17:22:16Z

src/lib.rs

@@ -50,6 +56,19 @@ extern crate failure;
 extern crate serde_derive;
 extern crate termcolor;

+#[cfg(target_os = "windows")]
+#[path = "windows_window.rs"]
+pub mod window;


Please refactor these modules. There should be a gui or window module which contains win.rs and x11.rs. Gate the loading of window(or gui) via the feature mentioned above, then select win or x11 based on platform

yoshuawuyts · 2018-07-11T18:33:57Z

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 · 2018-07-11T18:46:38Z

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.

Feature Flags
Separate crate

Any others?

yoshuawuyts · 2018-07-11T19:56:19Z

@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 · 2018-07-11T20:05:31Z

@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 · 2018-07-12T12:32:28Z

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:

Most people are going to be making CLI apps in rust. (probably)
Enabling a feature to remove a feature is unintuitive.

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 · 2018-07-12T14:52:16Z

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 · 2018-07-13T09:19:50Z

I think this case is similar to #7. My comment here: #7 (comment)

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 · 2018-07-13T12:41:29Z

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:

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

Also would using catch_unwind instead of set_hook avoid this issue?

rukai · 2018-07-13T13:34:14Z

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 · 2018-07-16T11:49:34Z

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 · 2018-07-18T19:40:14Z

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 · 2018-07-18T22:54:19Z

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 · 2018-07-19T10:21:52Z

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 · 2018-07-26T13:56:57Z

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:

I have moved the message box code into a separate crate: https://github.com/rukai/simple-message-box
I have implemented an external panic reporting program for my game: set the hook to call external program and external program

rukai added 6 commits July 11, 2018 20:15

Add basic error message window for X11

f359cf0

Add basic error message window for windows

3316971

Update Documentation

fd53f85

Restore paragraphs in error message + implement text flow in linux er…

f8ca99e

…ror message window

rustfmt

987d961

Add example image

df1ed0d

spacekookie requested changes Jul 11, 2018

View reviewed changes

Put message box behind a gui feature

215fc5d

rukai force-pushed the native_window branch from 61249ce to 215fc5d Compare July 12, 2018 22:52

rukai force-pushed the native_window branch from 12045b7 to d9a440c Compare July 13, 2018 13:22

Fix lints

e7733f0

rukai force-pushed the native_window branch from d9a440c to e7733f0 Compare July 13, 2018 13:31

rukai closed this Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rukai commented Jul 11, 2018 •

edited

Loading

rukai commented Jul 11, 2018

spacekookie left a comment

spacekookie Jul 11, 2018

spacekookie Jul 11, 2018

spacekookie Jul 11, 2018

spacekookie Jul 11, 2018

spacekookie Jul 11, 2018

rukai Jul 12, 2018

spacekookie Jul 13, 2018

spacekookie Jul 11, 2018

yoshuawuyts commented Jul 11, 2018

epage commented Jul 11, 2018

yoshuawuyts commented Jul 11, 2018 •

edited

Loading

spacekookie commented Jul 11, 2018 •

edited

Loading

rukai commented Jul 12, 2018 •

edited

Loading

spacekookie commented Jul 12, 2018 •

edited

Loading

skade commented Jul 13, 2018

rukai commented Jul 13, 2018 •

edited

Loading

rukai commented Jul 13, 2018 •

edited

Loading

skade commented Jul 16, 2018

killercup commented Jul 18, 2018

rukai commented Jul 18, 2018

rukai commented Jul 19, 2018

rukai commented Jul 26, 2018

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

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

Conversation

rukai commented Jul 11, 2018 • edited Loading

Description

Scope

Checklist

Semver Changes

Screenshots

Linux (i3 wm)

Windows

rukai commented Jul 11, 2018

spacekookie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts commented Jul 11, 2018

epage commented Jul 11, 2018

yoshuawuyts commented Jul 11, 2018 • edited Loading

spacekookie commented Jul 11, 2018 • edited Loading

rukai commented Jul 12, 2018 • edited Loading

spacekookie commented Jul 12, 2018 • edited Loading

skade commented Jul 13, 2018

rukai commented Jul 13, 2018 • edited Loading

rukai commented Jul 13, 2018 • edited Loading

skade commented Jul 16, 2018

killercup commented Jul 18, 2018

rukai commented Jul 18, 2018

rukai commented Jul 19, 2018

rukai commented Jul 26, 2018

rukai commented Jul 11, 2018 •

edited

Loading

yoshuawuyts commented Jul 11, 2018 •

edited

Loading

spacekookie commented Jul 11, 2018 •

edited

Loading

rukai commented Jul 12, 2018 •

edited

Loading

spacekookie commented Jul 12, 2018 •

edited

Loading

rukai commented Jul 13, 2018 •

edited

Loading

rukai commented Jul 13, 2018 •

edited

Loading