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

fix: EPERM error on windows installation #7

Merged
merged 10 commits into from
Dec 9, 2023
Merged

Conversation

MakakWasTaken
Copy link
Contributor

fix: EPERM error on windows installation

Moved the pull request from here: biomejs/biome#735

@nhedger
Copy link
Member

nhedger commented Nov 16, 2023

Thanks for reopening here @MakakWasTaken 🙏

src/main.ts Outdated Show resolved Hide resolved
@nhedger
Copy link
Member

nhedger commented Nov 16, 2023

Wondering if we should use storageUri instead of the temp dir to store the copy of the biome binary

The uri of a workspace specific directory in which the extension can store private state. The directory might not exist and creation is up to the extension. However, the parent directory is guaranteed to be existent. The value is undefined when no workspace nor folder has been opened.

https://code.visualstudio.com/api/references/vscode-api#ExtensionContext

@MakakWasTaken
Copy link
Contributor Author

Wondering if we should use storageUri instead of the temp dir to store the copy of the biome binary

The uri of a workspace specific directory in which the extension can store private state. The directory might not exist and creation is up to the extension. However, the parent directory is guaranteed to be existent. The value is undefined when no workspace nor folder has been opened.

https://code.visualstudio.com/api/references/vscode-api#ExtensionContext

Might be smart, would allow for a workspace-based tmp file.

src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
MakakWasTaken and others added 3 commits November 16, 2023 17:37
Co-authored-by: Nicolas Hedger <649677+nhedger@users.noreply.github.com>
Co-authored-by: Nicolas Hedger <649677+nhedger@users.noreply.github.com>
@nhedger nhedger linked an issue Nov 20, 2023 that may be closed by this pull request
1 task
@MakakWasTaken
Copy link
Contributor Author

Is there more missing on this PR?

@nhedger
Copy link
Member

nhedger commented Nov 20, 2023

Hey, I just want to make sure it works as expected on all platforms.

I'll test it this week and hopefully merge it before this weekend.

@MakakWasTaken
Copy link
Contributor Author

Alright, just wanted to make sure I didn't miss anything.

@nhedger
Copy link
Member

nhedger commented Nov 21, 2023

Currently, the code will copy the binary no matter where it comes from, but we only want to copy it when it comes from the project's dev dependencies.

Particularly, we do not want to copy the binary when:

  • the user is using the bundled biome binary
  • the binary has been configured with the lspBin setting
  • the binary path has been set with the DEBUG_SERVER_PATH env variable

@MakakWasTaken
Copy link
Contributor Author

I ended up adding a new property called workspaceDependency to the getServerPath function. An alternative way of doing this could be looking after node_modules/@biomejs or something like that in the path. There is however a possibility of users setting either the lspBin or DEBUG_SERVER_PATH to a path including this, which would mess up the result of the function.

@nhedger
Copy link
Member

nhedger commented Nov 27, 2023

I was able to confirm it works on macOS.

I'll be checking the other systems shortly.

I've slightly reworded some logs and added a missing await statement to prevent the extension from starting the server before the file is copied over.

@nhedger

This comment was marked as off-topic.

@nhedger

This comment was marked as off-topic.

@nhedger nhedger merged commit c39acd1 into biomejs:main Dec 9, 2023
3 checks passed
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.

🐛 EPERM: operation not permitted node_modules\@biomejs\cli-win32-x64\biome.exe
2 participants