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.

Sign up for GitHub

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-start-debugging-noexpand': expand :cwd #414

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

`dap-start-debugging-noexpand': expand :cwd #414

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4c3974
`dap-start-debugging-noexpand': expand :cwd
nbfalcon Nov 26, 2020
6d699ab
`dap-start-debugging-noexpand': expanded :cwd
nbfalcon Nov 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dap-chrome.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
(setq conf (-> conf
(plist-put :type "chrome")
(plist-put :dap-server-path dap-chrome-debug-program)
(dap--put-if-absent :cwd (expand-file-name default-directory))))
(dap--put-if-absent :cwd default-directory)))
(dap--plist-delete
(pcase (plist-get conf :mode)
("url" (-> conf
Expand Down
3 changes: 2 additions & 1 deletion dap-mode.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,7 +1590,8 @@ before starting the debug process."
:wait-for-port :type :request :port
:startup-function :environment-variables :hostName host) launch-args)
(session-name (dap--calculate-unique-name name (dap--get-sessions)))
(default-directory (or cwd default-directory))
(default-directory (or (and cwd (expand-file-name cwd))
Copy link
Member

Choose a reason for hiding this comment

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

default-directory should be expanded too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why that? From its doc: "it should be an absolute directory name". Also, against what would default-directory be expanded, given that we omit it here? The doc of expand-file-name says that its argument is expanded against default-directory unless the second argument is specified.

All this would achieve is that "." and ".." paths get eliminated. This way, we don't change default-directory at all unless :cwd is specified.

Copy link
Member Author

@nbfalcon nbfalcon Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Your fix for dap-chrome would still behave exactly the same way, because it specifies a default-directory :cwd that is then (needlessly) expanded against default-directory.

Copy link
Member

Choose a reason for hiding this comment

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

For the same reason we expand default-directory in dap-chrome.el - default directory could be ~/foo/bar

Copy link
Member

Choose a reason for hiding this comment

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

Also, the PR is introducing regression - we will send ~/foo/bar to the adapter since :cwd won't be expanded.

Copy link
Member Author

@nbfalcon nbfalcon Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to expand-file-name default directory to launch the adapter:

(let ((default-directory "~/Downloads"))
  (shell-command "pwd")) ;; => `message`s "/home/nikita/Downloads" on my system

I think there are five options here:

  • expand-file-name in dap-chrome
  • always send an expanded :cwd, even if unspecified
  • send an expanded :cwd only if specified
  • Either of the above, but also expand default-directory (against itself)

:cwd would be expanded in all cases. I ended up going with the last one, always sending an expanded :cwd and always expanding default-directory.

Copy link
Member

Choose a reason for hiding this comment

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

The cwd should be set in the template otherwise commands like restart session won't work in the expected manner when cwd is relative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why does dap-start-debugging-noexpand mutate LAUNCH-ARGS with plist-put :name?

Copy link
Member

Choose a reason for hiding this comment

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

What is the problem caused by that?

Copy link
Member

Choose a reason for hiding this comment

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

Why does dap-start-debugging-noexpand mutate LAUNCH-ARGS with plist-put :name?

Modifying the template should be fine and expected since we are copying it.

default-directory))
(process-environment (if environment-variables
(cl-copy-list process-environment)
process-environment))
Expand Down