internal: migrate to vscode.FileSystem API #8951

wxb1ank · 2021-05-23T20:47:32Z

I encountered an error where bootstrap() attempts to create a directory with the path C:\C:\.... I couldn't find this reported anywhere else. Using the vscode.FileSystem API instead of the fs one works here. I assume the latter automatically prepends C:\ to paths whereas the former does not. I don't know if this suggests vscode.FileSystem should be used in more places for consistency.

matklad · 2021-05-23T21:04:54Z

yeah, ideally we should never use bare fs are there other places we might want to adjust this?

wxb1ank · 2021-05-23T21:26:38Z

Well, it looks like fs is used in main.ts, net.ts, and toolchain.ts, at least. I don't immediately see any situations where vscode.FileSystem would be insufficient. I'm AFK at the moment but can look into replacing those calls later.

I guess the question at this point is which API is more stable and if the VS Code one has drawbacks I'm not aware of. I admit I'm not very familiar with Node/TypeScript, so please correct me if I'm mistaken on anything :)

wxb1ank · 2021-05-24T02:52:10Z

I replaced some cases of low-hanging fruit using fs with vscode.FileSystem equivalents. As the VS Code API uses vscode.Uri for file paths (and not plain old strings), I also introduced some Uri.file() and Uri.joinPath() code to avoid Node's path module. I would have liked to completely eliminate the need for path and fs, but unfortunately:

vscode.Uri is kind of limited, and vscode.FileSystem lacks some functionality (e.g., createWriteStream()) of fs
vscode.Uri.file() prepends / to the given path, which is undesirable when referencing executables in the PATH

yume-chan · 2021-05-24T04:07:20Z

The root cause is that in 95c51d8

- this.globalStoragePath = ctx.globalStoragePath;
+ this.globalStoragePath = ctx.globalStorageUri.path;

Uri#path returns /C:/Users/XXX on Windows (because the Uri itself is file:///c:/Users/XXX.

The correct property should be Url#fsPath (https://code.visualstudio.com/api/references/vscode-api#details-418), it returns c:\Users\XXX on Windows (and it should also work on Linux).

I'm not familiar with new APIs like vscode.FileSystem thus don't know which one is better.

wxb1ank · 2021-05-24T04:46:54Z

Thanks for clarifying, @yume-chan. As I was exploring the VS Code API I noticed that deprecation, but I didn't realize what exactly the cause for the path error was.

At the end of the day, the simplest solution would be to rollback my commits and replace path with fsPath; that would at least accomplish the original goal of this PR. However, looking through the VS Code extension code, I think the way we're using raw strings to represent paths and importing the Node fs module for FS operations is a bit messy. At the same time, I don't know if replacing fs with vscode.FileSystem will be a regression in this case. Perhaps a wrapper that uses the VS Code API as a base and draws from Node when necessary is warranted?

yume-chan · 2021-05-24T05:34:02Z

I thought vscode.FileSystem was added for Language Server type extensions to support virtual file systems, but the extension global store in this case should always be on the local disk, so Node.js fs module will work.

However when reading the documentation more carefully, I found this:

> globalStorageUri: Uri

[...]

see - workspace.fs for how to read and write files and folders from an uri.

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

So maybe Code expects extensions to use vscode.FileSystem to operate on globalStorageUri.

Closes rust-lang#8956 See rust-lang#8951 (comment)

Closes #8956 See #8951 (comment)

wxb1ank · 2021-05-24T11:07:24Z

Personally, I think Uri.fsPath is still a 'correct' way to do this, documentation wise. I found this in Uri.fsPath:

The resulting string shall not be used for display purposes but for disk operations, like readFile et al.

I didn't previously consider your comment about virtual file systems, but seeing as globalStorageUri is always local, I think this solution is fine for now.

matklad · 2021-05-24T11:10:27Z

I've pushed a fix to just use fsPath, which fixed the immediate issue. However, switching from fs to vscode.fs is something we need to do as well, so it's best to rebase this PR on master!

Thanks a bunch for figuring this all out @wxb1ank and @yume-chan, it was instrumental in preparing the quick fix for todays release!

wxb1ank · 2021-05-25T15:26:27Z

I found a vscode-python issue that documents their transition from Node's fs to vscode.FileSystem: microsoft/vscode-python#8533. I think this proves that the latter can (mostly) replace the former. I will rebase and look into this later this week.

wxb1ank · 2021-06-02T18:00:01Z

Sorry it took me a bit. Coming back to this, it looks like only net.ts uses the fs module now. Unfortunately, I still don't see a good solution to this: AFAIK, vscode.FileSystem can't modify file permissions (.e.g., make the downloaded executable rx), nor does it provide a suitable alternative to fs.createWriteStream() (necessary for storing gunzip streams*).

I've linted and rebased, and I think this is ready for review again.

*We also need a way to store non-gunzipped (gzipped?) streams. node-fetch conveniently provides Response.buffer() for this.

matklad · 2021-06-14T10:29:56Z

bors r+

Let's see if this breaks anything :D

bors · 2021-06-14T10:36:46Z

Build succeeded:

lnicola · 2021-06-14T10:45:09Z

editors/code/src/toolchain.ts


-            if (isFile(standardPath)) return standardPath;
+            if (isFile(standardPath.path)) return standardPath.path;


