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

Add hooks for preLaunchTask and postDebugTask #720

Closed
stevearc opened this issue Oct 13, 2022 · 14 comments
Closed

Add hooks for preLaunchTask and postDebugTask #720

stevearc opened this issue Oct 13, 2022 · 14 comments

Comments

@stevearc
Copy link

Problem Statement

The launch.json file format allows specifying a preLaunchTask and a postDebugTask, which will run an asynchronous job before and after the debugging session. At the moment, these values are ignored by nvim-dap, which is unfortunate because they may be a required step for making the debug session work.

Possible Solutions

I have written a task-manager plugin that has support for the VS Code tasks.json file format, and currently have an integration for nvim-dap that wires these properties up correctly. However, it unfortunately relies on some pretty gross monkey patching. I'm proposing adding a hook for dap.run that will, when present, call a user-defined function and pass it a callback that will resume execution when called. That would be enough for me to get rid of the monkey patching, and it's generic enough that it doesn't tie nvim-dap to overseer, or to the launch.json format.

There has been some prior discussion about this (#191), where the suggestion was to put the task running logic into the adapter. I believe that it makes more sense to add this to dap core instead of the adapters because the preLaunchTask/tasks.json format is universal across adapters.

Considered Alternatives

We could continue with the monkey patching. It's more brittle, but it does currently work in most cases.

@stevearc
Copy link
Author

I'm happy to do the work here, btw, I just want to get approval for the approach.

@mfussenegger
Copy link
Owner

I agree that some extension mechanism to handle this use-case would make sense. Do you have a proposal for the API?

Another use-case I can imagine is the codelldb adapter where the vscode extension handles a cargo property on the client side.
Currently handling this would also work via the adapter on_config mechanism, but it doesn't compose well - e.g. a cpp focused dap extension and a rust focused extension may both want to define the codelldb adapter, which would then conflict.

So I'm thinking this could either be something where extensions can register handles for specific properties, or it would be a more general pluggable config pre-processing mechanism that's global instead of per adapter.
Not sure how to deal with conflicts if multiple extensions want to handle the same property, or if it is a generic pre-processing how the ordering would work out.

@stevearc
Copy link
Author

So I'm thinking this could either be something where extensions can register handles for specific properties, or it would be a more general pluggable config pre-processing mechanism that's global instead of per adapter.

I would personally lean more towards a general pluggable config pre-processor. As you mentioned, adapters already have the ability to do this via the enrich_config/on_config function. Conceptually, this feels fairly similar to "listeners", except that it actually blocks execution instead of just being a fire-and-forget event. Perhaps the API could look similar?

local function my_hook() end
dap.on_pre_run["overseer"] = my_hook

Or since this is actually very similar to the adapter enrich_config, maybe we could re-use that name:

dap.enrich_config["overseer"] = my_hook

I don't know if we need to worry too much about conflicting extensions or the ordering of these methods. I've seen a lot of pub/sub APIs in various systems (including vim's autocmds) with minimal or no control over ordering and that usually isn't an issue. But if you think it's something that should be possible, the API could be changed to support internals that are more list-like.

local function my_hook() end
dap.add_pre_run_hook(my_hook) -- implicit ordering; first added, first called
dap.remove_pre_run_hook(my_hook)

This would allow users to roughly enforce some sort of order in a pinch, and the API could easily be extended in the future to take an options table with a priority if it needs to become a real feature.

@mawkler

This comment was marked as off-topic.

@stevearc

This comment was marked as off-topic.

@mawkler

This comment was marked as off-topic.

@mfussenegger

This comment was marked as off-topic.

@stevearc
Copy link
Author

@mfussenegger do you have any comments about the API proposal above?

@hahuang65
Copy link

This would be useful for things such as Ruby on Rail's server debugging, where after the debugging session ends, the server should be stopped.

This helps avoid the need to manually stop the server, as it's hogging the port required to start another debug session:
suketa/nvim-dap-ruby#25

@s1n7ax
Copy link

s1n7ax commented Nov 5, 2023

I would love to have an option for asynchronous tasks as well. But I guess what I'm looking for is more of a on_run hook. I don't think on_config or enrich_config can handle multiple configurations in java. At least I couldn't get it working. At the moment, I update the dap.configurations.java and dap.adapter.java on LspAttachevent. However, if a new main class is created or changed, then those are not reflected. Best option is to wrap functions like overseer has done and call dap.run in the callback.

@mfussenegger
Copy link
Owner

@stevearc could you check if #1241 would cover your requirements?

mfussenegger added a commit that referenced this issue May 30, 2024
mfussenegger added a commit that referenced this issue May 31, 2024
@mfussenegger
Copy link
Owner

mfussenegger commented Jun 1, 2024

Made a follow up that moves on_config from dap.providers to dap.listeners. See #1248

Closing this for now but please let me know if you encounter any limitations with this system.

@stevearc
Copy link
Author

stevearc commented Jun 1, 2024

The new API is great! I refactored my integration to use it instead of monkey patching stevearc/overseer.nvim@ecdfbac
Thanks for adding support!

@mfussenegger
Copy link
Owner

The new API is great! I refactored my integration to use it instead of monkey patching stevearc/overseer.nvim@ecdfbac Thanks for adding support!

Nice to hear.
In case you haven't already: Maybe worth keeping the monkey-patch as fallback and guarding the new mechanism behind a if dap.listeners.on_config then check to keep it working for people who stick to released/tagged nvim-dap versions. At least until I've tagged a new release that includes the functionality.

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

No branches or pull requests

5 participants