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

Improve error reportability #160

Merged
merged 7 commits into from
Aug 19, 2024
Merged

Conversation

jannis-baum
Copy link
Owner

Close #159

@jannis-baum jannis-baum marked this pull request as draft August 6, 2024 05:59
@jannis-baum jannis-baum force-pushed the issue/159-improve-error-reportability branch from e27529f to ac888d5 Compare August 6, 2024 06:02
@jannis-baum jannis-baum marked this pull request as ready for review August 9, 2024 21:33
@jannis-baum
Copy link
Owner Author

jannis-baum commented Aug 9, 2024

Hello @tuurep @Tweekism could one of you take a look at this and leave a review? Thanks! I got motivated to do this after we had quite a lot of back and forth on a bug the other day, I think this should make these things easier in the future.

EDIT the referenced issue has a list of the things I did here :)

@tuurep
Copy link
Collaborator

tuurep commented Aug 9, 2024

Whenever I write issues I quite hate these types of templates but that's a personal problem lol

I do see the need but am also thinking we could have a Troubleshooting Readme section

I read all of the template text, and it's good, with 2 worries:

  • Is it too verbose
  • One of my biggest traps, forgetting to kill the last process (i.e. killall vivify-server) might be worth a mention somewhere, but could go under some Troubleshooting doc as well?

@tuurep
Copy link
Collaborator

tuurep commented Aug 9, 2024

Do you have an example case where --version would hang before, and this fixes? :)

@jannis-baum
Copy link
Owner Author

Whenever I write issues I quite hate these types of templates but that's a personal problem lol

Haha yes so do I but I think I got to the point now where I really understand why people have them. Everything in that template are things that I/we would have to individually ask little by little otherwise and this makes the process of figuring out how to reproduce the bug very lengthy and annoying. I'd prefer to just fix it quickly without having to go through a huge conversation ^^

I do see the need but am also thinking we could have a Troubleshooting Readme section

Yes, this is something we could add in addition. What would you include there?

I still think we need the information in the bug template anyways though because people won't read everything before opening a bug

Is it too verbose

I tried my best to keep it as short as possible. If you have an idea for how to shorten it without losing information/understandability I'd be happy to improve it based on that!

One of my biggest traps, forgetting to kill the last process (i.e. killall vivify-server) might be worth a mention somewhere, but could go under some Troubleshooting doc as well?

Good point! I think we could add it to the start of the section where we ask to run vivify-server from the absolute path. What do you think?

Do you have an example case where --version would hang before, and this fixes? :)

You can set VIV_PORT to something invalid like VIV_PORT=hehe viv .. That made the server crash before which led to not printing STARTUP COMPLETE, which led to viv hanging indefinitely. Maybe there are other ways of making it crash as well. I hope I am catching everything that can go wrong now

@tuurep
Copy link
Collaborator

tuurep commented Aug 10, 2024

Yeah also it starts making more sense the more popular a repo is

I didn't realize that CONTRIBUTING.md already has a troubleshooting section that includes perhaps the most important one, Node SEA build issue,

and then the others are the ones you have in the template:

  • AUR/Brew package vs. compiled: which is running?
  • Running with a clean/minimal config
    • (Implied by the last section in the template)

Sooo yeah

I think we could add it to the start of the section where we ask to run vivify-server from the absolute path. What do you think?

This is the best idea here that I think we should do!

My thought was to add a README section until I saw the CONTRIBUTING also has it. Maybe worth extending that with all of the above?

If you have an idea for how to shorten it without losing information/understandability

I think it's a little wordy, might I say a little too polite if you know what I mean 😄

But it's quite fine, we don't need to add anything other than the tip about vivify-server unless you wanna.

You can set VIV_PORT to something invalid like VIV_PORT=hehe viv .

Ok, nice change! Tested a little and yes, on 0.3.1 it hangs and on this it gives an error where I can figure out what went wrong, even.

You might be aware, but in the case of the Linux build where it segfaults, it still hangs with e.g. viv . (only able to tell it segfaults running vivify-server directly). Wonder if it's possible to have viv exit in that case?

@jannis-baum jannis-baum force-pushed the issue/159-improve-error-reportability branch from d45a11b to 08eb505 Compare August 18, 2024 09:08
@jannis-baum
Copy link
Owner Author

So I added the tip about killall vivify-server now and I added some more code to viv to catch (hopefully) any type of crash, including the segfault. Could you test this?

My thought was to add a README section until I saw the CONTRIBUTING also has it. Maybe worth extending that with all of the above?

This is (as far as I can tell) the only thing still missing from the stuff you commented. I'll do this next but I'm out of time for today. Let me know if I missed anything else :)

@tuurep
Copy link
Collaborator

tuurep commented Aug 18, 2024

Ok, nice it works

When building purposely with system node and running viv .:

$ viv .
Fatal: vivify-server crashed. Please use the link below to submit a bug report.

https://github.com/jannis-baum/Vivify/issues/new?labels=type%3Abug&template=bug-report.md
/home/tuure/.local/bin/viv: line 63: 62404 Segmentation fault      (core dumped) nohup vivify-server $@ > "$output" 2> /dev/null

Bug report link interesting idea. Hopefully we don't get 100 bug reports for a known issue 😄 But the idea is that the template may point to the correct info.

I would add one newline after the link though.

@tuurep
Copy link
Collaborator

tuurep commented Aug 18, 2024

Hey since we're pretty much finished for the time being, I'll say that next I'm most interested in #146 but I don't know if it's a complicated one.

@jannis-baum
Copy link
Owner Author

Ok, nice it works

When building purposely with system node and running viv .:

$ viv .
Fatal: vivify-server crashed. Please use the link below to submit a bug report.

https://github.com/jannis-baum/Vivify/issues/new?labels=type%3Abug&template=bug-report.md
/home/tuure/.local/bin/viv: line 63: 62404 Segmentation fault      (core dumped) nohup vivify-server $@ > "$output" 2> /dev/null

Oh, very interesting that you get the segmentation fault info now as well! That wasn't the case before, was it?

Bug report link interesting idea. Hopefully we don't get 100 bug reports for a known issue 😄 But the idea is that the template may point to the correct info.

Haha, yes. I hope people will read the stuff on the bug report. I added some more info to that message, what do you think?

I would add one newline after the link though.

I added the newline, good call.

@jannis-baum
Copy link
Owner Author

Hey since we're pretty much finished for the time being, I'll say that next I'm most interested in #146 but I don't know if it's a complicated one.

Sounds great!

@tuurep
Copy link
Collaborator

tuurep commented Aug 19, 2024

Oh, very interesting that you get the segmentation fault info now as well! That wasn't the case before, was it?

Yeah it would just hang and I couldn't get that info from it

Copy link
Collaborator

@tuurep tuurep left a comment

Choose a reason for hiding this comment

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

Okay I think we can merge 😄

@jannis-baum jannis-baum merged commit 1120f1a into main Aug 19, 2024
6 checks passed
@jannis-baum jannis-baum deleted the issue/159-improve-error-reportability branch August 19, 2024 19:33
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.

Improve error reportability
2 participants