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

Use graalvm-compiled native image for clojure-lsp instead of jar #1017

Closed
bpringe opened this issue Feb 5, 2021 · 14 comments
Closed

Use graalvm-compiled native image for clojure-lsp instead of jar #1017

bpringe opened this issue Feb 5, 2021 · 14 comments
Labels

Comments

@bpringe
Copy link
Member

bpringe commented Feb 5, 2021

No description provided.

@PEZ
Copy link
Collaborator

PEZ commented Feb 6, 2021

Maybe we can have a setting where you put a path to any binary or jar with clojure-lsp, and Calva will use this one instead of the bundled jar of the setting is used.

I think that including binaries for all platforms will make the extension a bit heavy and generally harder to maintain. And creating some kind of dynamic download introduces risk and edge cases and stuff.

@bpringe
Copy link
Member Author

bpringe commented Feb 6, 2021

That's a good idea, at least for a first step, if not a permanent solution. @ericdallo mentioned that lsp-mode downloads lsp itself, but I agree that this adds complexity I'd rather not add, as you saw when I created that PR that downloaded the jar, and then closed it.

Just to be sure, Eric, you plan to keep packaging the jar for releases, right?

@ericdallo
Copy link
Contributor

Yes @bpringe the idea is to distribute only the jar and the native binaries.
The only file I'd want to remove is the embedded jar (clojure-lsp) since now we have native binaries, but I think calva don't uses it, only the jar, right?

IMO, it'd be nice if Calva have a option to auto download the correct binary based on user OS but also provide a way to override that path if user prefer. That's exactly how lsp-mode work for every server :)

@bpringe
Copy link
Member Author

bpringe commented Feb 7, 2021

Yeah, I'm not sure the best route here... as Peter said, and as I've seen, adding an auto download feature like that adds complexity and maintenance (though I know lsp-mode does this). The question is whether it's worth it for us. It may be, because I think by default Calva should use the graalvm binary out-of-the-box, without requiring the user to download it themselves and set some setting.

From the specs on mem usage and cpu usage you showed me, it looks like those are drastic improvements that I'd like to provide by default / out-of-the-box to users.

@ericdallo
Copy link
Contributor

ericdallo commented Feb 7, 2021

I see, my only concern is the need to include the 3 binaries on calva?
Just FYI, 3 binaries * ~30mb(zipped), each binary unzipped is ~118MB

I still think Calva download the correct one would be the best. Also, clojure-lsp provides a install-latest-clojure-lsp.sh script (but only work for linux/mac for now, we could have one for windows)

Just my point of view, you certainly know what is the best for Calva :)

@bpringe
Copy link
Member Author

bpringe commented Feb 7, 2021

We would not include the binaries. I think Peter was suggesting if we don't have Calva download the binary for the user's OS, then we just use the jar by default, and if the user wants to use a graalvm binary it's up to them to download it and provide the path to it, in which case Calva would use it.

@bpringe
Copy link
Member Author

bpringe commented Feb 7, 2021

I'll have to look into using that download script, if a Windows one could exist too.

@ericdallo
Copy link
Contributor

ericdallo commented Feb 7, 2021

I see, looks an alternative indeed.

A windows script would undoubtedly work, but it'll need to be another file, like install-latest-clojure-lsp.bat, that would simply download the latest windows binary and unzip it

@bpringe
Copy link
Member Author

bpringe commented Feb 7, 2021

@PEZ What do you think? If we do download the binary, should we do it via Calva code so we can provide incremental progress indication (% complete), or should we use a script like Eric mentioned, and just provide non-incremental progress?

@ericdallo Does lsp-mode show the progress of the download, and if so, how? My only concern with showing non-incremental progress is that if the user's internet is slow or something, they can't see the progress % and may think something's broken if a spinner is just spinning, for example.

@ericdallo
Copy link
Contributor

Yeah, lsp-mode just show a Installing clojure-lsp server... but it could be improved to get the download status just like curl, get and other commands provide (I think even emacs provide a tool like that).
But yeah, if you wanna show progress, probably it'd be necessary to do that on Calva's side.

@PEZ
Copy link
Collaborator

PEZ commented Feb 7, 2021

Since Calva handles itself pretty well without clojure-lsp, maybe this downloading can be pretty opaque? It could be a button in the statusbar that reads ”Initializing clojure-lsp” which on hover displays a tooltip that says ”Some more Calva features will be enabled when this is done”. There could be the same throbber as we use on the indicator for when clojure-lsp is indexing.

As long as this process does not block the rest of Calva it can just go on while the user jacks in or whatever. A progress indicator would signal that you need to wait for something, when in fact you don't.

@ericdallo
Copy link
Contributor

Looks good, maybe just differencing between downloading and initializing clojure-lsp to avoid false positives regarding clijure-lsp startup time : 😅

@bpringe
Copy link
Member Author

bpringe commented Feb 7, 2021

Makes sense. Yeah, I think it can say "Downloading clojure-lsp" and then switch to "Initializing" when that begins. The download would basically only happen when we upgrade clojure-lsp or on initial install of Calva. Calva will check if the current binary is the version that is set to be used, and if so no download will occur.

@ericdallo
Copy link
Contributor

Looks great to me

@bpringe bpringe added this to To do in Brandon's Board via automation Feb 9, 2021
@PEZ PEZ removed the enhancement label Feb 10, 2021
@bpringe bpringe moved this from To do to In progress in Brandon's Board Feb 23, 2021
@bpringe bpringe moved this from In progress to To do in Brandon's Board Mar 2, 2021
@bpringe bpringe moved this from To do to In progress in Brandon's Board Mar 10, 2021
bpringe added a commit that referenced this issue Mar 12, 2021
Brandon's Board automation moved this from In progress to Done Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

3 participants