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

Finalise FileSystemProvider api #47475

Closed
11 of 12 tasks
jrieken opened this issue Apr 9, 2018 · 24 comments
Closed
11 of 12 tasks

Finalise FileSystemProvider api #47475

jrieken opened this issue Apr 9, 2018 · 24 comments
Assignees
Labels
api feature-request Request for new features or functionality on-testplan remote Remote system operations issues
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Apr 9, 2018

For April we want to finalise the FileSystemProvider API. This will be mostly simplifying things and fixing how we read/write data. This issue is a bit of a kitchen sink

  • support simple readFile and writeFile since that's easy to implement and since it covers most use-cases
  • support reading/writing in chunks to enable large file support in remote file systems. This will also include open/close calls and thereby make the server stateful -> Support reading/writing chunks in remote fs #41985
  • simplify delete, e.g. don't differentiate between file/folder on the API level
  • rename FileSystemProvider#move to FileSystemProvider#rename
  • add optional function FileSystemProvider#copy
  • polish enum type FileSystemProvide API should declare enums starting with 1 #35449
  • 💪 define errors/error-codes
  • readDir should return [name, stat] tuples, not [uri, stat] tuples
  • polish file system activation event, rename to onFileSystem, e.g. onFileSystem:foo
  • should file scheme providers (schemes) be registered in package.json?
  • add subscribe-style-functions for file watching, e.g have a watch(resource: Uri, opts:{ recursive:boolean}): Disposable function
  • support to signal that a fs is case-sensitive

API Evolution

In order prevent breakage of early adopters we work on a copy of the current proposed API. To prevent a lock-step-approach we will "version" the new API. See below

    export interface FileSystemProvider {
        // current api
    }

    export interface FileSystemProvider2 {
        _version: 1;
        // new api
    }

   export namespace workspace {
        export function registerFileSystemProvider(
          scheme: string, 
          fallback: FileSystemProvider, 
          provider: FileSystemProvider2 // <- new world
       ): Disposable;
    }
  • When we make changed to the FileSystemProvider2 the _version-type will change, e.g. from 1 to 2 etc.
  • The registerFileSystemProvider-function will check that the new provider implements the proper version of the latest, proposed API, e.g. _version: 2 and only then register that provider. Otherwise it will use the old, fallback, provider.

/cc @siegebell @bolinfest @hansonw

Sample

A sample, memory-based file system provider can be found here: https://github.com/Microsoft/vscode-extension-samples/blob/master/fsprovider-sample/

@jrieken jrieken self-assigned this Apr 9, 2018
@jrieken jrieken added api remote Remote system operations issues labels Apr 9, 2018
@jrieken jrieken added this to the April 2018 milestone Apr 9, 2018
jrieken added a commit that referenced this issue Apr 9, 2018
jrieken added a commit that referenced this issue Apr 9, 2018
jrieken added a commit that referenced this issue Apr 10, 2018
jrieken added a commit that referenced this issue Apr 11, 2018
jrieken added a commit that referenced this issue Apr 11, 2018
jrieken added a commit that referenced this issue Apr 11, 2018
jrieken added a commit that referenced this issue Apr 11, 2018
@IlyaBiryukov
Copy link

@jrieken How about a way to get the current SearchProvider, or the default one if none is registered? The scenario is that an extension wants to run find-in-files and get the results off the current workspace, including unsaved changes in the editor buffers.

@jrieken
Copy link
Member Author

jrieken commented Apr 12, 2018

@IlyaBiryukov the SearchProvider isn't part of the FileSystemProvider anymore. Both are now separated and will ship independently.

jrieken added a commit that referenced this issue Apr 13, 2018
jrieken added a commit that referenced this issue Apr 13, 2018
jrieken added a commit that referenced this issue Apr 13, 2018
@siegebell
Copy link

Great to see this progress!

  1. Rather than readFile and writeFile, how about providing an example FSP that implements open/close/read/write/etc. in terms of readFile and writeFile?
  2. Simplify delete: this does not simplify the implementation, so what's gained? Since vscode is the only client of the API, simplicity to implement should take precedence over simplicity to use.
  3. Could you go into more detail on planned changes for the file system activation event?
  4. Is the versioning only until the API is finalized/public?

@siegebell
Copy link

@jrieken I want to provide multiple file systems on the same scheme. But I'm not sure if this should be a feature request or if I should just implement the multiplexing myself...

  1. Feature: vscode multiplexes file system providers on the scheme and on the authority/hostname. Downside: seems odd to allow different extensions to potentially register for the same scheme.
  2. Extensions should each claim their own scheme and register just one provider that multiplexes filesystems (e.g. based on authority/hostname). Downside: redundant work; most of the extensions that provide file systems could end up reimplementing the same multiplexer.

Either way, vscode should (but does not currently) throw an error if registering a conflicting file system provider. The current behavior is to simply ignore subsequent registrations.

@jrieken
Copy link
Member Author

jrieken commented Apr 17, 2018

Rather than readFile and writeFile, how about providing an example FSP that implements open/close/read/write/etc. in terms of readFile and writeFile?

That is the plan but raw read/write isn't part of the proposed API yet. The internal variant is ready and can be found here: https://github.com/Microsoft/vscode/blob/f373a15de059b7ed4918797172eeea224a7a939c/src/vs/platform/files/common/files.ts#L206

Simplify delete: this does not simplify the implementation, so what's gained? Since vscode is the only client of the API, simplicity to implement should take precedence over simplicity to use.

Yeah, it will make our implementation simpler ;-). Internally, we just have delete and knowing what to delete would require another stat-call. This API is more loose but allows for more freedom, e.g for something like rm -rf or using something like a trash-can.

