Resolve dune >= 3.0 sources by mapping /workspace_root - #94
Open
tmcgilchrist wants to merge 3 commits into
Open
Conversation
Add four scenarios on top of the existing scopes/frames tests: - test_stepping drives next/stepIn/stepOut and records where the debuggee lands, using stack depth as the stable signal so the test does not depend on exactly which line each step snaps to. - test_values renders one local per shape of value (ints, floats, chars, strings, bools, unit, boxed ints, lists, arrays, tuples, options, records, variants, closures, lazies) and expands the structured ones a level. The fixture references every local so none is dropped as unused. - test_heap stops inside a closure and reads the Heap scope, i.e. the variables captured from the enclosing scope. This exercises Value_scope.iter_compenv_heap, which is version-conditional in the same way the Globals scope was in hackwaly#74. - test_events exercises Code_module.find_event/find_events directly, without a debuggee, and records how each source line maps to a debug event. It documents the current mapping, including the lines that are not breakpointable and the backward snap noted in KNOWN_ISSUES.md. The dap_client helper gains stepping commands and value expansion that fetches named and indexed children separately, with a filter, the way an editor does. debugger.ml re-exports the symbol-table modules so the event-mapping test can reach them.
The variables request handler asserted that a value had no indexed children when the request omitted the filter field, and raised Assert_failure for an array. The DAP spec says an omitted filter returns both named and indexed children, so this is a valid request.
Since dune 3.0, map_workspace_root is on by default and rewrites the build directory in a bytecode's debug info to a fixed "/workspace_root", which does not exist on disk. The adapter could not find a module's source there, so it was never registered, and since breakpoints are matched to a module by the source file's digest, no breakpoint in a dune-built module could bind. Derive the real directories "/workspace_root" stands for from the executable's path (the source root and the build context, which both hold the sources), and rewrite the recorded search dirs with them before resolving. Paths without the prefix, and executables not under a _build directory, are left unchanged, so non-dune builds are unaffected. Add test_workspace (unit test of the path logic) and test_dune_source (an integration test that reproduces dune's /workspace_root debug info with BUILD_PATH_PREFIX_MAP and checks a breakpoint binds). Fixes hackwaly#58, hackwaly#11, hackwaly#20, hackwaly#47, hackwaly#57
sim642
reviewed
Jul 25, 2026
Comment on lines
+35
to
+81
| let remap_dir workspace_dirs dir = | ||
| let n = String.length workspace_root_prefix in | ||
| let has_prefix = | ||
| String.length dir >= n | ||
| && String.sub dir 0 n = workspace_root_prefix | ||
| && (String.length dir = n || dir.[n] = '/') | ||
| in | ||
| if has_prefix && workspace_dirs <> [] then | ||
| let suffix = String.sub dir n (String.length dir - n) in | ||
| List.map (fun root -> root ^ suffix) workspace_dirs | ||
| else [ dir ] | ||
|
|
||
| (* The real directories that dune's "/workspace_root" stands for, derived from | ||
| the executable's path. A dune executable lives at | ||
| <root>/_build/<context>/<...>, and dune mirrors the source tree under the | ||
| build context, so both the source root <root> and the build context | ||
| <root>/_build/<context> hold the sources (byte-identical copies). The source | ||
| root is listed first so a resolved source is the user's own file rather than | ||
| the build copy. Returns [] when the path is not under a "_build" directory, | ||
| in which case no rewriting happens. *) | ||
| let derive_workspace_dirs executable = | ||
| let executable = | ||
| if Filename.is_relative executable then | ||
| Filename.concat (Sys.getcwd ()) executable | ||
| else executable | ||
| in | ||
| let marker = "/_build/" in | ||
| let marker_len = String.length marker in | ||
| let len = String.length executable in | ||
| let rec find i = | ||
| if i + marker_len > len then None | ||
| else if String.sub executable i marker_len = marker then Some i | ||
| else find (i + 1) | ||
| in | ||
| match find 0 with | ||
| | None -> [] | ||
| | Some i -> | ||
| let source_root = String.sub executable 0 i in | ||
| let after = | ||
| String.sub executable (i + marker_len) (len - i - marker_len) | ||
| in | ||
| let context = | ||
| match String.index_opt after '/' with | ||
| | Some j -> String.sub after 0 j | ||
| | None -> after | ||
| in | ||
| [ source_root; source_root ^ marker ^ context ] |
Collaborator
There was a problem hiding this comment.
This all seems so ad hoc, reimplementing various path operations on strings directly. Doesn't the standard Filename suffice?
If not, then maybe depending on Fpath wouldn't be a bad idea because this is quite difficult to follow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since dune 3.0, map_workspace_root is on by default and rewrites the build directory in a bytecode's debug info to a fixed /workspace_root, which doesn't exist on disk. The adapter couldn't find a module's source there, so it was never registered — and since breakpoints are matched to a module by the source file's digest, no breakpoint in a dune-built module could bind.
Derive the real directories /workspace_root stands for from the executable's path (the source root and the build context, which both hold the sources) and rewrite the recorded search dirs before resolving. Paths without the prefix, and executables not under a _build directory, are left unchanged, so non-dune builds are unaffected.
Adds test_workspace (unit test of the path logic) and test_dune_source (an integration test that reproduces dune's /workspace_root debug info with BUILD_PATH_PREFIX_MAP and checks a breakpoint binds).
Fixes #58, #11. Removes the need for the (map_workspace_root false) workaround previously required for #20, #47, #57.
This builds on the integration tests from #93. This deserves some extended testing as I'm primarily using Emacs to validate this fix.