The implementation would be pretty simple, and it would let you use txtar format for templates and http.FileServers.
Comment From: earthboundkid
@rogpeppe
Comment From: rsc
SGTM. No need for a proposal.
Comment From: earthboundkid
I’m on paternity leave now so I can only type with my left thumb during the day. (Gotta keep the bottle in my right hand.) I’ll try to send out a CL as soon as I can but it may not be until next month.
Comment From: zikaeroh
Hate to be a broken record on this particular thing, but I'm pretty sure thanks to #40067, this can't happen until x/tools no longer supports versions of Go without the fs package available, as "new" standard library packages break go mod tidy on older versions of Go (regardless of build tag usage).
The fs.FS interface isn't exactly huge, so potentially there could be a redeclaration of the types in fs for older versions of Go to allow it to work, but that sounds a little annoying.
(This could certainly be implemented in some other repo/module and work fine, though. golang-migrate has done this for now to support io/fs implementations for migrations, for example.)
Comment From: ianlancetaylor
For this kind of use--implementing the interface--the relevant code can be protected by a build tag.
Comment From: earthboundkid
IIUC, #40067 means that a build tag won’t be able to keep go tidy from failing on older versions of Go. There seems to be no way to maintain compatibility with new packages, as opposed to new identifiers, in the standard library. Is that a correct interpretation, @zikaeroh?
Comment From: zikaeroh
Yes, that's correct. My interpretation of Ian's comment was that all of the bits and pieces that make up the io/fs.FS interface could be redeclared to make it possible (it's not all that many), but I may have interpreted that incorrectly. Build tags alone cannot guard against new stdlib package usage when it comes to the eyes of the module code (even if when downloaded, the build would have worked fine).
IMO #40067 is going to gate quite a few users for the much-anticipated io/fs and embed packages (or break people who aren't aware), hence me mentioning it before anyone gets into trouble...
Comment From: bcmills
Yes, that's correct. It is unfortunately not possible to implement the fs.FS interface without importing the fs package.
Probably the cleanest solution in the short term is to put the txtar implementation of fs.FS in a separate package (perhaps golang.org/x/tools/txtar/txtarfs?), which can then safely import the new io/fs package.
Comment From: zikaeroh
Probably the cleanest solution in the short term is to put the
txtarimplementation offs.FSin a separate package (perhapsgolang.org/x/tools/txtar/txtarfs?), which can then safely import the newio/fspackage.
Even a new package won't work, it'd need to be another module; #40067 affects modules, as the tooling refuses to continue once it detects that a module it's downloaded contains a package it can't resolve.
Comment From: bcmills
@zikaeroh, I believe you are mistaken. Neither go get nor go mod tidy scans unimported packages in dependencies. (go get scans the dependencies of the specific packages named on the command line.)
Comment From: bcmills
It would mean that users would be unable to run go mod tidy within x/tools when authoring changes to the x/tools module. However, users authoring changes to x/tools itself ought to be testing with at least the most recent release anyway, and if they have it installed for testing, they can use it for go mod tidy as well.
Comment From: zikaeroh
I was basing my experience on https://github.com/golang/go/issues/40067#issuecomment-731626053, but I can see how that list of packages means I was indirectly importing it. I'll have to make a minimal example to look at the edge cases.
I guess this is overall better than "it's impossible", but this behavior is a little infectious in nature and non-obvious that there's a different set of checks that aren't aware of build tags or new packages.
Comment From: earthboundkid
The separate package thing will only work if nothing imports it, at which point I may as well just publish the package on my own account and wait until Go 1.16 is the lowest supported version to comeback and add it to x/tools.
Comment From: josharian
I couldn't quite tell what the status was here, and I wanted this for my own purposes, so I put together an extremely quick and dirty bridge package: https://github.com/josharian/txtarfs. I look forward to txtar directly supporting fs.FS in the future.
Comment From: zikaeroh
I was wrong about it infecting the whole module (@bcmills was of course correct); x/tools gained a new package for godoc/vfs to io/fs and it hasn't broken my usage of x/tools (so long as I don't try to tidy x/tools itself with Go 1.15 or something), so this could safely go in as something similar.
Comment From: zikaeroh
Oh, I'm a liar, that was added to an existing package and I didn't notice when I read the CL, so it doesn't serve as a working example (and probably should be added to #40067...).
Comment From: zikaeroh
With #44557 fixed and now backported, nothing stops adding io/fs and embed to existing packages anymore (other than waiting for patch release adoption).
Comment From: earthboundkid
If it's possible to make this change now, @josharian do you want to just submit your txtarfs as a CL?
Comment From: josharian
It's kind of a hack, and it allocates unnecessarily, and it creates a new thing instead of just using the txtar directly as an fs.FS. So I think it's not worth upstreaming—it'd be better to do properly.
Comment From: earthboundkid
I started working on an implementation, and getting the directory stuff right basically requires reimplementing MapFS, so I don't think there's actually that much to be gained by doing it "right". I would change As() to Archive.FS() though.
Comment From: josharian
I see. Pity.
Feel free to steal code wholesale from txtarfs as you please. If you want to credit me, you can add a "Co-authored-by: " trailer to the git commit.
Comment From: earthboundkid
Now I'm looking at it more, and the directory stuff is a pain, but not insurmountable, and having the Open method on Archive itself would be nice. From could be copied wholesale though.
Comment From: earthboundkid
I added a co-authored credit, but that seems to have made the CLA bot angry 😆
Comment From: earthboundkid
Okay, I opened a PR without the Co-authored-by comment and now it seems to be accepting the CLA.
Comment From: rsc
I said "no need for a proposal" above but it came up in #61386 and it sounds like there are design issues, so moving back to the proposal process.
Archive/zip does this to convert each file name to a valid name:
// toValidName coerces name to be a valid name for fs.FS.Open.
func toValidName(name string) string {
name = strings.ReplaceAll(name, `\`, `/`)
p := path.Clean(name)
p = strings.TrimPrefix(p, "/")
for strings.HasPrefix(p, "../") {
p = p[len("../"):]
}
return p
}
It seems like txtar can do something similar? The initFileList code from archive/zip can be reused too.
Comment From: rsc
This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group
Comment From: earthboundkid
The big thing we need to decide here is whether Archive has an fs.FS returning method or Archive is an fs.FS itself. There are pros and cons to either. Returning an fs.FS leaves the door open to future optimizations. Being an fs.FS makes some of the code simpler at the cost of not leaving room for future optimizations.
Comment From: rsc
I think txtar.Archive should just be an FS, the same way that zip.Reader is. It can still have its own unexported cache supporting the FS operations. The only tricky part would be if users expected to be able to change a.File[i].Name and expect to see those changes reflected immediately in future uses of the Archive as an FS. I am inclined to define that the Archive must not be modified after being used an FS. The alternative is that we have a relatively expensive O(N) method a.FS() returning an FS, but people may well expect that to be cheap, using it for things like a.FS().ReadFile("name"), and be surprised that it's O(N) and not O(1).
Comment From: rsc
On the other hand, we've resisted adding any methods at all to Archive so far, so maybe we should continue that. A top-level function is less likely to be expected to be "cheap" than a method too, so that resolves that problem.
// FS returns the file system form of an Archive.
// It returns an error if any of the file names in the archive
// are not valid file system names.
// It also builds an index of the files and their content:
// adding, removing, or renaming files in the archive
// after calling FS will not affect file system method calls.
// However, FS does not copy the underlying file contents:
// change to file content will be visible in file system method calls.
func FS(a *Archive) (fs.FS, error)
Comment From: earthboundkid
That works for me.
Comment From: earthboundkid
Implementing this, I notice we have filepath.IsLocal, but not path.IsLocal. We should probably also have path.IsLocal which would just be the same as Unix filepath.IsLocal.
Comment From: rsc
Have all remaining concerns about this proposal been addressed?
Add the following to package txtar:
// FS returns the file system form of an Archive.
// It returns an error if any of the file names in the archive
// are not valid file system names.
// It also builds an index of the files and their content:
// adding, removing, or renaming files in the archive
// after calling FS will not affect file system method calls.
// However, FS does not copy the underlying file contents:
// change to file content will be visible in file system method calls.
func FS(a *Archive) (fs.FS, error)
Comment From: josharian
Reading that doc comment makes me wonder whether we should copy the file contents too. Txtar files aren’t likely to be massive, and it makes the API much cleaner.
Comment From: rsc
But if they were massive, it'd be weird to make a full copy of all the content.
Comment From: earthboundkid
If there are a lot of files, the current implementation will be slow because it has to iterate a MapFS. I think copying probably makes the most sense.
Comment From: josharian
I hear you, Russ, but it feels weird to copy some but not all of it. I won’t die on this hill, though—happy to have this available any which way.
Comment From: bcmills
Would it feel less weird if we pass the Archive to FS by value instead of by pointer? That would emphasize that the returned fs.FS wraps the underlying content rather than aliasing the Archive at the top level.
Comment From: rsc
If FS takes an Archive then you have to explain why Format takes an *Archive. I think we should follow our usual convention that one doesn't try to express sharing details with pointer-ness of arguments.
Comment From: rsc
@josharian How about this:
// FS returns the file system form of an Archive.
// It returns an error if any of the file names in the archive
// are not valid file system names.
// The archive must not be modified while the FS is in use.
func FS(a *Archive) (fs.FS, error)
We can even have the implementation save integer indexes into the archive and double-check the ones it is implicitly using during each FS operation, and return ErrModified if it sees a modification.
Comment From: rsc
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group
The new API is:
// FS returns the file system form of an Archive.
// It returns an error if any of the file names in the archive
// are not valid file system names.
// The archive must not be modified while the FS is in use.
func FS(a *Archive) (fs.FS, error)
// ErrModified indicates that file system returned by FS
// noticed that the underlying archive has been modified
// since the call to FS. Detection of modification is best effort,
// to help diagnose misuse of the API, and is not guaranteed.
var ErrModified error
Comment From: rsc
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
The new API is:
// FS returns the file system form of an Archive.
// It returns an error if any of the file names in the archive
// are not valid file system names.
// The archive must not be modified while the FS is in use.
func FS(a *Archive) (fs.FS, error)
// ErrModified indicates that file system returned by FS
// noticed that the underlying archive has been modified
// since the call to FS. Detection of modification is best effort,
// to help diagnose misuse of the API, and is not guaranteed.
var ErrModified error
Comment From: gopherbot
Change https://go.dev/cl/598756 mentions this issue: txtar: implement fs.FS