Skip to content

Conversation

@jmorse
Copy link
Member

@jmorse jmorse commented Feb 24, 2025

AFAIUI the original rationale behind having tools abstracted behind the directory that they live is was to ease development -- create a new tool by creating a new directory. However it also puts in awkward abstractions whereby you can't know what's going to be run without first loading a module. There's also effectively no authoritative list of tools, because you can load one from anywhere. And the tool names are affected by pythons normalisation of module names!

Instead: move the tools into named files in the tools directory, have a list of them, and create the tool object from that list when needed. No module wrangling requried at all, and there can be certainty about what's going on. We can also delete some name-normalisation that happens now that it's not subject to module-loading-normalisation.

AFAIUI the original rationale behind having tools abstracted behind the
directory that they live is was to ease development -- create a new tool by
creating a new directory. However it also puts in awkward abstractions
whereby you can't know what's going to be run without first loading a
module. There's also effectively no authoritative list of tools, because
you can load one from anywhere. And the tool names are affected by pythons
normalisation of module names!

Instead: move the tools into named files in the tools directory, have a
list of them, and create the tool object from that list when needed. No
module wrangling requried at all, and there can be certainty about what's
going on. We can also delete some name-normalisation that happens now that
it's not subject to module-loading-normalisation.
@jmorse jmorse requested review from OCHyams and SLTozer February 24, 2025 18:17
@github-actions
Copy link

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r 6de5d1e46d1812de2bbbbe8d8d2c811e4d16acbe...dee8f47272327c04230a59e5e125180d901146e7 cross-project-tests/debuginfo-tests/dexter/dex/debugger/Debuggers.py cross-project-tests/debuginfo-tests/dexter/dex/tools/Main.py cross-project-tests/debuginfo-tests/dexter/dex/tools/__init__.py cross-project-tests/debuginfo-tests/dexter/dex/tools/help.py cross-project-tests/debuginfo-tests/dexter/dex/tools/list_debuggers.py cross-project-tests/debuginfo-tests/dexter/dex/tools/no_tool_.py cross-project-tests/debuginfo-tests/dexter/dex/tools/run_debugger_internal_.py cross-project-tests/debuginfo-tests/dexter/dex/tools/test.py cross-project-tests/debuginfo-tests/dexter/dex/tools/view.py
View the diff from darker here.
--- tools/__init__.py	2025-02-24 18:10:32.000000 +0000
+++ tools/__init__.py	2025-02-24 18:20:23.010541 +0000
@@ -7,13 +7,22 @@
 
 from dex.tools.Main import Context, main, tool_main
 from dex.tools.TestToolBase import TestToolBase
 from dex.tools.ToolBase import ToolBase
 
+
 def get_tool_names():
     """Returns a list of expected DExTer Tools"""
-    return ["help", "list-debuggers", "no-tool-", "run-debugger-internal-", "test", "view"]
+    return [
+        "help",
+        "list-debuggers",
+        "no-tool-",
+        "run-debugger-internal-",
+        "test",
+        "view",
+    ]
+
 
 def get_tools():
     """Returns a dictionary of expected DExTer Tools"""
     return _the_tools
 
@@ -24,13 +33,12 @@
 from .run_debugger_internal_ import Tool as run_debugger_internal_tool
 from .test import Tool as test_tool
 from .view import Tool as view_tool
 
 _the_tools = {
-      "help" : help_tool,
-      "list-debuggers" : list_debuggers_tool,
-      "no_tool_" : no_tool_tool,
-      "run-debugger-internal-" : run_debugger_internal_tool,
-      "test" : test_tool,
-      "view" : view_tool
+    "help": help_tool,
+    "list-debuggers": list_debuggers_tool,
+    "no_tool_": no_tool_tool,
+    "run-debugger-internal-": run_debugger_internal_tool,
+    "test": test_tool,
+    "view": view_tool,
 }
-
--- tools/test.py	2025-02-24 18:10:32.000000 +0000
+++ tools/test.py	2025-02-24 18:20:23.152053 +0000
@@ -216,13 +216,18 @@
         """Attempt to run test files specified in options.source_files. Store
         result internally in self._test_cases.
         """
         try:
             if self.context.options.binary:
-                if platform.system() == 'Darwin' and os.path.exists(self.context.options.binary + '.dSYM'):
+                if platform.system() == "Darwin" and os.path.exists(
+                    self.context.options.binary + ".dSYM"
+                ):
                     # On Darwin, the debug info is in the .dSYM which might not be found by lldb, copy it into the tmp working directory
-                    shutil.copytree(self.context.options.binary + '.dSYM', self.context.options.executable + '.dSYM')
+                    shutil.copytree(
+                        self.context.options.binary + ".dSYM",
+                        self.context.options.executable + ".dSYM",
+                    )
                 # Copy user's binary into the tmp working directory.
                 shutil.copy(
                     self.context.options.binary, self.context.options.executable
                 )
             steps = self._get_steps()

Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

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

SGTM, hurray for simplifying (and deleting) things. Got a couple of inline nits though.

_the_tools = {
"help" : help_tool,
"list-debuggers" : list_debuggers_tool,
"no_tool_" : no_tool_tool,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no-tool- (hyphens instead of underscores) for consistency?

Note this is currently out of sync with the get_tool_names spelling. Perhaps get_tool_names should return list(_the_tools.keys())?


def _warn_meaningless_option(context, option):
if hasattr(context.options, "list_debuggers"):
if hasattr(context.options, "list-debuggers"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have thought list-debuggers isn't a valid variable name, I'm confused how this works?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants