-
Notifications
You must be signed in to change notification settings - Fork 152
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
Make Windows support more obvious #344
base: main
Are you sure you want to change the base?
Conversation
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.
This should probably mention both the dtrace and the blondie backends, and ideally make a recommendation which of the two is more useful/reliable.
Okay, I reworked the Readme a bit more, let me know what you think. I tried to imitate the readmes of other popular projects, like this one. Regarding the backends, is blondie the more reliable one? That was my best guess after reading the issue tracker. If yes, then I'll add the recommendation. |
Sorry, it's a little hard to review these changes since it's a whole bunch of stuff mashed together. Would prefer small commits that make one logical change at a time, which makes it easier to review, so I can verify the changes against the motivation. |
Okay, I'll rewrite the git history and let you know once I'm done. I was hoping that it's short enough to review in one go, but I suppose having a more granular changeset is helpful. |
Thanks -- sorry, I don't have a ton of time to devote to flamegraph so trying to make this efficient. |
Remove note that has been moved to quick start
@djc This took a while, but I finally got around to splitting it up into granular commits. This set of commits takes care of the following two goals
|
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.
Thanks, I have a few more nits but this mostly looks good. Please fix up the existing commits (I use rebase -i
for this).
#### DTrace on Windows | ||
|
||
Alternatively, one can [install DTrace on Windows](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/dtrace). If found, flamegraph will always prefer using `dtrace` over the built-in Windows support. | ||
If you try this out please let us know how it goes. :D |
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.
Please remove this last sentence.
wonderful [Inferno](https://github.com/jonhoo/inferno) all-rust flamegraph generation library! | ||
Windows is getting [dtrace support](https://techcommunity.microsoft.com/t5/Windows-Kernel-Internals/DTrace-on-Windows/ba-p/362902), | ||
so if you try this out please let us know how it goes. :D | ||
|
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.
Should not add these empty lines in this commit.
I read the flamegraph-rs readme, and couldn't quite tell if Windows is supported. This edits the Readme to make it more obvious.
Though, with this edit, it no longer pushes users to try out dtrace on Windows. Is that good or bad?