-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: check for installed serve-d version + improve get.serve-d #40
Conversation
src/get-serve-d.ts
Outdated
import decompressTarxz from "decompress-tarxz" | ||
|
||
// function to download serve-d binaries from GitHub | ||
export async function getServeD() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think nightly & pre-release support would match the vscode extension well
src/get-serve-d.ts
Outdated
/* repo */ "serve-d", | ||
/* download folder */ distFolder, | ||
/* filter release */ undefined, | ||
/* filter asset */ (asset) => asset.name.indexOf(platform) >= 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test should probably be a little more exact (at least also include the architecture)
you might want to copy the function from https://github.com/Pure-D/code-d/blob/master/src/installer.ts#L261 (first argument is release name / release version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But do you build for other architectures? If you don't, there is no need to add code for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't build for other architectures yet, but vscode is supported on ARM, so if there is an ARM user (e.g. coders on cheap linux ARM laptops or raspberry pi) they don't download a release that is unsupported anyway.
It probably makes sense to move all the release downloading & manual compilation into an NPM package which can be used by all JS based extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows installation fails because tarxz runs node-gyp and this requires Visual Studio Installation.
I think it would be easier to bundle serve-d with the package
on windows the releases are zips, there is no need for tarxz. In code-d I use AdmZip to unzip these releases. For .tar.xz I invoke |
"@terascope/fetch-github-release" itself supports extracting zip. The decompress plugin is only needed for tarxz. I wish the assets were consistent, and they all used zip. |
the releases are currently more or less following what is usually used on each platform, I believe that makes the most sense for end users manually downloading releases. The code in code-d for extracting tar.xz & tar.gz is fairly simple, would it be a solution to use that code instead of the decompress plugin? https://github.com/Pure-D/code-d/blob/master/src/installer.ts#L441-L462 |
How do you check if serve-d is up to date? |
I feel like this approach is just more complex for no reason (e.g. all these manual downloading, decompressing, checking for versions, etc). The better solution is to make an npm package for serve-d, so it can be easily installed as a dependency. For now, I am going to bundle serve-d exe as part of the package. |
3d0d811
to
2e587b5
Compare
f29aa32
to
0e46811
Compare
No description provided.