Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Release? #7

Closed
Jake-Shadle opened this issue Jun 26, 2020 · 9 comments
Closed

Release? #7

Jake-Shadle opened this issue Jun 26, 2020 · 9 comments
Labels
question Further information is requested

Comments

@Jake-Shadle
Copy link
Contributor

I don't know what more you want to do before doing another release, but would it be possible to make a new one? Or at least a pre-release if you don't want to make a real one?

I'd like to use this in our project, but we can't use git patches due to rust-lang/cargo#8258 and the multiple submodules that are hosted on google code, at least until a new version of cargo is released that has the bugfix in it.

@daxpedda daxpedda added the question Further information is requested label Jun 26, 2020
@daxpedda
Copy link
Owner

daxpedda commented Jun 26, 2020

There's only a couple of things left:

  • Implement sentry_envelope_get_event, which should be no problem.
  • Check/fix the documentation and doctests.
  • Do integration tests.
  • More examples.

And a couple of questions, best answered by you, because atm you are my only user 😄:

  • Is panicking when converting String to Value alright? Some alternatives:
    • Replacing null bytes with a space or something, like it was before.
    • Cut off the string at the null byte.
    • Leave it as it is.
  • How to handle panics in closures passed to FFI. This is a catastrophe atm, because this is undefined behaviour until RFC: 'C-unwind' ABI rust-lang/rfcs#2945 is done. In the meantime anyway, I'm still trying to catch it with panic::catch_unwind and than process::abort. Some alternative:
    • I could just do nothing instead of abort.
    • Considering that almost all of the closures passeed to FFI happen in Event::capture, I could introduce another Mutex and return the panic over there. Which would still leave the question open for set_logger, for which I can't do that, because it can happen at any given time.
  • If or when sentry-rust implements a version of Crashpad/Breakbad, maybe with sentry-contrib-native-sys, whats the point of the main crate here? Would you still use it?

I think thats it. For a timeline: there's either gonna be a release today or tomorrow, is that alright?

@Jake-Shadle
Copy link
Contributor Author

  • IMO panicing on String -> Value is fine since that is a problem in the user's code that should be fixed.
  • Yah, that one is tricky, I would probably just leave it as is for now and "recommend" users not have any code that could panic in those closures 😅
  • I would say if sentry-rust supported native crash handling via the sys crate, then yah, there would not be much point in the higher level crate, we are actually using both of them in tandem, this crate for all of the actual reporting of events/crashes, and the rust crate for eg gathering device information etc. The native crash handling (and correct stacktraces fix(debug-images): Use first PT_LOAD to obtain load address getsentry/sentry-rust#222) are the killer features of using this vs the current Rust crate, so if that one had those capabilities, we would probably just use it.

There's not a super rush on it, though I am going on vacation for 2 weeks but someone else on my team might pick this up in the mean time, as unfortunately, we do have quite a lot of C/C++ code that does crash at times. 😉

@daxpedda
Copy link
Owner

Alright, thanks!

@daxpedda
Copy link
Owner

This got delayed a bit, but should be done by tomorrow (hopefully), not much left.

@daxpedda
Copy link
Owner

Alright I'm done with all the unit and doc tests, reviewed everything, next is integration tests.

@daxpedda
Copy link
Owner

I built the groundwork for integration tests now. I'm gonna stop saying "tomorrow", clearly this doesn't work out.

@daxpedda
Copy link
Owner

daxpedda commented Jun 30, 2020

I must say it's costing me quiet some energy to debug why CI fails. But I think I figured it all out finally.

  1. Spike protection, I will just have to disable it.
  2. Crash when uploading timeout is hit getsentry/sentry-native#322, which is being fixed here: fix: Avoid assertion when hitting shutdown_timeout getsentry/sentry-native#323.
  3. Sentry can sometimes take ages (up to 10 min.) to process issues, until then they are unavailable.

All issues can be circumvented for now. In any case, I'm going to write the last couple of integration tests tomorrow, the rest should be peanuts.

@daxpedda
Copy link
Owner

daxpedda commented Jul 1, 2020

I have release on crates.io now: https://crates.io/crates/sentry-contrib-native/0.1.0-alpha-2.

For a non-alpha release I would like to:

  • Finish the integration tests.
  • Finish the main example.
  • Finish documenting start_session and end_session.

But since I started doing the integration tests I didn't find any bugs, so it's good to go.

@Jake-Shadle
Copy link
Contributor Author

Great, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants