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

[DAP] Prioritize information from module_info when available #1421

Merged
merged 4 commits into from
Feb 21, 2023

Conversation

robertoaloi
Copy link
Member

Description

Certain build systems (most notoriously Buck2) store build artifacts (including a copy of the source files) in custom folders (e.g. buck-out in Buck2). This can lead to an incorrect path to be returned by the debugger to the client whenever a breakpoint triggers. In turn, this result in the editor opening a buffer from the build system custom folder rather than the original source file and confusion among the users.

This PR changes the logic how the location of source files is identified. The current logic is:

  1. Ask the interpreter first (can return an incorrect path)
  2. If not found, ask the code server
  3. Only as a last resort, try reading the compile info from module_info.

The module_info is supposed to contain the most accurate information, but - in the case of non hermetic builds - it can contain a path to a non-existing file (e.g. if the code was built on a machine and ported to a different one). That's the main reason the current logic looks like the above (see this PR for context).

Instead:

  1. Check whether the compile info from module_info points to a regular file
  2. If it does, use it (eventually converting it to an absolute path)
  3. If it does not, ask the interpreter
  4. If not found, ask the code server

The logic has been refactored into separate functions to ease reading. Debug logs have also been added to simplify eventual troubleshooting.

Copy link
Contributor

@TheGeorge TheGeorge left a comment

Choose a reason for hiding this comment

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

Looks good, there is just a nit in one of the comments

{ok, Joined}
end;
false ->
%% Build is not hermetic
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be hermetic, e.g. with +deterministic you get only the base name and is_regular will fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I tried the +determinsitc option on OTP 25 I didn't get a source entry at all, so that case would be handled in the undefined clause above. But I can remove the comment.

@robertoaloi robertoaloi merged commit 229175e into main Feb 21, 2023
@robertoaloi robertoaloi deleted the dap-debugger-prioritize-module-info branch February 21, 2023 09:59
zengjinc pushed a commit to zengjinc/erlang_ls that referenced this pull request Jun 12, 2023
…ls#1421)

* [DAP] Cleanup logic around source path extraction

* [DAP] Prefer source from module_info/compile if available

* [DAP] Add logging, prefer module_info if available

* [DAP] Remove un-necessary (and maybe misleading) comment
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