|
|
|
go.tools: godoc/vfs -> buildfs
Turn godoc's vfs into a general build system fs for go.tools.
Major API changes:
- Add a Context method buildfs.FileSystem that returns
a *build.Context. This lets programs carry around a
single buildfs.FileSystem containing all file context.
- gcimporter no longer automatically hooks itself up to
the types package, instead it must be initialized. This
is because all tools must add their own FileSystem to
gcimporter, if those tools are to be easily extended with
a custom file system.
Suggested reading order:
1. buildfs/vfs.go - new method on FileSystem
2. go/gcimporter/gcimporter.go - manual initialization
3. go/types/api.go - no major changes
4. cmd/vet/main.go - a simple worked example
5. everything else is just deck chair rearrangement
This CL can be divided into smaller components, but if it
is it becomes hard to see the purpose of the API changes.
Patch Set 1 #Patch Set 2 : diff -r 139f5d7a1e02 https://code.google.com/p/go.tools #Patch Set 3 : diff -r 139f5d7a1e02 https://code.google.com/p/go.tools #
Total comments: 9
Patch Set 4 : diff -r 139f5d7a1e02 https://code.google.com/p/go.tools #
Total comments: 6
Patch Set 5 : diff -r 32340194fa90 https://code.google.com/p/go.tools #
Total comments: 18
Total messages: 4
|
adonovan
LGTM This all seems pretty straightforward. I'm sure gri will have some suggestions about the ...
|
12 years ago (2013年12月17日 22:26:38 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
LGTM This all seems pretty straightforward. I'm sure gri will have some suggestions about the getting the gcimporter API right. Please add package documentation to buildfs explaining the theory of how to use it. For example, there are now effectively two kinds of filenames. Ordinary POSIX filenames which work with system calls, and "build" filenames that only work relative to the instance of BuildFS in use. Applications need to exercise proper hygiene to make sure they don't use build filenames in system calls, and they may need to call utility functions to convert build filenames into something appropriate for printing out (and presumably such conversions can fail). In particular, the file name that a token.FileSet associates with an ast.Node is an internal names, coming from build.Import, so it's not ok to print it using fset.Position(pos).String() in the user interface. https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go File buildfs/build.go (right): https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go#newcode20 buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err error) { return fs.ReadDir(dir) } Newlines wouldn't hurt. https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go File buildfs/build.go (right): https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go#newcode20 buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err error) { return fs.ReadDir(dir) } Newlines wouldn't hurt here and below. https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go File buildfs/httpfs/httpfs.go (right): https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go#n... buildfs/httpfs/httpfs.go:48: func (h *httpFS) Context() *build.Context { return h.fs.Context() } Move closer to the other *httpFS method. https://codereview.appspot.com/43490043/diff/60001/go/types/api.go File go/types/api.go (right): https://codereview.appspot.com/43490043/diff/60001/go/types/api.go#newcode111 go/types/api.go:111: // The package The quoted code is still a declaration.
This still needs the path translation functions we have discussed (and some more documentation), but the tests now pass and this hopefully captures the general intention of this CL. https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go File buildfs/build.go (right): https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go#newcode20 buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err error) { return fs.ReadDir(dir) } On 2013年12月17日 22:26:39, adonovan wrote: > Newlines wouldn't hurt. Done. https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go File buildfs/build.go (right): https://codereview.appspot.com/43490043/diff/60001/buildfs/build.go#newcode20 buildfs/build.go:20: ctx.ReadDir = func(dir string) (fi []os.FileInfo, err error) { return fs.ReadDir(dir) } On 2013年12月17日 22:26:39, adonovan wrote: > Newlines wouldn't hurt here and below. Done. https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go File buildfs/httpfs/httpfs.go (right): https://codereview.appspot.com/43490043/diff/60001/buildfs/httpfs/httpfs.go#n... buildfs/httpfs/httpfs.go:48: func (h *httpFS) Context() *build.Context { return h.fs.Context() } On 2013年12月17日 22:26:39, adonovan wrote: > Move closer to the other *httpFS method. Done. https://codereview.appspot.com/43490043/diff/60001/go/types/api.go File go/types/api.go (right): https://codereview.appspot.com/43490043/diff/60001/go/types/api.go#newcode111 go/types/api.go:111: // The package On 2013年12月17日 22:26:39, adonovan wrote: > The quoted code is still a declaration. Done.
> This still needs the path translation functions we have discussed (and some more documentation) For a change like this which comes with a "theory", it's not a bad idea to start with the documentation and at least a brief explanation of how the code yet to be written will work: e.g. the path conversion functions, and challenging parts such as guessBuildPackage. As you point out, the mechanical renamings are distracting during the review, so it's fine to omit them from the CL initially. We can't commit the change until we're confident the rest of it can in fact be implemented. The code looks like a reasonable implementation of the buildfs concept, but I'm worried that we're committing ourselves to a future of "incomplete virtualization" problems. We need to: - audit all of these applications for all direct access to the file system (via os, syscall, os/exec, etc) and for all places where an implicit conversion occurs between an "openable" file path and a file path that is only meaningful w.r.t. some buildfs.FileSystem instance. - make sure that for each string variable containing a path, that there is not doubt which of these two kinds of path it is. - avoid calling parse.ParseFile directly. It will need to be wrapped so that it is provided both the logical (not necessarily openable) filename to associate with the file as well as a Reader for the actual bits (which will need to be closed). and we need to keep do all of these things as the code evolves. What I'd really like to see is an end-to-end demo of one of the tools (e.g. oracle) using a file system implementation that does the path remapping necessary to read from (e.g.) Google's proprietary build system. Though this wouldn't be exercised by the existing oracle tests, it would help convince us that the path name problems are manageable. https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go File buildfs/build.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go#newcode13 buildfs/build.go:13: // BuildFS modifies a Context to use the given FileSystem, s/BuildFS/Build/ It doesn't "modif[y] a Context" (it's passed by value), it "returns a new FileSystem using a Context based on ctx, modified to use the given FileSystem". https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go#newcode30 buildfs/build.go:30: fs FileSystem If you make this field anonymous, you can lose some of the method declarations below, I think. https://codereview.appspot.com/43490043/diff/80001/buildfs/build.go#newcode47 buildfs/build.go:47: func (defaultFS) String() string { return "OS FS" } "defaultFS"? https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go File buildfs/gatefs/gatefs.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go#n... buildfs/gatefs/gatefs.go:20: func New(fs buildfs.FileSystem, gateCh chan bool) buildfs.FileSystem { Do callers send/recv on gateCh too? If not, all you're getting here is a number, cap(gateCh). https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go#n... buildfs/gatefs/gatefs.go:27: type gate chan bool chan struct{} ? https://codereview.appspot.com/43490043/diff/80001/buildfs/gatefs/gatefs.go#n... buildfs/gatefs/gatefs.go:29: func (g gate) enter() { g <- true } Is this the most efficient implementation of a counting semaphore in Go? https://codereview.appspot.com/43490043/diff/80001/buildfs/httpfs/httpfs.go File buildfs/httpfs/httpfs.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/httpfs/httpfs.go#n... buildfs/httpfs/httpfs.go:5: // Package httpfs implements http.FileSystem using a godoc buildfs.FileSystem. s/godoc // https://codereview.appspot.com/43490043/diff/80001/buildfs/mapfs/mapfs.go File buildfs/mapfs/mapfs.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/mapfs/mapfs.go#new... buildfs/mapfs/mapfs.go:13: pathpkg "path" I assume you're intentionally avoiding "path/filepath" since you don't want its file system notions to creep into purely syntactical operations. https://codereview.appspot.com/43490043/diff/80001/buildfs/namespace.go File buildfs/namespace.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/namespace.go#newco... buildfs/namespace.go:126: return fs.fs.Context() This scares me. You are assuming that all contexts of all filesystems are identical and you're choosing one nondeterministically on each call. If you believe the assumption is sound, assert it. If not---and I think it is not---this will need more thought. https://codereview.appspot.com/43490043/diff/80001/buildfs/os.go File buildfs/os.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/os.go#newcode27 buildfs/os.go:27: func (root osFS) Context() *build.Context { return &build.Default } How would a user specify a non-default build.Context? (For a concrete use-case, note how importer/util.go disables cgo in its context.) https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go File buildfs/vfs.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go#newcode21 buildfs/vfs.go:21: // The FileSystem interface specifies the methods godoc is using // A FileSystem defines a read-only view of a file tree. // Implementations may be based on the OS's file system, // or alternatives such as archive files, in-memory data. https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go#newcode23 buildfs/vfs.go:23: type FileSystem interface { Can you comment on the "rootedness" of this abstraction? i.e. are all paths relative to some implicit root, or can they be absolute too? What about uplevel references (../x)? https://codereview.appspot.com/43490043/diff/80001/buildfs/vfs.go#newcode28 buildfs/vfs.go:28: String() string No need for String() here. https://codereview.appspot.com/43490043/diff/80001/buildfs/zipfs/zipfs.go File buildfs/zipfs/zipfs.go (right): https://codereview.appspot.com/43490043/diff/80001/buildfs/zipfs/zipfs.go#new... buildfs/zipfs/zipfs.go:81: func (fs *zipFS) Context() *build.Context { return &build.Default } What does it mean for a zipFS to have a build.Context? Are there any meaningful interactions between the two at all? I'm concerned that an application supplying a non-default build.Context will find its preference ignored because several other FSs use &build.Default and the Namespace.Context() function returns an arbitrary one. https://codereview.appspot.com/43490043/diff/80001/cmd/gotype/gotype.go File cmd/gotype/gotype.go (right): https://codereview.appspot.com/43490043/diff/80001/cmd/gotype/gotype.go#newco... cmd/gotype/gotype.go:27: fs = buildfs.Default I'm having flashbacks to Blaze, in which we built a similar layered virtual filesystem. It was a constant battle to stop programmers writing library routines that called on the global buildfs.Default instance (or the standard file I/O library) to interpret some filename string, without awareness of the risk involved. Effectively we're asking them to forego a bunch of basic services and instead always plumb the correct FS all the way through their program. https://codereview.appspot.com/43490043/diff/80001/importer/importer.go File importer/importer.go (right): https://codereview.appspot.com/43490043/diff/80001/importer/importer.go#newco... importer/importer.go:66: fs buildfs.FileSystem // client's file system FS? I think you'll need to expose this since it's the effective file system in use. Config.FS may be nil. https://codereview.appspot.com/43490043/diff/80001/importer/importer.go#newco... importer/importer.go:113: FS buildfs.FileSystem You'll need to merge this with CL 48770043 when it goes in. https://codereview.appspot.com/43490043/diff/80001/oracle/oracle.go File oracle/oracle.go (right): https://codereview.appspot.com/43490043/diff/80001/oracle/oracle.go#newcode280 oracle/oracle.go:280: _, importPath, err := guessImportPath(fqpos.fset.File(fqpos.start).Name(), impcfg.FS.Context()) guessImportPath makes direct access to the file system. It will need rethinking to use the buildfs. There may be other such functions too, but this one is interesting because it's a candidate utility for moving closer to go/build: it answers the query: given an openable filename, which (GOPATH entry, package, package-relative path) does it belong to?
https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go File buildfs/build.go (right): https://codereview.appspot.com/43490043/diff/40001/buildfs/build.go#newcode26 buildfs/build.go:26: fs FileSystem embed FileSystem and then you don't need (some of) the manual forwarders below ? https://codereview.appspot.com/43490043/diff/40001/buildfs/httpfs/httpfs.go File buildfs/httpfs/httpfs.go (right): https://codereview.appspot.com/43490043/diff/40001/buildfs/httpfs/httpfs.go#n... buildfs/httpfs/httpfs.go:5: // Package httpfs implements http.FileSystem using a godoc buildfs.FileSystem. remove reference to godoc? https://codereview.appspot.com/43490043/diff/40001/buildfs/vfs.go File buildfs/vfs.go (right): https://codereview.appspot.com/43490043/diff/40001/buildfs/vfs.go#newcode16 buildfs/vfs.go:16: // The FileSystem interface specifies the methods godoc is using this is not godoc-specific, eventually, I assume? https://codereview.appspot.com/43490043/diff/40001/cmd/gotype/gotype.go File cmd/gotype/gotype.go (right): https://codereview.appspot.com/43490043/diff/40001/cmd/gotype/gotype.go#newco... cmd/gotype/gotype.go:241: types.DefaultImport = gcimporter.Importer(fs, "") The global DefaultImport is a really bad hack to avoid cycles in common cases. Since there's an explicit Config provided (line 191), instead you should set the Config.Import field. https://codereview.appspot.com/43490043/diff/40001/cmd/vet/main.go File cmd/vet/main.go (right): https://codereview.appspot.com/43490043/diff/40001/cmd/vet/main.go#newcode103 cmd/vet/main.go:103: types.DefaultImport = gcimporter.Importer(fs, "") Should probably also use a Config when calling types.Check. https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.go File go/gcimporter/gcimporter.go (left): https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.g... go/gcimporter/gcimporter.go:31: types.DefaultImport = Import This might break some code... https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.go File go/gcimporter/gcimporter.go (right): https://codereview.appspot.com/43490043/diff/40001/go/gcimporter/gcimporter.g... go/gcimporter/gcimporter.go:111: // If no FileSystem is specified, buildfs.Default is used. s/FileSystem/file system/ since there's no such parameter name (it's "fs"). And later you refer to srcDir and not source directory.