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

Load commands and gdblib explicitly in __init__.py #1243

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

gsingh93
Copy link
Member

@gsingh93 gsingh93 commented Oct 7, 2022

For historical reasons, the modules in pwndbg/commands and pwndbg/gdblib rely on executing code on import. This isn't great design, but we're stuck with it for now. At the very least, instead of importing a bunch of things in pwndbg/__init__.py that aren't actually used in that file, we import them in the specific __init__.py for that module inside a function, and then we just call that function to trigger the side effects of importing them.

The remaining modules in pwndbg/__init__.py may also be able to be removed in the future, but for now I'm just looking gdblib and commands.

Some motivations for this:

  • This may make it easier to add unit tests, as we can hopefully just patch these two functions out
  • It makes it clear in __init_.py what is actually required to be imported for the code in that file, as opposed to side effects
  • If you ever want to know which files have side effects, you can look at these two functions to get the full list
  • It's easier to figure out if an import is missing. For example, with this PR I added the pwndbg.gdblib.ctypes import, which was easy to spot with the reduced amount of imports.
  • It makes refactoring easier in the future, as we just need to move around two function calls instead of a ton of imports
  • This may make it easier to reload modules during development, but this is something I still need to investigate.

@@ -4,3 +4,18 @@
from pwndbg.gdblib.arch import arch

__all__ = ["ctypes", "memory", "typeinfo"]


# TODO: should the imports above be moved here?
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what the best way to deal with this is. The above imports are there so that we can rename pwndbg.gdblib.arch to arch_mod and export the pwndbg.gdblib.arch object as arch. Maybe the right way to do it is to make them globals, and then assign to those globals inside load_gdblib?

@gsingh93 gsingh93 marked this pull request as ready for review October 7, 2022 21:41
@gsingh93 gsingh93 merged commit 135ced5 into pwndbg:dev Oct 7, 2022
@gsingh93 gsingh93 deleted the load-modules branch October 7, 2022 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants