feature: Run local coursier if available to avoid using bootstraped one - #1385
Conversation
9d28ed7 to
94ce9d6
Compare
| (process.platform === "win32" && p.endsWith(path.sep + "cs.bat")) | ||
| ); | ||
| if (possibleCoursier) { | ||
| const coursierVersion = spawnSync(possibleCoursier, ["version"]); |
There was a problem hiding this comment.
Not sure how to work around this, I can't find a way to chain ChildProcess since then is only available on Promise. Alternatively, we could make a spawn with a wait to make sure this doesn't take too long. Ideas?
There was a problem hiding this comment.
there is already dependency on promisify-child-process which provides a promisfied equivalents of child_process, did you check it?
There was a problem hiding this comment.
I think I did? Doing .then on ChildProcessPromise was only returning Promise<Output> and not sure of there is a way around it really.
There was a problem hiding this comment.
Maybe https://github.com/sindresorhus/execa help?
As long as we use raw child_process stuffs (without writing a wrapper) there's no way to do that (get stdout wrapped by Promise if success, and Promise failed if return code isn't 0).
There was a problem hiding this comment.
Doesn't look like it :/ We want to pipe the output from coursier to VS Code output, but it seems I can do it only via ChildProcessPromise. I tried returning a Promise, but that seems to have a wrong type in the actual javascript underneath:
906323d
There was a problem hiding this comment.
Och actually if I have an intermediate object it works, not 100% pretty, but feels ok.
509aba3 to
022a78c
Compare
tanishiking
left a comment
tanishiking
left a comment
There was a problem hiding this comment.
I left a few comments, but overall it looks good 👍
| } | ||
|
|
||
| export function validateCoursier(pathEnv: string): string | undefined { | ||
| const isWindows = process.platform === "win32"; |
There was a problem hiding this comment.
[note]
I thought this might fail to detect windows when it's 64bit system, but it looks like there's only win32 😄
https://nodejs.org/api/process.html#process_process_platform
There was a problem hiding this comment.
yep :P I checked the docs
| "-p", | ||
| ]; | ||
|
|
||
| const path = process.env["PATH"]; |
There was a problem hiding this comment.
I'm not sure this work for Windows (while CI tests pass), maybe it's Path instead of PATH on windows?
avajs/ava#3014
There was a problem hiding this comment.
PATH works, the CI actually checks this correctly. https://superuser.com/questions/441611/what-is-the-linux-equivalent-of-windows-path
| ], | ||
| "sourceMaps": true, | ||
| "outFiles": ["${workspaceRoot}/out/**/*.js"], | ||
| "outFiles": ["${workspaceRoot}/packages/metals-vscode/out/**/*.js"], |
There was a problem hiding this comment.
Why do we need these changes?
There was a problem hiding this comment.
otherwise it's not possible to start the extension inside VS Code, I can move it separate, but this is just a follow up from the PR that merged the language client
|
|
||
| export function validateCoursier(pathEnv: string): string | undefined { | ||
| const isWindows = process.platform === "win32"; | ||
| const possibleCoursier = pathEnv |
There was a problem hiding this comment.
Maybe we want to use https://github.com/otiai10/lookpath to find commands from PATH, this library deals well with the platform differences (and it takes care of PATHEXT to checking cs.bat and cs.exe)
There was a problem hiding this comment.
Hmm... I would add the library if we see any issue, but this seems to work correctly both in linux and windows.
There was a problem hiding this comment.
Also it seems that the alst release was in 2021 so I would rather tweak our logic if needed.
| (process.platform === "win32" && p.endsWith(path.sep + "cs.bat")) | ||
| ); | ||
| if (possibleCoursier) { | ||
| const coursierVersion = spawnSync(possibleCoursier, ["version"]); |
There was a problem hiding this comment.
Maybe https://github.com/sindresorhus/execa help?
As long as we use raw child_process stuffs (without writing a wrapper) there's no way to do that (get stdout wrapped by Promise if success, and Promise failed if return code isn't 0).
a2fc784 to
b665fdc
Compare
This might help out in corporate environements since it will require installing coursier only once and Metals will be able to use it. In later steps I will try to automatically download coursier or native image via metals if not available.
3 participants
This might help out in corporate environements since it will require installing coursier only once and Metals will be able to use it.
In later steps I will try to automatically download coursier or native image via metals if not available.
Continuation of scalameta/metals-languageclient#524