Windows support #86
Windows support #86
Conversation
alexsaveliev
commented
Jan 13, 2017
alexsaveliev
commented
- dealing with Windows-specific paths, converting them to Unix style where needed.
- dealing with Windows-specific paths, converting them to Unix style where needed
Conflicts: langserver/fs.go langserver/handler.go langserver/symbol.go
- dealing with Unix and Windows-style root paths
| @@ -59,7 +59,7 @@ func goRangesToLSPLocations(fset *token.FileSet, nodes []*ast.Ident) []lsp.Locat | |||
|
|
|||
| func goRangeToLSPLocation(fset *token.FileSet, pos token.Pos, end token.Pos) lsp.Location { | |||
| return lsp.Location{ | |||
| URI: "file://" + fset.Position(pos).Filename, | |||
There was a problem hiding this comment.
why can't we return file URIs like we did before? What makes this incorrect on windows?
There was a problem hiding this comment.
On Windows, file URL uses triple slash, see https://en.wikipedia.org/wiki/File_URI_scheme#Windows
I'm trying to use the same function everywhere
There was a problem hiding this comment.
We also return triple slash on unix, since fset.Position(pos).Filename starts with /. I'm surprised we can't do this on windows.
How easy is it to get a windows environment I can SSH or RDP into? This change touches a lot of places and is the sort of thing that seems like it would break in future. I'd like to confirm it is necessary.
Relatedly, can't we change vfs.OS so it returns paths that just work?
There was a problem hiding this comment.
We also return triple slash on unix, since fset.Position(pos).Filename starts with /. I'm surprised we can't do this on windows.
On Windows, filename will be C:/foo/bar thus making file://C:/foo/bar while we need file:///C:/foo/bar
Also, I believe that refactoring to introduce common path2uri function is good as is :)
How easy is it to get a windows environment I can SSH or RDP into?
I'm using neat feature Appveyor https://www.appveyor.com/docs/how-to/rdp-to-build-worker/ to connect to build server via RDP, you have 1 hour to experiments. Hope this helps
Relatedly, can't we change vfs.OS so it returns paths that just work?
Won't be enough.vfs.OSmay return list of directory entries, for example, and thenbuild.Contextwill be responsible for constructing path elements...
There was a problem hiding this comment.
path2uri implementations typically need to add a leading slash if the path matches a windows path. That is typically done by testing against a drive letter followed by a colon as the first path segment.
There was a problem hiding this comment.
I feel like there must be a better way to do this, especially since we do everything through the VFS. Adding convertors inside code just feels wrong. The only place conversion should happen is when we receive or send a file path over LSP.
From reading this diff I'd expect you should be able to do the same. bctx.GOPATH and bctx.GOROOT likely need to go through some VFS conversion as well. I don't want to merge this in because sprinkling path conversion all over the codebase is just gonna lead to us constantly breaking windows support as we add features/update + it is harder to reason about + removes one of the main advantages of the VFS.
Can we not create something like vfs.OS, except it maps /c/foo to c:/foo on the fly? Could you try an approach like that? If not, I'll make an attempt at supporting it. If I can't work out a nice way, we can go for this approach.
| @@ -42,35 +41,29 @@ func (h *HandlerShared) OverlayBuildContext(ctx context.Context, orig *build.Con | |||
| ctxt := © | |||
|
|
|||
| ctxt.OpenFile = func(path string) (io.ReadCloser, error) { | |||
| return fs.Open(ctx, path) | |||
| return fs.Open(ctx, vfsPath(path)) | |||
There was a problem hiding this comment.
why does this wrapper need to exist? Shouldn't users of build context only be using virtual paths?
| "/go/src/p/a.go": "p", | ||
| "/go/src/p/a_test.go": "p_test", | ||
| bctx.GOPATH + "/src/p/a.go": "p", | ||
| bctx.GOPATH + "/src/p/a_test.go": "p_test", |
There was a problem hiding this comment.
why does this make a difference? Line 17 sets bctx.GOPATH to "/go"
|
closing in favor of #133 |
