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

Possibility to run build from the external code editor. #6604

Closed
AGulev opened this issue May 14, 2022 · 25 comments
Closed

Possibility to run build from the external code editor. #6604

AGulev opened this issue May 14, 2022 · 25 comments
Labels
editor Issues related to the Defold editor feature request A suggestion for a new feature

Comments

@AGulev
Copy link
Contributor

AGulev commented May 14, 2022

Is your feature request related to a problem? Please describe (REQUIRED):
When a developer uses an external code editor he has to switch back to the editor after every change he wanna test in the build.
That would be nice to have a possibility to ask editor read changes and build game without changing active window.

Describe the solution you'd like (REQUIRED):
The editor may receive a command from an external code editor using the editor's web server.

Describe alternatives you've considered (REQUIRED):
none

@AGulev AGulev added feature request A suggestion for a new feature editor Issues related to the Defold editor labels May 14, 2022
@britzl
Copy link
Contributor

britzl commented May 14, 2022

I think it is also worthwhile to investigate how to speed up a bob build.

@AGulev
Copy link
Contributor Author

AGulev commented May 14, 2022

I think it is also worthwhile to investigate how to speed up a bob build.

yep, it's another issue. I'll create a ticket, now when we have a nice time report that should help to figure out what's going on there

@mikatuo
Copy link
Contributor

mikatuo commented Feb 11, 2023

Hello,
Will you accept a PR for this task? Have you had a chance to think about the design and requirements?

How I see it:

Rather than adding new endpoints for each command ("Build", "Rebuild", "Hot Reload", etc) add new single endpoint that will be accepting different "commands".
Just to be safe it might be better to start a second web server. This way it will not affect the existing web server in any way.
The first command that it will support might be "Build". Later "handlers" for new commands could be added.
It could accept JSON for ease of use. Then it could be called from Lua, in example, from .editor_script files.
Another big question I have is can it be written in Java?

@britzl
Copy link
Contributor

britzl commented Feb 12, 2023

Will you accept a PR for this task?

Yes, absolutely! But we need a design discussion first! You have some suggestions that we need to discuss and agree upon before moving on with the implementation!

Rather than adding new endpoints for each command ("Build", "Rebuild", "Hot Reload", etc) add new single endpoint that will be accepting different "commands"

There are pros and cons with this. With one endpoint per action it becomes obvious from the url what the endpoint does. And if an endpoint doesn't exists you'll get a 404.

A single endpoint will obviously result in fewer endpoints but it will be more prone to errors as you need to either POST some command or include the command as a query argument.

Just to be safe it might be better to start a second web server. This way it will not affect the existing web server in any way.

I don't see the value in running another web server. More open ports and more CPU resources are required.

Another big question I have is can it be written in Java?

The existing web server is configured with its handlers in boot_open_project.clj and if we are to use the same web server for the new requests then these handlers need to be added here as well. The actual handler code could be written in Java, but in the case of a Build command you want to immediately call the corresponding Clojure code to trigger the build so I see very little point in using Java.

We need some overall input from @matgis and @vlaaad here!

@mikatuo
Copy link
Contributor

mikatuo commented Feb 13, 2023

There are pros and cons with this. With one endpoint per action it becomes obvious from the url what the endpoint does. And if an endpoint doesn't exists you'll get a 404.
A single endpoint will obviously result in fewer endpoints but it will be more prone to errors as you need to either POST some command or include the command as a query argument.

Good point. I haven't thought about it from that perspective. The main reason was extensibility, to make it super simple adding more commands in future. Another reason was to try to avoid possible confusion between existing "internal" build or hot-reload endpoints with the "external" stuff.

I don't see the value in running another web server. More open ports and more CPU resources are required.

Makes sense. Probably pros of an independent web server for "external use" are not worth the extra resource costs. One possible challenge with the existing server is to figure out how to make it discoverable since it is being assigned a random port by OS. To keep it simple for now, I will try to scan/find it on the VSCode side.

The existing web server is configured with its handlers in boot_open_project.clj and if we are to use the same web server for the new requests then these handlers need to be added here as well. The actual handler code could be written in Java, but in the case of a Build command you want to immediately call the corresponding Clojure code to trigger the build so I see very little point in using Java.

I was not sure if existing Java files in the source code were legacy code or not and what was your strategy as far as Clojure/Java is concerned. Since it will be for external editors perhaps someone else would like to contribute in future.

@britzl
Copy link
Contributor

britzl commented Feb 13, 2023

One possible challenge with the existing server is to figure out how to make it discoverable since it is being assigned a random port by OS. To keep it simple for now, I will try to scan/find it on the VSCode side.

This could be made configurable. We can check for an environment variable and use the port specified if the env var is set.

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

It should be relatively trivial for us to add new endpoints for build, rebuild, and hot reload as long as they are fire-and-forget requests. If we need progress reporting or even just some way to know the build has completed, things may be a bit more complicated. You could do a long-polling request and simply not answer the request until the build has completed, but we don't want to lock up either VSCode or the editor in the meantime. On the editor side we could make sure such requests are processed on a background thread and presumably we could use promises or some similar mechanism in VSCode to get around it.

Another approach would be to expose a websocket interface from the editor for external tools to communicate with. That gives us more flexibility since we can do two-way communication and not just respond to direct requests.

I think we should focus on implementing fire-and-forget requests with the existing web server for now, though.

