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

x/tools/godoc/vfs: NameSpace.bind not working on Windows #18629

Closed
alexsaveliev opened this issue Jan 12, 2017 · 10 comments
Closed

x/tools/godoc/vfs: NameSpace.bind not working on Windows #18629

alexsaveliev opened this issue Jan 12, 2017 · 10 comments

Comments

@alexsaveliev
Copy link

@alexsaveliev alexsaveliev commented Jan 12, 2017

What version of Go are you using (go version)?

go version go1.7 windows/amd64

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\projects\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=.....=/tmp/go-build -gno-record-gcc-
switches
set CXX=g++
set CGO_ENABLED=1

What did you do?

Hello, I think that there is a missing way to set up a vfs.NameSpace filesystem backed up by Windows filesystem without specific root to be able to:

ns.Open("C:\\foo\\bar") // gives content of C:\foo\bar
ns.Open("D:\\qux") // gives content of D:\qux

Why do I need it? Imagine a local file system backed by in-memory map (pretty common scenario in language server protocol where some files may be read directly from disk and some may be provided by a client while being edited). Sometimes it's impossible to predict what parts of local file system may be needed and caller may be unable to declare all the potential mount points.

fs.Bind("/", vfs.OS("/"), "/", vfs.BindBefore) does not work on Windows because PATH being resolved into \PATH and Windows FS is unable to access such a file.

I think it's may be worth to add such a possibility and resolve paths on Windows specific way (remove "" prefix if there is any). What do you think?

Test case that fails on Windows

package vfs_test

import (
	"io/ioutil"
	"os"
	"path/filepath"
	"testing"

	"golang.org/x/tools/godoc/vfs"
)

func TestOpen(t *testing.T) {

	tmpDir, err := ioutil.TempDir("", "vfs-localfs")
	if err != nil {
		t.Fatal(err)
	}
	defer os.RemoveAll(tmpDir)
	if err = os.MkdirAll(tmpDir, 0700); err != nil {
		t.Fatal(err)
	}
	file := filepath.Join(tmpDir, "foo")
	if err = ioutil.WriteFile(file, []byte("bar"), 0600); err != nil {
		t.Fatal(err)
	}

	fs := vfs.NameSpace{}
	fs.Bind("/", vfs.OS("/"), "/", vfs.BindAfter)

	_, err = fs.Open(file)
	if err != nil {
		t.Errorf("Cannot read local file (%s)", err)
	}
}

I have a pull request in mind, if needed.

Sincerely,
Alex

@mattn
Copy link
Member

@mattn mattn commented Jan 12, 2017

You should call fs.Bind with specifying second argument vfs.OS(tmpdir).

package vfs_test

import (
	"io/ioutil"
	"os"
	"path/filepath"
	"testing"

	"golang.org/x/tools/godoc/vfs"
)

func TestOpen(t *testing.T) {

	tmpDir, err := ioutil.TempDir("", "vfs-localfs")
	if err != nil {
		t.Fatal(err)
	}
	defer os.RemoveAll(tmpDir)
	if err = os.MkdirAll(tmpDir, 0700); err != nil {
		t.Fatal(err)
	}
	file := filepath.Join(tmpDir, "foo")
	if err = ioutil.WriteFile(file, []byte("bar"), 0600); err != nil {
		t.Fatal(err)
	}

	fs := vfs.NameSpace{}
	fs.Bind("/", vfs.OS(tmpDir), "/", vfs.BindAfter)

	_, err = fs.Open("/foo")
	if err != nil {
		t.Errorf("Cannot read local file (%s)", err)
	}
}

The path to call fs.Open should be the virtual path, not physical.

@alexsaveliev
Copy link
Author

@alexsaveliev alexsaveliev commented Jan 12, 2017

@mattn

You should call fs.Bind with specifying second argument vfs.OS(tmpdir).

I think that your proposal solves only particular case, where I assume that all files I will work with are located on the same logical disk, e.g. C:\Users\alexsaveliev\AppData\Local\Temp\. But if there is a need to access any file on any logical disk, there is currently no way to make all of them accessible without iterating over them and binding C:\ => C:\, D:\ => D:\, and so on which I find inconvenient, because on Unix it's enough to bind / => / and you'll see all the files on all partitions.

Is there a way to call fs.Bind once to make the following snippet work?

fs.Bind(?)
fs.Open("C:/foo");
fs.Open("D:/foo");
fs.Open("E:/foo");
...

will not work.

The path to call fs.Open should be the virtual path, not physical.

It is. My tests says fs.Open("/foo") - I expect to open virtual file /foo located somewhere in temp dir on local FS

Thanks

@mattn
Copy link
Member

@mattn mattn commented Jan 12, 2017

Then, you can call ns.Bind for each drives.

package main

import (
	"io"
	"log"
	"os"

	"golang.org/x/tools/godoc/vfs"
)

func main() {
	fs := vfs.NameSpace{}
	fs.Bind("/C", vfs.OS("C:/"), "/", vfs.BindAfter)
	fs.Bind("/D", vfs.OS("D:/"), "/", vfs.BindAfter)
	fs.Bind("/E", vfs.OS("E:/"), "/", vfs.BindAfter)

	file, err := fs.Open("/C/Windows/System32/drivers/etc/hosts")
	if err != nil {
		log.Fatalf("Cannot read local file (%s)", err)
	}
	io.Copy(os.Stdout, file)
}
@mdlayher mdlayher changed the title golang.org/x/tools/godoc/vfs on Windows x/tools/godoc/vfs: NameSpace.bind not working on Windows Jan 12, 2017
@bradfitz bradfitz added this to the Unreleased milestone Jan 13, 2017
@alexsaveliev
Copy link
Author

@alexsaveliev alexsaveliev commented Jan 13, 2017

@mattn

Thanks for the suggestion, will do it this way

@keegancsmith
Copy link
Contributor

@keegancsmith keegancsmith commented Jan 13, 2017

@alexsaveliev I don't have a windows machine to test on, but this may possibly work without having to mount each drive:

fs := vfs.NameSpace{}
fs.Bind("/", vfs.OS(""), "/", vfs.BindAfter)
@mattn
Copy link
Member

@mattn mattn commented Jan 13, 2017

@keegancsmith No, doesn't work. Because

file, err := fs.Open("C:/Windows/System32/drivers/etc/hosts")

the path is not absolute in VFS path. So the vfs will access /C:/Windows/System32/drivers/etc/hosts.

@keegancsmith
Copy link
Contributor

@keegancsmith keegancsmith commented Jan 13, 2017

@mattn given that fs is a vfs.NameSpace shouldn't you be using virtual paths. This instead

file, err := fs.Open("/C:/Windows/System32/drivers/etc/hosts")
@mattn
Copy link
Member

@mattn mattn commented Jan 14, 2017

@keegancsmith doesn't work because it call

os.Open(`\C:\Windows\System32\drivers\etc\hosts`)
@keegancsmith
Copy link
Contributor

@keegancsmith keegancsmith commented Jan 26, 2017

@mattn do you think we could adjust the vfs.OS implementation for windows such that resolve does the "right" thing for vfs.OS("")?

@mattn
Copy link
Member

@mattn mattn commented Jan 26, 2017

Sorry, I don't think that it is "right".

@golang golang locked and limited conversation to collaborators Jan 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.