-
Notifications
You must be signed in to change notification settings - Fork 101
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
Increase code sharing between py-spy and rbspy #350
Comments
I think sharing more code is a great idea - and is something I've been meaning to do for a while but haven't had time. I'm wondering if we want to merge all of remoteprocess into this spytools crate ? I think having one more fully fleshed out crate will make life a little easier for us - |
That sounds good to me. I think my first refactor attempt will be pretty rough (not merge-worthy), but it should give us a sense of what's possible. I'll ping you here when that's done so that we can think through next steps. |
WIP branch here: https://github.com/acj/rbspy/tree/spytools-crate So far, I've moved PythonProcessInfo (now ProcessInfo) and BinaryParser into the new crate (just a subdirectory right now), and they work pretty well. I was able to delete a lot of scruffy address lookup code. The main compatibility snag is that there's a list of symbols (get_windows_symbols), a handful of regexes (is_lib), and a hard coded name or two (is_framework) that we'll need to parameterize somehow if py-spy and rbspy are going to share the code. To keep it simple for the calling code, I was thinking about defining a trait in the spytools crate that has a method for each bit of info that varies between python and ruby, and then adding a generic parameter to ProcessInfo::new that implements that trait. Not sure whether the trait implementations should live in spytools or in the respective spy repos. If you know of a cleaner way, or if that seems like a bad idea, let me know. Once the dust settles on ProcessInfo, I'll take a look at merging remoteprocess into the crate, and then I'll put up a PR for feedback. |
I've been thinking about refactoring the
initialize
andaddress_finder
code to make it easier to test, fully migrate to goblin where we can (#50), etc. py-spy has a cleaner and more mature solution to the problems that those modules solve. Maybe we could share more code?There was an earlier effort in this direction (#180) that seemed healthy for both projects.
Things that seem straightforward to share:
And maybe also:
The first group falls under a kind of "process info discovery" umbrella that doesn't vary much between the projects. We need to find different symbols for ruby vs. python, but the code for finding them—and dealing with any OS quirks—could probably be the same. My plan has been to build a
RubySpy
struct (based onPythonSpy
) and see what happens. I'm hoping to get a first version of that working this week. Much of the work has effectively beens/python/ruby
, which seems promising.Does factoring out a
spytools
crate still feel like a good move? Should they be a set of smaller, more focused crates instead? Could these responsibilities be moved from py-spy into remoteprocess? (I'm open to suggestions!)cc @benfred
The text was updated successfully, but these errors were encountered: