svaante / dape

Debug Adapter Protocol for Emacs
GNU General Public License v3.0
477 stars 31 forks source link

Initial support for Java debugging #22

Closed MagielBruntink closed 10 months ago

MagielBruntink commented 11 months ago

Following the discussion on #19 I created an extension/companion dape-jdtls.el for Java debugging support. Instructions have been added to the README.

MagielBruntink commented 11 months ago

@svaante I saw your comments in the other PR regarding Elpa submission and FSF contributor sign offs. I would have to discuss such with my employer. An easier and faster route is probably keeping dape-jdtls in a separate repo, maintained by me. Perhaps it will go on Melpa, which doesn't have the FSF reqruirwment. What do you say?

svaante commented 11 months ago

First off nice work đź‘Ť

As of now I have not decided what makes most sense regarding battery inclusions. I don't belive that dap-mode way of doing things is the way forward for Dape (having multipe files and the user needs to require them).

I am leaning more and more towards having base configs included. There are some things that needs to be solved before that can happen.

  1. How to verify that the adapter it installed. Which is not trivial, if we take java-debug we can't really assume that its available just because jdtls is in $PATH.
  2. Freeze the dape configuration format, I think there are some things that can be done better and make the config format more concise, I have left it as good enough but there are a few things I want to change (automatic port resolve, fn implementation, simplify usage with 'command and maybe find a way of shortening absolut paths).
  3. Define a way for the user to define configurations based on the default ones (in progress).
    
    ;;; Directory Local Variables            -*- no-byte-compile: t -*-

((java-mode . ((dape-local-configs (jdtls-for-my-project (jdtls :env (:PROJECT_ENV "SOME_VAL")))))))


Then `jdtls-for-my-project` will be available as a minibuffer completion when calling `dape`

Regarding the PR in I am uncertain about the implementation of `fn` in bf91567 as `:mainClass`, `:projectName` and `:classPaths` are not present in `dape-history` after running `jdtls`. Its maybe better if `fn` is resolved inside of `dape--read-config` instead. What are your thoughts?

I still think that the `dape--read-config` is nice to change values in the original configuration and with the current implementation of `Dape`s `fn` keyword that is not possible.
MagielBruntink commented 11 months ago

As you say somewhere in your README, these DAP adapters and servers are definitely messy. Same for LSP servers. Eglot only includes the very basics for starting up LSP servers to prevent constant changes. Basically just the binary name. I think you should stick with that idea too for sanity reasons.

But good generic configuration options for DAP integrations would prevent a dap-mode situation, where everything is really tangled up. So I'm all for experimenting with that and adjusting dape-jdtls to see if it makes things easier.

The JDTLS specific config parameters like :mainClass and :projectName would be useful to have also in dape--read-config. Now there is a completing read to assist a user, but setting those values on the call to dape would be a bit quicker by skipping the completing read and also allow easy repetition. For :classPaths not so much: in practice it is so long that nobody will want to see it in the minibuffer, let alone change it there. Let the JDTLS handle the classpath, based on the project and entrypoint.

There are also some JDTLS + Debug Server options that really don't need user changes, like the :console :type and :request, those are essentially fixed or it breaks. Perhaps we should place those in a different part of the config?

So yes, I would like to see where the fn evaluation leads to more user configurability. Maybe the name of the function could be pre-init-fn or something to be a bit more specific?

MagielBruntink commented 11 months ago

In https://github.com/svaante/dape/pull/22/commits/3c007860785b2234c7c1671c1375bdfabef149d8 I reworked the approach to function like a completion. So, through dape-jdtls-complete-config a user-supplied config (perhaps empty) will be fully completed to one that works for jdtls.

The approach still uses eglot to do some completions and sometimes requests user selection (eg. of entrypoint). The user can pre-fill any value into the config it provides to the completion function, and those will take priority. The user is trusted to provide the right values.

This can be run as the fn function "inside" Dape, or completely outside of Dape by just generating a config on the fly. Then Dape can be called with that completed config as an argument. I think this provides maximum flexibility.

svaante commented 11 months ago

Sorry for taking some time to respond.

Some pointers and tips.

  1. interactive functions should be avoided at all costs inside of the fn keyword, it brakes usage with repeat-mode.
  2. Dape will not store stuff in the history variable that does not change from the original config but will use them as compleation if tab is used after writing jdtls
  3. Using history is in when running dape is your friend, if you run dape and press up or C-r to access old configurations which ignores modes.

Example below is something to work out of, the key here is that :program is all fn needs to start the debug session which is also the only thing saved in dape-history as it's the only thing that differs from the original configuration.

(add-to-list 'dape-configs
             `(jdtls
               modes (java-mode java-ts-mode)
               fn (lambda (config)
                    (with-current-buffer (find-file-noselect (plist-get config :program))
                      (unless (eglot-current-server)
                        ;; Or maybe start it??
                        (user-error "Please start jdtls eglot server."))
                      (let* ((entrypoint
                              (seq-find (lambda (main-class)
                                          (equal (plist-get config :program)
                                                 (plist-get main-class :filePath)))
                                        (eglot-execute-command (eglot-current-server)
                                                               "vscode.java.resolveMainClass"
                                                       (project-name (project-current)))))
                             (class-paths-candidates
                              (eglot-execute-command (eglot-current-server)
                                                     "vscode.java.resolveClasspath"
                                         (vector (plist-get entrypoint :mainClass)
                                           (plist-get entrypoint :projectName))))
                             (class-paths
                              (pcase (plist-get config 'classPaths)
                                ("auto"
                                 (or (seq-find (lambda (class-paths)
                                                 (length> class-paths 0))
                                               class-paths-candidates)
                                     []))
                                ;; Maybe some other strategy
                                (_ (error "Unable to derive class paths.")))))
                        (append
                         (list 'port (eglot-execute-command (eglot-current-server)
                                                            "vscode.java.startDebugSession" nil)
                               :classPaths class-paths)
                         entrypoint
                         config))))
               classPaths "auto" ;; Just a suggestion
               :request "launch"
               :type "java"
               :console "dape"
           :program (lambda ()
                    (unless (eglot-current-server)
                      (user-error "Please start jdtls eglot server."))
                    (if-let ((candidates
                              (cl-map 'list
                                      (lambda (entry) (plist-get entry :filePath))
                                      (eglot-execute-command (eglot-current-server)
                                                             "vscode.java.resolveMainClass"
                                                     (project-name (project-current))))))
                        (completing-read "Main file: "
                                         candidates nil t)
                      (dape-find-file)))
               :env ()
               :args ""
               :stopOnEntry t)
MagielBruntink commented 11 months ago

Hi, thanks!

Looks much simplified! Some remarks:

That last point perhaps points to a different moment in the initialization to run such config helpers, like you suggested before?

svaante commented 11 months ago

the :program selection code would run while setting the dape-configs which may not be always practical (eg. in an init.el file`.

Not sure what you mean here? It's just executed when you call dape with "jdtls". It's not called again if the :program property is set in the minibuffer. So it's a one time thing when you want to debug an project.

I think the :mainClass and the :program can be two different things. A user may be looking at a file A.java they want to debug, but which is not a main class. Then I think :program should become A.java and they should get some help selecting an entrypoint/main class to use for the debug.

Fully agree, see exampel below Also it makes sense to not use :program is not used by java-debug AFAIK.

I'll investigate :classPaths "auto" nice find! [Edit: does not work, unfortunately.]

This not really a thing I realized, was just lookig at the example configuration from the vscode plugin, it's something that the vscode java debugging plugin consumes.

I think I'm getting more and more in favor of running this config "completion" just outside of Dape, and then using the completed config as a startup argument like M-: (dape (dape-jdtls-complete-config '(...)) Perhaps with a helper/wrapper function to get it started.

I would not reccomend this approch, the dape interactive command should be the only way to start a session, I am willing to change the dape configuration format to allow for jdtls to be configurable. I believe that dape minibuffer usage to be an integral part of Dape.

I don't know if the idea behind the minibuffer compleation is unintuative, but the idea is that you run "jdtls" then it resolves all properties needed.

This is my suggested approach, if you think there is some functionallity that is missing, I am not totaly against of haveing an fn-read which is resolved inside of the interactive part of the dape command.

(setq dape-configs
      `((jdtls
         fn (lambda (config)
              ;; Eglot looks for servers with `default-directory'
              ;; Maybe we should start eglot here if it does not run for project `:cwd'
              ;; But then we need to wait for it to start
              ;; Additionally it's probobly prudent to check the eglot capabilities for
              ;; "vscode.java.resolveMainClass"
              (pcase-let* ((default-directory (plist-get config :cwd))
                           (entrypoint
                            (seq-find (lambda (entrypoint)
                                        (equal (plist-get entrypoint :mainClass)
                                               (plist-get config :mainClass)))
                                      (eglot-execute-command (eglot--current-server-or-lose)
                                                             "vscode.java.resolveMainClass" nil)))
                           (`[,module-paths ,class-paths]
                            (eglot-execute-command (eglot--current-server-or-lose)
                                                   "vscode.java.resolveClasspath"
                                                   (vector (plist-get entrypoint :mainClass)
                                                           (plist-get entrypoint :projectName)))))
                (thread-first config
                              (plist-put 'port
                                         (eglot-execute-command (eglot--current-server-or-lose)
                                                                "vscode.java.startDebugSession" nil))
                              (plist-put :mainClass (plist-get entrypoint :mainClass))
                              (plist-put :projectName (plist-get entrypoint :projectName))
                              (plist-put :modulePaths module-paths)
                              (plist-put :classPaths class-paths))))
         :type "java"
         :request "launch"
         :vmArgs " -XX:+ShowCodeDetailsInExceptionMessages"
         :console "integratedConsole"
         :internalConsoleOptions "neverOpen"
         :javaExec ,(executable-find "java")
         :cwd dape--default-cwd
         :mainClass (lambda ()
                      (completing-read "Main class: "
                                       (cl-map 'list
                                               (lambda (entrypoint)
                                                 (plist-get entrypoint :mainClass))
                                               (eglot-execute-command (eglot--current-server-or-lose)
                                                                      "vscode.java.resolveMainClass" nil))
                                       nil t))
         :shortenCommandLine "none"
         :stopOnEntry nil)))
MagielBruntink commented 11 months ago

Very nice! There is one issue with this, because :program is no longer there (which I used to switch to a .java buffer to ensure there is an Eglot server active. This issue happens if you do r(restart) in the REPL, for instance. Because in that REPL buffer, there is no Eglot server, and it won't find one with just the default-directory or working directory. That's because there can actually be multiple servers in the same project, eg. for different languages.

We could also try to get the file path of the main class and use that, but that actually comes too late. Or store the file path of the file for which the user initiated the debugging session (I thought :program was a nice place for that).

MagielBruntink commented 11 months ago

Also, in complex projects with nested modules, there can be two main classes called eg. "Main". Then we get in trouble if we match only on the main class name to find a matching project - it may pick the wrong one. I think we need to get the projectName also in the first go (outside of the fn).

MagielBruntink commented 11 months ago

I think the following fixes these issues. It's a bit more involved around the completing-read, but code can probably be streamlined. Only one call to resolveMainClass though, that's a win:

(setq dape-configs
      `((jdtls
         fn (lambda (config)
          (with-current-buffer (find-file-noselect (plist-get config 'program))
        (pcase-let* ((default-directory (plist-get config :cwd))
                 (entrypoint (plist-get config 'entrypoint))
                           (`[,module-paths ,class-paths]
                            (eglot-execute-command (eglot--current-server-or-lose)
                                                   "vscode.java.resolveClasspath"
                                                   (vector (plist-get config :mainClass)
                                                           (plist-get entrypoint :projectName)))))
                (thread-first config
                              (plist-put 'port
                                         (eglot-execute-command (eglot--current-server-or-lose)
                                                                "vscode.java.startDebugSession" nil))
                              (plist-put :mainClass (plist-get entrypoint :mainClass))
                              (plist-put :projectName (plist-get entrypoint :projectName))
                              (plist-put :modulePaths module-paths)
                              (plist-put :classPaths class-paths)))))
         :type "java"
         :request "launch"
         :vmArgs " -XX:+ShowCodeDetailsInExceptionMessages"
         :console "integratedConsole"
         :internalConsoleOptions "neverOpen"
         :javaExec ,(executable-find "java")
         :cwd dape--default-cwd
     program dape-find-file-buffer-default
         entrypoint (lambda ()
              (let ((selected-entrypoint
                 (s-split "/"
                      (completing-read "Entrypoint: "
                          (cl-map 'list
                              (lambda (entrypoint)
                            (s-concat (plist-get entrypoint :projectName) "/"
                                  (plist-get entrypoint :mainClass)))
                              (eglot-execute-command (eglot--current-server-or-lose)
                                         "vscode.java.resolveMainClass" nil))
                          nil t))))
            (list :projectName (nth 0 selected-entrypoint)
                  :mainClass (nth 1 selected-entrypoint))))
         :shortenCommandLine "none"
         :stopOnEntry t)))
svaante commented 11 months ago

Nice!

If we really don't want to fill in program we can derive the Eglot instance with :cwd which I failed to do in my example.

      `((jdtls
         fn (lambda (config)
              ;; Eglot looks for servers with `default-directory'
              ;; Maybe we should start eglot here if it does not run for project `:cwd'
              ;; But then we need to wait for it to start
              ;; Additionally it's probobly prudent to check the eglot capabilities for
              ;; "vscode.java.resolveMainClass"
              (pcase-let* ((default-directory (plist-get config :cwd))
                           (server
                            (or
                             (seq-find (lambda (server)
                                         (seq-contains-p (thread-first (eglot--capabilities server)
                                                                       (plist-get :executeCommandProvider)
                                                                       (plist-get :commands))
                                                         "vscode.java.startDebugSession"))
                                       (gethash (eglot--current-project)
                                                eglot--servers-by-project))
                             (error "Unable to find jdtls Eglot instance.")))
...

I don't know if it makes sense but maybe entrypoint could be a string instead of an plist. Maybe "<project name>:<main class>"

svaante commented 11 months ago

I would also avoid (require 's) as it's not a part of the emacs distribution.

MagielBruntink commented 11 months ago

Calls to eglot--current server really needs to be in a buffer that has an active server. It's not enough to be in a buffer that shares a working directory with a buffer that has a server running.

So we need a Java file in the right project, essentially. Perhaps just grab it with (buffer-file-name (buffer-current)) instead of the dape function that asks for user input.

svaante commented 11 months ago

Calls to eglot--current server really needs to be in a buffer that has an active server. It's not enough to be in a buffer that shares a working directory with a buffer that has a server running.

Do you mean eglot--current-server or eglot--current-projectif you mean eglot--current-server in my example all of the instances of eglot--current-server-or-lose be replaced by the let binding server. If you mean eglot--current-project it's just a call to project.eland project-current. But I guess you will end up in some problems with that if the current process is not started in an project.el project.

MagielBruntink commented 11 months ago

There can also be multiple eglot servers in one project. I think overall it’s more predictable if we keep track of the file on which the user launched dape. Could be called “program” or something else.

MagielBruntink commented 11 months ago

@svaante What do you think of this iteration?

All the "dynamic" config values are calculated outside of fn then inserted as a plist. fn basically only appends it to the config.

(setq dape-configs
    `((jdtls
       modes (java-mode java-ts-mode)

       fn (lambda (config)
        (append config (plist-get config 'entrypoint)))

       port (lambda () (eglot-execute-command (eglot-current-server)
                          "vscode.java.startDebugSession" nil))
       entrypoint (lambda ()
            (pcase-let* ((`(,project-name ,main-class)
                      (split-string
                       (completing-read
                    "Main class: "
                    (cl-map 'list
                        (lambda (candidate)
                          (concat (plist-get candidate :projectName) ":"
                              (plist-get candidate :mainClass)))
                        (eglot-execute-command (eglot-current-server)
                                       "vscode.java.resolveMainClass"
                                       (project-name (project-current))))
                    nil t)
                       ":"))
                     (`[,module-paths ,class-paths]
                      (eglot-execute-command (eglot-current-server) "vscode.java.resolveClasspath"
                                 (vector main-class project-name))))
              (list :projectName project-name :mainClass main-class
                :modulePaths module-paths :classPaths class-paths)))
       :type "java"
       :request "launch"
       :stopOnEntry t
       :vmArgs " -XX:+ShowCodeDetailsInExceptionMessages"
       :console "integratedConsole"
       :internalConsoleOptions "neverOpen")))
MagielBruntink commented 11 months ago

It works almost perfectly and I think the code is quite streamlined. One downside is those classpaths now do show up in the minibuffer and for non-trivial projects that's too much stuff.

svaante commented 11 months ago

Looks good to me! See last commit ba63e50, I have now included base configurations for all tested adapters. Would be great if you could compleate the Copyright Assignment and add jdtls to the list.

MagielBruntink commented 11 months ago

Alright, I am settling on the following. The classpaths showing up in the minibuffer were really problematic so I have changed that back to happen within the fn function. So :classPaths stays out of the history. Code is a bit longer, but it's OK.

I'll work out the copyright assignment thing.

  (add-to-list 'dape-configs
    `(jdtls
       modes (java-mode java-ts-mode)

       fn (lambda (config)
        (pcase-let* ((target (plist-get config 'target))
                 (default-directory (project-root (project-current)))
                 (server (with-current-buffer (find-file target)
                       (eglot-current-server)))
                 (`(,project-name ,main-class)
                  (split-string (plist-get config 'entrypoint) ":"))
                 (`[,module-paths ,class-paths]
                  (eglot-execute-command server "vscode.java.resolveClasspath"
                             (vector main-class project-name))))
          (thread-first config
                (plist-put :mainClass main-class)
                (plist-put :projectName project-name)
                (plist-put :modulePaths module-paths)
                (plist-put :classPaths class-paths))))

       port (lambda () (eglot-execute-command (eglot-current-server)
                          "vscode.java.startDebugSession" nil))
       entrypoint (lambda ()
            (completing-read
             "Main class: "
             (cl-map 'list
                 (lambda (candidate)
                   (concat (plist-get candidate :projectName) ":"
                       (plist-get candidate :mainClass)))
                 (eglot-execute-command (eglot-current-server)
                            "vscode.java.resolveMainClass"
                            (project-name (project-current))))
             nil t))

       target (lambda () (file-relative-name (buffer-file-name)
                         (project-root (project-current))))
       :args ""
       :stopOnEntry t
       :type "java"
       :request "launch"
       :vmArgs " -XX:+ShowCodeDetailsInExceptionMessages"
       :console "integratedConsole"
       :internalConsoleOptions "neverOpen"))
MagielBruntink commented 10 months ago

I'll be creating a new PR with the Java config isolated to the README. Closing this one. Also I will request copyirght transfer to GNU for that new PR.