@britzl
Copy link
Contributor

britzl commented Feb 13, 2023

I think we should focus on implementing fire-and-forget requests with the existing web server for now, though.

I totally agree that this should be the first step!

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

What if there's a build error? We will show it in the editor, but we won't have any way to communicate that something went wrong to VSCode if it is a fire-and-forget request.

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

Regarding scanning for ports, users can have other servers running on their machine, so we probably want to restrict ourselves to a port range or something like that. I guess users might also have several Defold editors open with different projects. We might want some mechanism to match up the project path with a particular editor. Maybe we could port-scan a small range and send a handshake request that responds with the project path or something like that?

@vlaaad
Copy link
Contributor

vlaaad commented Feb 13, 2023

What if 2 editors run on the same machine? Also, AFAIK we use UPnP for the discovery of remote engines running on the network (e.g. mobile), perhaps this is something that could also be used for vscode-editor communication and discovery?

@mikatuo
Copy link
Contributor

mikatuo commented Feb 13, 2023

Eventually, it would be great to identify an opened project as well as Editor version (+sha1). I also will need guidance where to put new handlers in code.

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

Handlers are registered with the web server here: https://github.com/defold/defold/blob/editor-dev/editor/src/clj/editor/boot_open_project.clj#L179-L183

I think it makes sense for the app-view to handle the request, since we want it to show the build errors view, be able to enable debugging views, etc.

You pretty much want to do the equivalent of triggering the relevant defhandler from app_view.clj, I.e. :build, :rebuild or :hot-reload. We also want to respect the enabled? semantics of the handlers. I.e. you shouldn't be able to trigger a build while one is in progress. This ensures the user will not be able to trigger a build from outside and inside the editor at the same time.

Perhaps it would even make sense to be able to trigger arbitrary :global defhandlers from a "command" endpoint? 🤔

Also of concern: For the editor to be able to see the changes saved from VSCode without it getting app focus, we'd want to call disk/async-reload! and trigger the build after we've incorporated the saved changes into our data model. We'd definitely need to do that for anything that builds the project.

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

I'm starting to feel us editor developers need to get involved in this request as it might not be immediately obvious what I'm talking about to someone new to the code base. 😅

I'd love to hear your thoughts around the interface we expose to VSCode. Are there other commands you'd like to see exposed in the near future?

@mikatuo
Copy link
Contributor

mikatuo commented Feb 13, 2023

Thanks, @matgis! That should be enough to start. I'm a little bit familiar with the editor code so overall it makes sense, but the part that executes context menu commands -> global defhandlers is Terra Incognito :)

If I could manage to make it working then I might ask for more guidance.

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

Sorry if I stepped on your toes, but I got a bit carried away and implemented part of it in this PR: #7382

Turns out we could just use the existing ui mechanism for triggering command handlers, which let us add this functionality in a separate file from app_view.clj.

I need to figure out a mechanism for integrating the external changes into our data model before it can be merged, but you might be able to play around with it a bit at least!

@matgis
Copy link
Contributor

matgis commented Feb 13, 2023

Updated the PR to integrate the external changes into our data model before executing any command.

It would be nice to have the UPnP discovery mechanism as well.

@mikatuo
Copy link
Contributor

mikatuo commented Feb 14, 2023

Wow! That's awesome! Thank you!!!
At the first glance the code in the PR reminded me of this :D
image

I'd love to hear your thoughts around the interface we expose to VSCode. Are there other commands you'd like to see exposed in the near future?

I'm more than happy with the 3 that you added!
Only if it's not too complicated... It might be useful to be able to "pull" Editor logs with a request. Use case: trigger build and then for the duration of 10 seconds every 1 second "pull" logs to show progress or errors.

@matgis
Copy link
Contributor

matgis commented Feb 16, 2023

The PR has been merged. Please play around with it and let us know how it works for you!

Caveats:
The port is still randomized, and we haven't implemented SSDP discovery yet, so you'll have to scan for it or parse it from the editor console output or log file message.

@mikatuo
Copy link
Contributor

mikatuo commented Feb 16, 2023

Wonderful! Thank you!

@matgis do I need to run the editor from the dev branch or is there any chance to try it as part of 1.4.3-beta?

@matgis
Copy link
Contributor

matgis commented Feb 16, 2023

As long as you are on the default editor-alpha update channel, you should have it in 1.4.2 once you update the editor. You can see the :local-url in the editor log after you've opened a project:

INFO  util.http-server - {:line 103, :msg "Http server running", :local-url "http://localhost:12345"}

The documentation has more info around how to use it.

@mikatuo
Copy link
Contributor

mikatuo commented Feb 16, 2023

Works like a charm!

Code_8nqzOGeIKg

@mikatuo
Copy link
Contributor

mikatuo commented Feb 19, 2023

Reading HTTP server port from log files on first call, then the port is saved. On Windows and Linux works great, can't test on Mac but should also be ok. I think that SSDP is not necessary atm.

@AGulev
Copy link
Contributor Author

AGulev commented Mar 18, 2023

So, that means this ticket is done?

@britzl
Copy link
Contributor

britzl commented Mar 19, 2023

I think we can close it.

@britzl britzl closed this as completed Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor Issues related to the Defold editor feature request A suggestion for a new feature
Projects
None yet
Development

No branches or pull requests

5 participants