Could you go into more detail on planned changes for the file system activation event?

Just name-polish and depending on the scheme vs scheme+authority discussion more fine grained control.

Is the versioning only until the API is finalized/public?

We only have to allow for more rapid change without carnage. Plan is to remove it end of this week.

I want to provide multiple file systems on the same scheme. But I'm not sure if this should be a feature request or if I should just implement the multiplexing myself...

Yeah, tough question. Agreeing on throwing on double registration - that is something we miss today. On the one side knowing how to handle a certain scheme should be enough, e.g. all ftp-servers talk the same protocol and the other side each authority requires special handling (creds etc)... I wonder how often extensions author will run into this?

jrieken added a commit that referenced this issue Apr 23, 2018
jrieken added a commit that referenced this issue Apr 23, 2018
@jrieken jrieken closed this as completed Apr 23, 2018
@siegebell
Copy link

@jrieken small bug: vscode.d.ts declares watch as taking excludes, but exclude is actually passed.

@jrieken
Copy link
Member Author

jrieken commented Apr 26, 2018

Thanks, fixed that

@siegebell
Copy link

@jrieken what does it mean for writeFile to be given option {create: false, overwrite: false}?

@jrieken
Copy link
Member Author

jrieken commented Apr 26, 2018

The create flag means create in case the file doesn't exist and overwrite means fail when create is set but the the file exists (like wx+ in nodejs). Having both false means write to file but don't create file (like w).

I see how overwrite might be confusing because it's not clear what to overwrite - content vs file...

@siegebell
Copy link

So {create: false, overwrite: false} is equivalent to {create: false, overwrite: true}?

@siegebell
Copy link

@jrieken when will watch ever be given recursive: false?

(I'm using fb-watchman, which always does a recursive watch, unless I specify an expression to filter out results from subdirs).

@jrieken
Copy link
Member Author

jrieken commented Apr 26, 2018

Yeah, we might call it for files, not just folders. Today, the driver does two things: Call watch recursive and with user-defined-excludes (like /node_modules/) for every (workspace-)folder and watch all files that are open and not inside those folders.

@siegebell
Copy link

Sure, but recursive is a no-op for files, right?

@jrieken
Copy link
Member Author

jrieken commented Apr 26, 2018

Yep, it should be set to false then. So

  • for files watch(Uri.file('/path/to/file.js', { recursive: false, excludes: [] }); -> changes to that file
  • for folder watch(Uri.file('/path/to/folder', { recursive: true, excludes: [ **/node_modules/**, .git/**] }); -> changes inside that folder and inside folders of that folder etc
  • for folder watch(Uri.file('/path/to/folder', { recursive: false, excludes: [ **/node_modules/**, .git/**] }); -> changes only inside that folder (not recursive). We currently don't make use of that, but we might.

@siegebell
Copy link

@jrieken isCaseSensitive won't work if registering a single file system provider that multiplexes over multiple file systems. Either there should be finer-grained control of what hostnames/subdirectories are case sensitive or else vscode will have to handle multiplexing on e.g. hostnames.

@jrieken
Copy link
Member Author

jrieken commented Apr 27, 2018

hostnames/subdirectories are case sensitive or else vscode will have to handle multiplexing on e.g. hostnames.

Fair point. Today the option is more or less ignored, see #48258. I tend to keep the option simple and eventually support multiplexing in VS Code, e.g. accepting an object instead of a scheme for fs-providers (similar to document selectors), e.g. registerFileSystemProvider({ scheme: 'foo', authority: 'bi.ba.bum.com', path: /some/prefix' }, myProvider);

@siegebell
Copy link

@jrieken maybe add Unavailable/Busy to FileSystemError? E.g. when a remote filesystem cannot be loaded because the network is down.

@siegebell
Copy link

@jrieken It's not clear how symlinks should be handled. Do you support both symlinks to files and directories, or just files? (I.e. are FileType fields bit flags?) Should symlink directories be treated like normal directories?

@jrieken
Copy link
Member Author

jrieken commented Apr 30, 2018

It's not clear how symlinks should be handled.

Yeah, the symbol is additive to the type-flag. So, be either a file, folder, xor unknown and a symlink.

maybe add Unavailable/Busy to FileSystemError? E.g. when a remote filesystem cannot be loaded because the network is down.

Yeah, I like

@siegebell
Copy link

@jrieken Given a chain of symlinks, A -> B -> ... -> foo.txt, should the stats (is-file, is-directory, mtime, etc.) be from A, B, or foo.txt?

@bolinfest
Copy link
Contributor

I believe that in POSIX these flags are mutually exclusive in mode_t of struct stat. The caller effectively decides whether to traverse by using lstat() as opposed to stat().

The VS Code does not have to map 1:1 with these syscalls, but it should be explicit about what it expects.

jrieken added a commit that referenced this issue May 2, 2018
jrieken added a commit that referenced this issue May 2, 2018
jrieken added a commit that referenced this issue May 2, 2018
@jrieken
Copy link
Member Author

jrieken commented May 2, 2018

@jrieken Given a chain of symlinks, A -> B -> ... -> foo.txt, should the stats (is-file, is-directory, mtime, etc.) be from A, B, or foo.txt?

It should be the stat of foo.txt plus the information about it being a symlink. Yep, this isn't posix but tailored to our use-case. I have added doc-comment about that

jrieken added a commit that referenced this issue May 2, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality on-testplan remote Remote system operations issues
Projects
None yet
Development

No branches or pull requests

5 participants