Closed jcs090218 closed 7 months ago
As part of this PR, we would also get rid of the dist
folder and current chore
workflow no?
@emil-vdw We may need two steps:
@jcs090218
Could you also modify copilot--start-agent
to load the agent.js
from the new installing method?
It would be better if you check the currently installed version of the NPM package and send a warning if it is mismatched with copilot-version
.
Prioritize using the agent.js installed by the new method then the old dist folder. If a user is using the agent.js from the dist folder, send a deprecation warning and ask them to install it via the new method.
@emil-vdw After giving it another thought, I realized that this step can be skipped. It's okay to delete dist
after providing a new way to install them.
We may need two steps:
- Prioritize using the agent.js installed by the new method then the old dist folder. If a user is using the agent.js from the dist folder, send a deprecation warning and ask them to install it via the new method.
- Remove the dist folder and the chore workflow.
What if we prompt the user to install the server when it's not yet installed in the new location? This is much more straightforward IMO. 🤔 Then we can remove dist
and chore
workflow in this PR (or later).
Could you also modify copilot--start-agent to load the agent.js from the new installing method? It would be better if you check the currently installed version of the NPM package and send a warning if it is mismatched with copilot-version.
👍 Done.
@jcs090218 Very cool! You solved the long-term issue!
One last request: Could you test the new installation method on Windows? (In case you have a Windows machine available.)
Could you test the new installation method on Windows? (In case you have a Windows machine available.)
Actually, I need someone to test on Unix-like system (linux or macOS), because I'm using Windows now. 😂
I assumed it will work since the code are dragged directly from lsp-mode
.
- The reliability of the copilot-node-server npm package. Can we trust it to be available, up-to-date and not to be hijacked? Should we investigate whether to pull the agent distributables straight from the corresponding github tag of the official copilot.vim repo?
I don't know much about the upstream copilot.vim
, so I cannot suggest this. However, downloading npm packages seems very common in the Emacs world; use lsp-mode
and lsp-grammarly
as examples.
I'm not sure but those examples may be using official distributions (albeit through npm). The copilot vim is an official github owned (official copilot) repository so it can be relied upon. The npm package being installed by us through npm is not official and we should at least strongly consider pulling from the official distribution instead.
In my opinion there should be very good reasons for using the unofficial distribution instead.
Ah, okay. I thought copilot-node-server
is the official... How about publishing our own npm package? Like lsp-grammarly maintains its own grammarly-language-server. 🤔
I found the npm package copilot-node-server
from https://github.com/TerminalFi/LSP-copilot a long while ago, which is another unofficial Copilot plugin for Sublime. The NPM package is maintained at https://github.com/jfcherng/copilot-node-server.
Maybe we can use it for now and later switch to our own NPM package.
Maybe we can use it for now and later switch to our own NPM package.
Yeah, I second this. It would take me some time to do this. Plus, it would make this PR too big if it was implemented in the same PR. 😅
@jcs090218 I have added some commits to fix some bugs and support Linux. I don't have access to push to your branch though.
Merged! :)
Would you mind testing those changes on Windows again as well?
Would you mind testing those changes on Windows again as well?
Yeah, it works now! :)
I've made the changes to this PR. Can we merge this? 🤔
Shall we do the cleanup in a subsequent PR @jcs090218 @zerolfx?
I think it's fine to clean up in a subsequent PR. Feel free to merge :+1:.
this seems to install (or try to start) copilot server at the wrong location:
[stderr] node:internal/modules/cjs/loader:1152
[stderr] throw err;
[stderr] ^
[stderr]
[stderr] Error: Cannot find module 'c:\Users\simon\.emacs.d\.cache\copilot\node_modules\copilot-node-server\copilot\bin\copilot-node-server'
[stderr] at Module._resolveFilename (node:internal/modules/cjs/loader:1149:15)
[stderr] at Module._load (node:internal/modules/cjs/loader:990:27)
[stderr] at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
[stderr] at node:internal/main/run_main_module:28:49 {
[stderr] code: 'MODULE_NOT_FOUND',
[stderr] requireStack: []
[stderr] }
[stderr]
[stderr] Node.js v21.6.1
the extra copilot
beforebin
leads to the wrong directory. for ref, the server was installed to C:\Users\simon\.emacs.d\.cache\copilot\node_modules\copilot-node-server\bin\copilot-node-server
You are right. It's weird since I did applied the change but it some how got reverted. 🤔
I'm on it.
FYI, I've opened the fix in #267. However, I cannot merge PRs without other maintainer's approval. Therefore, the current workaround is to update copilot--server-executable
manually.
(setq copilot--server-executable (f-join copilot-install-dir "node_modules" "copilot-node-server" "bin" "copilot-node-server"))
When opening emacs with this PR I get the following it complains that I should use copilot-install-server
but when I try I get
Symbol's value as variable is void: package
Can you try toggle-debug-on-error
? Paste the backtrack here. Thanks!
Edit: make sure you've updated to the latest version.
Debugger entered--Lisp error: (void-variable package)
copilot-install-server()
funcall-interactively(copilot-install-server)
call-interactively(copilot-install-server record nil)
command-execute(copilot-install-server record)
execute-extended-command(nil "copilot-install-server" "install copilot")
funcall-interactively(execute-extended-command nil "copilot-install-server" "install copilot")
call-interactively(execute-extended-command nil nil)
command-execute(execute-extended-command)
Indeed, I get this with main but don't get it if I checkout right before this PR
I've made the fix in #268. Thanks for the bug report!
For #120.