Skip to content

Change the node LSP for the native binaries #7 #384

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

Merged
merged 49 commits into from
Mar 26, 2025

Conversation

AntoineGS
Copy link
Collaborator

@AntoineGS AntoineGS commented Mar 20, 2025

Since native binaries are now available (here) figured it would be best to use them as it means fewer dependencies (no need for node) and hopefully less overhead so better performance.
I included the binaries as part of the report though I am not sure that is ideal.
But since the LSP was already included I figured it's no different than before except now there are 5 versions.
Hopefully I got the os detection logic right, as I do not have access to most of the architectures to test.

Seems to fix #309 as well!

language server now expects to be run directly through language-server.js
the configuration supports overriding the default function as get_root_dir
fixes zbirenbaum#371
the configuration supports overriding the default function as get_root_dir
fixes zbirenbaum#371
@zbirenbaum
Copy link
Owner

Awesome work, thank you so much!! My one note is that I think it's pretty important for a project that is supplying prebuilt binaries to do so through an automated update script which pulls down the files directly or performs some setup step through a user command like :copilot install-deps or something. There was a recent instance of a plugin with malicious software:

https://www.reddit.com/r/neovim/comments/1j45stl/someone_wrote_malicious_code_in_the_neovim_plugin/?rdt=38437

If you want to go ahead and merge this as just this once feel free to, I see you have a ton of awesome PRs in flight and don't want to make too much extra work for you to get these in.

@AntoineGS
Copy link
Collaborator Author

Thank for the feedback! It makes complete sense to automate this through an update script for security reasons, I will work on that :P
Yeah I took a liking to the project as you can see 😂 It's well built and fun to maintain/add features.

@zbirenbaum
Copy link
Owner

zbirenbaum commented Mar 20, 2025

Thank for the feedback! It makes complete sense to automate this through an update script for security reasons, I will work on that :P Yeah I took a liking to the project as you can see 😂 It's well built and fun to maintain/add features.

No worries. I actually can't seem to download your branch. The binaries are way too huge for Lazy to handle in a timely manner and it looked like it was frozen for a solid minute at 100% resolving delta. I'm going to be firm that this should be done through a separate script which only downloads the binary for your platform

@AntoineGS
Copy link
Collaborator Author

AntoineGS commented Mar 22, 2025

@zbirenbaum I think I got it, the file gymnastics was trickier than I expected but it seems to have fixed another issue at the same time with the WSL authentication.
I have tested on both Linux and Windows and the files management seems OK!
Would you mind doing a test on that one?
Since that change is more core than others it would be ideal to have another person test it :)

Edit: I am not sure what the conflict is with master 😣, since I even merged master into it minutes ago.
If you see what it is let know.

@mirkolenz
Copy link

Thanks for your recent improvements to this repo! In case you haven't seen it already: It seems that setting the integration id is no longer necessary:

community/community#151719 (reply in thread)

Just wanted to let you know in case it's relevant for these upcoming changes in this PR.

@AntoineGS
Copy link
Collaborator Author

Thanks for the info!
I've been testing the upgrade to the new version (1.291) but some other APIs have changed so I have not yet committed it but it is on the way :P

@AntoineGS AntoineGS merged commit af15584 into zbirenbaum:master Mar 26, 2025
@AntoineGS AntoineGS deleted the copilot_binaries branch March 26, 2025 23:31
@ornicar
Copy link

ornicar commented Mar 27, 2025

Unfortunately this breaks functionality on NixOs. Copilot doesn't start, here's the contents of ~/.local/state/nvim/lsp.log:

[START][2025-03-27 09:49:34] LSP logging initiated
[ERROR][2025-03-27 09:49:34] .../vim/lsp/rpc.lua:770	"rpc"	"/home/thib/.local/share/nvim/lazy/copilot.lua/copilot/linux-x64/copilot-language-server-1.292.0"	"stderr"	"Could not start dynamically linked executable: /home/thib/.local/share/nvim/lazy/copilot.lua/copilot/linux-x64/copilot-language-server-1.292.0\nNixOS cannot run dynamically linked executables intended for generic\nlinux environments out of the box. For more information, see:\nhttps://nix.dev/permalink/stub-ld\n"

@mirkolenz
Copy link

You can set the lsp binary path yourself, this worked perfectly for me 😄 0e3585b

AntoineGS added a commit that referenced this pull request Mar 27, 2025
AntoineGS added a commit that referenced this pull request Mar 27, 2025
* Revert "fix: add wget fallback, prevent Copilot from re-attaching upon startup error"

This reverts commit 2a43872.

* Revert "fix: use vim built-in mkdir to create the directory"

This reverts commit 535ebb5.

* Revert "fix: remove -p in folder creation as it is not supported by some shells"

This reverts commit 996194e.

* Revert "chore: add logging"

This reverts commit 066193f.

* Revert "fix: attempt to resolve a "cookies must be enabled" error"

This reverts commit 0da7ebc.

* Revert "feat: add ability to set custom copilot binary"

This reverts commit 0e3585b.

* Revert "refactor: remove deprecated setEditorInfo API calls and follow new SDK (#406)"

This reverts commit 73dbc0e.

* Revert "feat: move to the official Copilot LSP binaries (#384)"

This reverts commit af15584.
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.

:Copilot auth popup never closes after inserting the one-time code on WSL2
4 participants