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

OCaml earlybird debugger #1148

Merged
merged 37 commits into from
Jul 11, 2023
Merged

OCaml earlybird debugger #1148

merged 37 commits into from
Jul 11, 2023

Conversation

sim642
Copy link
Contributor

@sim642 sim642 commented Jun 23, 2023

Closes #534.

This is more-or-less a port of earlybird's own small VSCode extension to OCaml: https://github.com/hackwaly/ocamlearlybird/tree/271bdd877ae285c06df1d4e90bc1d11dbdf83fb5/integrations/vscode. Parts of this port are borrowed from the earlier attempt #650.

Unlike that earlier attempt, this contains no dune executable detection or anything of that sort. It simply allows the earlybird DAP server to be launched if it is installed in the opam switch. The only additional integration is that ocamlearlybird is launched from the switch chosen in this extension.

By the way, I have picked up maintaining and updating earlybird itself such that it works on newer OCaml versions, but that is not yet released (only available directly from https://github.com/hackwaly/ocamlearlybird). It would be nice if this PR was part of the revival of earlybird, such that its own extension could be deprecated.

@tmattio
Copy link
Collaborator

tmattio commented Jun 23, 2023

I'm thrilled to see this PR, thanks a lot for working on this @sim642!

Coincidentally, we've had a lot of discussions related to debugging with the OCaml Platform team recently. One thing that was blocking the adoption of earlybird was its license. I see that you've moved the code (I assume with the permission of the author) to be licensed under MIT. That's great, this is going to enable the use of earlybird in the Platform!

Before we consider using earlybird as a DAP in VSCode, it'd be great to incubate it in the Platform: the vscode extension is a driver for the tools of the OCaml Platform. Would you be ok to work together on an RFC for the incubation of early bird as part of your work to have earlybird used in VSCode?

@tmattio
Copy link
Collaborator

tmattio commented Jun 23, 2023

We discussed this further with @rgrinberg.

We agree that promoting an incubation too early doesn't make sense. At the same time, an integration in VSCode would help earlybird to get the user feedback and user testing it needs to demonstrate that it's ready for incubation in the Platform.

We'll still want to wait for it to be incubated to fully commit to maintaining an integration, but in the immediate term, we could merge a first integration behind an experimental flag.

@sim642
Copy link
Contributor Author

sim642 commented Jun 23, 2023

I see that you've moved the code (I assume with the permission of the author) to be licensed under MIT.

That was done by the original author in hackwaly/ocamlearlybird@a4398c4.

We'll still want to wait for it to be incubated to fully commit to maintaining an integration, but in the immediate term, we could merge a first integration behind an experimental flag.

Fair enough, although I'm not sure it's possible to put the debugger declaration behind a configuration option in package.json.

@tmattio
Copy link
Collaborator

tmattio commented Jun 23, 2023

I'm not sure it's possible to put the debugger declaration behind a configuration option in package.json.

We could maintain a staging branch for pre-release versions (https://code.visualstudio.com/api/working-with-extensions/publishing-extension#prerelease-extensions)

@rgrinberg
Copy link
Contributor

I would personally lean towards just marking this feature as "experimental" in big letters and making it available without much hassle. At this point, the main advantage of integrating this feature would be to gather some user experience reports from using it. It would be counter productive to make people go through another step before they're able to use this.

@sim642
Copy link
Contributor Author

sim642 commented Jun 23, 2023

I would personally lean towards just marking this feature as "experimental" in big letters and making it available without much hassle.

Indeed as-is this isn't too visible: it just shows up in the list of debuggers when trying to add a a debug configuration and in the context menu for .bc files. Right now there aren't any messages suggesting to opam install earlybird either.

I'll append "experimental" to the registered debugger name, etc, which should be visible to anyone trying to activate it.

@tmattio
Copy link
Collaborator

tmattio commented Jun 23, 2023

Alright, as long as (1) we're comfortable we'll be able to remove support (without alienating users) in case we see that earlybird isn't ready for wide adoption just yet, or that the maintenance status becomes unclear in the future and (2) we have in mind that we should aim for incubation to commit to long term support - I'm happy to go with the integration you see fit.

@sim642 sim642 requested a review from mnxn June 29, 2023 08:21
Copy link
Collaborator

@mnxn mnxn left a comment

Choose a reason for hiding this comment

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

Nice work with the binding changes!
I added a comment about one more issue in the code that I overlooked.

I attempted to use the debugger yesterday on macOS, but wasn't able to get it to work. I'll try again soon, but I think that either my setup is broken or there might be a lack of feedback on invalid configurations.

src/earlybird.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@mnxn mnxn left a comment

Choose a reason for hiding this comment

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

I got the debugging feature working on macOS. The only awkward behavior I noticed was that the debugger doesn't terminate if the a.out file (from the default launch configuration) doesn't exist. The integrated terminal will show a missing file error, but the VS Code debugging UI still stays active. I'm not sure there's a way to present an VS Code error in this case, but it might be worth looking into in the future.

I believe this PR is ready to merge in a experimental state.

@sim642
Copy link
Contributor Author

sim642 commented Jul 4, 2023

The only awkward behavior I noticed was that the debugger doesn't terminate if the a.out file (from the default launch configuration) doesn't exist. The integrated terminal will show a missing file error, but the VS Code debugging UI still stays active. I'm not sure there's a way to present an VS Code error in this case, but it might be worth looking into in the future.

Thanks for the feedback! As a quick improvement I changed that a.out path to something more suggestive.
In some further future it'd be nice if the extension could get a list of executables from dune and the debugging configurations would be suggested dynamically from them.

-> ?column:ViewColumn.t
-> ?preserveFocus:bool
-> unit
-> TextEditor.t Promise.t
[@@js.global "vscode.window.showTextDocument"]

val showTextDocument2 :
val showTextDocument' :
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you describe the issue this commit is fixing? I think also putting it in a separate PR would be valuable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In VSCode extension API showTextDocument has a couple of overloads. Specifically the cases without TextDocumentShowOptions only take TextDocument arguments, not Uri: https://code.visualstudio.com/api/references/vscode-api#window.showTextDocument.

I think there wasn't a problem before because the Uri overload is not used by OCaml Platform.

@mnxn
Copy link
Collaborator

mnxn commented Jul 8, 2023

The debugging feedback is much better now when the file doesn't exist.
I'm still concerned that the initial configuration is invalid by default.

The VS Code debugger extension example shows a way to ask for a program name: https://code.visualstudio.com/api/extension-guides/debugger-extension#anatomy-of-the-package.json-of-a-debugger-extension

Maybe it would be better to ask for a path to the program to debug that way?

@sim642
Copy link
Contributor Author

sim642 commented Jul 8, 2023

I think I can give it a try although typing in a long path into _build into a pop-up dialog isn't super convenient either. The most convenient way is to use the "Start debugging" command from the file explorer context menu on a .bc file, although it's completely undiscoverable right now.
I should probably also add something to README about the debugging support, both for OCaml Platform and earlybird itself (if OCaml Platform becomes the new way to use it).

@mnxn
Copy link
Collaborator

mnxn commented Jul 8, 2023

I agree writing a long path isn't ideal, but I think having this behavior is the best option for now. In the future (depending on how the experiment progresses), the command could be changed to list executables built by dune like you mention.

In the meantime, you can describe the right-click workflow in the README so it's easier to find.

@sim642
Copy link
Contributor Author

sim642 commented Jul 9, 2023

I agree writing a long path isn't ideal, but I think having this behavior is the best option for now. In the future (depending on how the experiment progresses), the command could be changed to list executables built by dune like you mention.

I found a nice compromise using vscode.window.showOpenDialog and implemented it for the ask command.

In the meantime, you can describe the right-click workflow in the README so it's easier to find.

That and other information about earlybird is now in the README.

src/earlybird.ml Outdated Show resolved Hide resolved
src/earlybird.ml Outdated Show resolved Hide resolved
@rgrinberg rgrinberg added this to the 1.13.0 milestone Jul 11, 2023
@rgrinberg rgrinberg merged commit 3d184cd into ocamllabs:master Jul 11, 2023
mseri pushed a commit to ocaml/opam-repository that referenced this pull request Jul 12, 2023
CHANGES:

### Added

* Add OCaml 4.13, 4.14 and 5.0 support.
* Add `--version` command line option.

### Changed

* Relicense under MIT (from GPL).
* Deprecate own VSCode extension in favor of VSCode OCaml Platform integration: ocamllabs/vscode-ocaml-platform#1148.

### Fixed

* Fix being stuck if program doesn't exist (hackwaly/ocamlearlybird#49).

### Removed

* Remove OCaml 4.11 support.
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

### Added

* Add OCaml 4.13, 4.14 and 5.0 support.
* Add `--version` command line option.

### Changed

* Relicense under MIT (from GPL).
* Deprecate own VSCode extension in favor of VSCode OCaml Platform integration: ocamllabs/vscode-ocaml-platform#1148.

### Fixed

* Fix being stuck if program doesn't exist (hackwaly/ocamlearlybird#49).

### Removed

* Remove OCaml 4.11 support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate ocamlearlybird
5 participants