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

feat: automatic debugging support #2699

Merged
merged 5 commits into from
Sep 19, 2024
Merged

Conversation

stuartwdouglas
Copy link
Contributor

Automatically start go/java processes with remote debugging enabled, and automatically create profiles for both Intellij and VSCode.

This was referenced Sep 16, 2024
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/auto-debugging branch 4 times, most recently from 3bae0bb to b02bb00 Compare September 17, 2024 05:18
@stuartwdouglas stuartwdouglas added the skip-proto-breaking PRs with this label will skip the breaking proto check label Sep 17, 2024
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/auto-debugging branch 5 times, most recently from d34711f to 1fdb3ea Compare September 17, 2024 06:50
@stuartwdouglas stuartwdouglas marked this pull request as ready for review September 17, 2024 07:53
@stuartwdouglas stuartwdouglas requested review from safeer and removed request for a team September 17, 2024 07:53
@stuartwdouglas stuartwdouglas removed the skip-proto-breaking PRs with this label will skip the breaking proto check label Sep 17, 2024
internal/localdebug/local_ide_debug.go Outdated Show resolved Hide resolved
return ""
}

type DebugInfo struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer to place types at the top of the file to make it easier to read down the file.

}

// findIdeaFolder recurses up the directory tree to find a .idea folder.
func findIdeaFolder(startPath string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate code to that in the VSCode one, parameterise on the relative path you're looking for.

// findLaunchJSON recurses up the directory tree to find a .vscode folder
// If it can't find one it creates one next to project.toml
// It then returns the path to the launch.json file
func findLaunchJSON(startPath string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change this to terminate at the project root rather than the root directory, so we don't randomly start writing into an unrelated VSCode directory. Same with the IDEA stuff.

continue
}
entry := map[string]interface{}{
"name": name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

configurations[pos].(map[string]interface{})["port"] = v.Port //nolint:forcetypeassert
continue
}
entry := map[string]interface{}{
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/interface{}/any/g

@alecthomas
Copy link
Collaborator

Nice! This is an awesome QoL change.

@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/auto-debugging branch 4 times, most recently from a4a5f59 to 1337366 Compare September 17, 2024 20:03
@stuartwdouglas
Copy link
Contributor Author

I have made a few changes:

  • IDE support is now passed around in the context
  • No cleanup, instead the debug port from existing profiles is re-used. This should give a better experience if there is a delay between start and the IDE loading the profile

@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/auto-debugging branch 4 times, most recently from 1a57c23 to a66b26c Compare September 17, 2024 22:28
@stuartwdouglas
Copy link
Contributor Author

This should be good to go now

Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

LGTM but I do find it not great that it always creates a .vscode directory. I feel like it would be better to read either config. Also it seems possible these could get desynced anyway...eg. if you start ftl dev, there's no IDEA config, it writes the ports for VSCode, then you start GoLand. At this point GoLand won't have the correct ports and they won't get updated?

I also think it would be a good idea to have this IDE-specific logic behind an interface of some sort, to make it simpler to add new ones.

eg.

type IDE interface {
  // Used returns true if this IDE is in use in the project.
  Used() bool
  UpdatePorts(ports map[string]*DebugInfo) error
  // ExistingPorts reads the existing ports mapped for this IDE.
  ExistingPorts() (map[string]*DebugInfo, error)
}

return ret
}

func (r *IDEIntegration) IntoContext(ctx context.Context) context.Context {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to pass this through the context? Usually that's only reserved for things that are used all over the place, like loggers.

internal/localdebug/local_ide_debug.go Outdated Show resolved Hide resolved
internal/localdebug/local_ide_debug.go Outdated Show resolved Hide resolved
@stuartwdouglas
Copy link
Contributor Author

LGTM but I do find it not great that it always creates a .vscode directory. I feel like it would be better to read either config. Also it seems possible these could get desynced anyway...eg. if you start ftl dev, there's no IDEA config, it writes the ports for VSCode, then you start GoLand. At this point GoLand won't have the correct ports and they won't get updated?

It would get updated when the first runner starts. This would also only apply if it is the first time you opened the project with GoLand.

In terms of always writing .vscode directories I don't think it is a big deal, e.g. we currently have both in FTL. We could potentially improve it later based on feedback.

I also think it would be a good idea to have this IDE-specific logic behind an interface of some sort, to make it simpler to add new ones.

I agree, but I think we should just get this in an test it out and then refine it from there.

@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/auto-debugging branch 4 times, most recently from 8e61508 to ff43ab0 Compare September 18, 2024 02:22
@stuartwdouglas stuartwdouglas merged commit 8184741 into main Sep 19, 2024
91 checks passed
@stuartwdouglas stuartwdouglas deleted the stuartwdouglas/auto-debugging branch September 19, 2024 02:40
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