-
Notifications
You must be signed in to change notification settings - Fork 66
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
Enable Python and Ruby Unwinding by default #2281
Conversation
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
a4e5157
to
c9a7d7f
Compare
@@ -177,6 +174,12 @@ Flags: | |||
object files from disk. It keeps FDs open, | |||
so it should be kept in sync with ulimits. | |||
0 means no limit. | |||
--dwarf-unwinding-disable Do not unwind using .eh_frame information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Perhaps we should make all this "positive", swapping them all to "[whatever]-enable" so it's clear when we want to disable it, rather than having "--thing-disable=false" which might be harder to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used "disable" because of our existing flags and since it's enable by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
EnablePythonUnwinding bool `default:"false" help:"Enable Python unwinding." hidden:""` | ||
EnableRubyUnwinding bool `default:"false" help:"Enable Ruby unwinding." hidden:""` | ||
EnablePythonUnwinding bool `default:"false" help:"[deprecated] Python unwinder enabled by default. Use --python-unwinding-disable to disable it." hidden:""` | ||
EnableRubyUnwinding bool `default:"false" help:"[deprecated] Ruby unwinder enabled by default. Use --ruby-unwinding-disable to disable it." hidden:""` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy it you prefer to remove it now and add it to the release notes as a backwards incompatible change, your call!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add it to the changelog and we can remove it in subsequent releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely up to you!
Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com
depends: