Extend the pex tool to support debuggers #2141

tiagovtristao · 2021-10-27T15:59:36Z

This PR mainly extends the pex tool to support debuggers. Some refactoring, including files being moved around, was done for better clarity.

tiagovtristao · 2021-10-27T16:02:04Z

rules/go_rules.build_defs

    port = CONFIG.DEBUG.PORT
-    if not port:
-        # A CLI debug session needs access to the source files for improved UX.


This is actually also required for IDEs connecting to the debugger, since it needs access to generated artefacts and toolchain sources that don't appear on the original code.

tiagovtristao · 2021-10-27T16:03:12Z

src/core/build_env.go

@@ -175,8 +175,8 @@ func TestEnvironment(state *BuildState, target *BuildTarget, testDir string) Bui
 	if target.HasLabel("cc") {
 		env = append(env, "GCNO_DIR="+path.Join(RepoRoot, GenDir, target.Label.PackageName))
 	}
-	if state.DebugTests {
-		env = append(env, "DEBUG=true")
+	if state.DebugFailingTests {


This was renamed to avoid any confusion with the more general case of debugging via plz debug

tiagovtristao · 2021-10-27T16:04:10Z

src/please.go

-		TestResultsFile cli.Filepath `long:"test_results_file" default:"plz-out/log/test_results.xml" description:"File to write combined test results to."`
-		SurefireDir     cli.Filepath `long:"surefire_dir" default:"plz-out/surefire-reports" description:"Directory to copy XML test results to."`
-		ShowOutput      bool         `short:"s" long:"show_output" description:"Always show output of tests, even on success."`
-		Debug           bool         `short:"d" long:"debug" description:"Allows starting an interactive debugger on test failure. Does not work with all test types (currently only python/pytest, C and C++). Implies -c dbg unless otherwise set."`


Support for debugging failed C/C++ tests seems to have disappeared since the original commit that introduced it

tiagovtristao · 2021-10-27T16:05:33Z

tools/please_pex/pex/pex_run.py

 if __name__ == '__main__':
-    if 'PEX_PROFILE_FILENAME' in os.environ:
+    # If PEX_INTERPRETER is set, then it starts an interactive console.
+    if os.environ.get('PEX_INTERPRETER', '0') != '0':
+        import code
+        resutl = code.interact()
+    # If PEX_PROFILE_FILENAME is set, then it collects profile information into the filename.
+    elif os.environ.get('PEX_PROFILE_FILENAME'):
        with profile(os.environ['PEX_PROFILE_FILENAME'])():
            result = run()
    else:
        result = run()
+


Consolidate different execution paths (i.e. based on external factors such as env vars) here.

peterebden · 2021-10-27T16:26:10Z

rules/python_rules.build_defs

+    # `DEBUG_PORT` is read by the pex generated for debuggers that support server mode.
+    cmd = f"{bin}" if not port else f"DEBUG_PORT={port} {bin}"
+
+    cmd = f"{cmd} {flags}"


nit: would probably add this to above rather than creating another string (since it is fairly simple)

Tatskaari

Looks good to me! Some of this will need to be migrated over to the plugin though i.e. the build def and pex tool changes.

Tatskaari · 2021-11-01T11:49:26Z

src/please.go

@@ -158,7 +158,7 @@ var opts struct {
 		CoverageXMLReport   cli.Filepath  `long:"coverage_xml_report" default:"plz-out/log/coverage.xml" description:"XML File to write combined coverage results to."`
 		Incremental         bool          `short:"i" long:"incremental" description:"Calculates summary statistics for incremental coverage, i.e. stats for just the lines currently modified."`
 		ShowOutput          bool          `short:"s" long:"show_output" description:"Always show output of tests, even on success."`
-		Debug               bool          `short:"d" long:"debug" description:"Allows starting an interactive debugger on test failure. Does not work with all test types (currently only python/pytest, C and C++). Implies -c dbg unless otherwise set."`
+		DebugFailingTest    bool          `short:"d" long:"debug" description:"Allows starting an interactive debugger on test failure. Does not work with all test types (currently only python/pytest). Implies -c dbg unless otherwise set."`


Oh this is cool!

Tatskaari · 2021-11-01T13:38:31Z

rules/python_rules.build_defs

@@ -103,6 +103,9 @@ def python_binary(name:str, main:str, srcs:list=[], resources:list=[], out:str=N
    by the dependent python_library rules, and this rule simply builds the
    metadata for it and concatenates them all together.

+    This target can be debugged via:
+        plz debug [--debugger=[gdb|debugpy]] [--port] //:target


Do you mean pdb?

Tatskaari · 2021-11-01T13:39:04Z

rules/python_rules.build_defs

+    debug_cmd, debugger = _debug_cmd("./$OUT")
+    if debug_cmd:
+        if debugger == "debugpy":
+            cmd = cmd.replace(' --zip_safe', '')


qq: why is this needed?

debugpy was freaking out during some file access of some sort IIRC

Tatskaari · 2021-11-01T13:39:15Z

rules/python_rules.build_defs

@@ -205,6 +215,9 @@ def python_test(name:str, srcs:list, data:list|dict=[], resources:list=[], deps:
    on which is set for the test runner, which can be configured either via the python_test_runner
    package property or python.testrunner in the config.

+    This target can be debugged via:
+        plz debug [--debugger=[gdb|debugpy]] [--port] //:target


Extend the pex tool to support debuggers

15a6d4b

tiagovtristao commented Oct 27, 2021

View reviewed changes

peterebden approved these changes Oct 27, 2021

View reviewed changes

Tatskaari approved these changes Nov 1, 2021

View reviewed changes

tiagovtristao added 2 commits November 2, 2021 14:04

Merge remote-tracking branch 'upstream/master' into python-debug

71354b1

Address comments

444289c

tiagovtristao merged commit 4176a6c into thought-machine:master Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the pex tool to support debuggers #2141

Extend the pex tool to support debuggers #2141

tiagovtristao commented Oct 27, 2021

tiagovtristao Oct 27, 2021

tiagovtristao Oct 27, 2021

tiagovtristao Oct 27, 2021

tiagovtristao Oct 27, 2021

peterebden Oct 27, 2021

Tatskaari left a comment •

edited

Tatskaari Nov 1, 2021

Tatskaari Nov 1, 2021

tiagovtristao Nov 2, 2021

Tatskaari Nov 1, 2021

tiagovtristao Nov 2, 2021 •

edited

Tatskaari Nov 1, 2021

Extend the pex tool to support debuggers #2141

Extend the pex tool to support debuggers #2141

Conversation

tiagovtristao commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tatskaari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagovtristao Nov 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tatskaari left a comment •

edited

tiagovtristao Nov 2, 2021 •

edited