We convert an Uri to a string here, then convert it back to an Uri. And I'm not sure what happens if the path doesn't exist -- is there a way to require("vscode") in the Developer Tools?

is there a way to require("vscode") in the Developer Tools?

I would expect not. The developer tools run inside the chromium based part of electron. The extensions run in the node.js based part.

@wxb1ank I haven't looked at the code outside of the diff, but do you think this can be simplified?

lnicola · 2021-06-15T07:58:25Z

editors/code/src/toolchain.ts

-        return false;
-    }
+async function isFile(path: string): Promise<boolean> {
+    return ((await vscode.workspace.fs.stat(vscode.Uri.file(path))).type & vscode.FileType.File) !== 0;


What happens here if the file doesn't exist?

If I had to guess... it throws. (Note: that's for FileSystemProvider, not FileSystem, but I'm assuming the latter is a wrapper over the former?) This should definitely be wrapped in a try/catch block ASAP.

I'm not very proud of my implementation for this function, either; I wanted to maintain the original function signature (i.e. with a string path, not Uri) so that it could be reused in lookupInPath().

Looking at it now, I think it would be best to separate isFile() into two functions: one that takes a string path, and one that takes a Uri. The former should be a simple wrapper over the latter, translating the string into a Uri with Uri.file(path).

This PR has already been merged, so tomorrow (later today?) I can try and clean this up in a new one.

9292: fix: Code: clean-up #8951 r=wxb1ank a=wxb1ank #8951 was a major change in the VS Code extension and caused quite a few problems. This PR is a catch-all for bugs and improvements in the new code. This should fix: - #9284 - [this unreported bug](https://github.com/rust-analyzer/rust-analyzer/pull/8951/files#r651570446) - ...and one or two uncaught exceptions I just found The original lack of testing was my own fault, but this area of the VS Code API is also tricky for a couple reasons: - The [FileSystem](https://github.com/rust-analyzer/rust-analyzer/pull/8951/files#r651570446) API does not list or warn about any exceptions, but [FileSystemProvider](https://github.com/rust-analyzer/rust-analyzer/pull/8951/files#r651570446) (which `FileSystem` is a wrapper of, AFAICT) does. - At first glance, [Uri.path](https://github.com/rust-analyzer/rust-analyzer/pull/8951/files#r651570446) *looks* like it works for FS operations. It does not, at least, on Windows. You need to use `Uri.fsPath`. I only use Windows, so I need people on macOS, Linux, and (possibly) NixOS to test this. Co-authored-by: wxb1ank <[email protected]>

Veykril mentioned this pull request May 23, 2021

RA 2021-23-05 can't bootstrap on Windows #8956

Closed

matklad added a commit to matklad/rust-analyzer that referenced this pull request May 24, 2021

fix: can download server binary on windows again

df14634

Closes rust-lang#8956 See rust-lang#8951 (comment)

lnicola pushed a commit that referenced this pull request May 24, 2021

fix: can download server binary on windows again

3022a2c

Closes #8956 See #8951 (comment)

wxb1ank added 3 commits June 2, 2021 12:07

internal: use vscode.FileSystem API in main.ts

3ca7f61

migrate from fs to vscode.FileSystem API

0448b73

Lint toolchain.ts

388a194

wxb1ank changed the title ~~fix: use vscode.FileSystem API for globalStorage~~ internal: migrate to vscode.FileSystem API Jun 2, 2021

bors bot merged commit 388a91c into rust-lang:master Jun 14, 2021

lnicola reviewed Jun 14, 2021

View reviewed changes

This was referenced Jun 15, 2021

Bootstrap error [TypeError: Only absolute URLs are supported #9280

Closed

fix: Code: use Uri.toString() for URLs #9281

Merged

lnicola reviewed Jun 15, 2021

View reviewed changes

wxb1ank mentioned this pull request Jun 15, 2021

Cannot Update/Bootstrap rust-analyzer in Visual Studio Code #9284

Closed

wxb1ank added a commit to wxb1ank/rust-analyzer that referenced this pull request Jun 15, 2021

fix: clean-up rust-lang#8951

56e128a

wxb1ank mentioned this pull request Jun 15, 2021

fix: Code: clean-up #8951 #9292

Merged


		if (isFile(standardPath)) return standardPath;
		if (isFile(standardPath.path)) return standardPath.path;

internal: migrate to vscode.FileSystem API #8951

internal: migrate to vscode.FileSystem API #8951

Uh oh!

Conversation

wxb1ank commented May 23, 2021

Uh oh!

matklad commented May 23, 2021

Uh oh!

wxb1ank commented May 23, 2021

Uh oh!

wxb1ank commented May 24, 2021

Uh oh!

yume-chan commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxb1ank commented May 24, 2021

Uh oh!

yume-chan commented May 24, 2021

Uh oh!

wxb1ank commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented May 24, 2021

Uh oh!

wxb1ank commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxb1ank commented Jun 2, 2021

Uh oh!

matklad commented Jun 14, 2021

Uh oh!

bors bot commented Jun 14, 2021

Uh oh!

lnicola Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

bjorn3 Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

lnicola Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

lnicola Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

wxb1ank Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yume-chan commented May 24, 2021 •

edited

Loading

wxb1ank commented May 24, 2021 •

edited

Loading

wxb1ank commented May 25, 2021 •

edited

Loading