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

proposal: io/fs: add writable interfaces #45757

Closed
matthewmueller opened this issue Apr 25, 2021 · 157 comments
Closed

proposal: io/fs: add writable interfaces #45757

matthewmueller opened this issue Apr 25, 2021 · 157 comments

Comments

@matthewmueller
Copy link

matthewmueller commented Apr 25, 2021

Go 1.16 introduced the embed and io/fs packages. The current implementation is a minimal readable filesystem interface.

This is a wonderful first step towards standardizing filesystem operations and providing the community with a lot of flexibility in how we implement filesystems. I would like us to take the next step and define a writable file system interface.

Problem

fs.FS can't be written to after it's defined.

func Write(fs fs.FS) {
  // We can't write to FS.
  fs 
}

Optional interfaces could be defined in user-land:

func Write(fs fs.FS) {
  // We can't rely on vfs.Writable being implemented across community packages.
  writable, ok := fs.(vfs.Writable)
}

But it suffers from the same issues that the readable filesystem interface aimed to solve: standardizing the interface across the ecosystem.

Use Cases

I'll list of few use-cases that I've come across since Go 1.16, but I'm sure the community has many more:

  • A virtual filesystem that you can write to over time. Useful for file bundlers and databases that work in-memory and flush to the operating system's filesystem at the end.
  • Write files to cloud storages like Google Cloud Storage or Amazon S3.

We've already seen started to see this pop up in the community around io/fs to address the problem in user-land:

A quick search on Github will yield more community libraries: https://github.com/search?q=%22io%2Ffs%22+language%3Ago. For many of these implementations, you can imagine a useful writable implementation.

Of course, there are many other file system libraries that came before io/fs that define writable interfaces like afero and billy.

Proposal

I don't feel qualified to define an interface, I know people have thought about this much harder than I have. What I would love to see from a community member's perspective is the following:

package fs

func WriteFile(fs FS, name string, data []byte, perm FileMode) error
func MkdirAll(fs FS, path string, perm FileMode) error
func Remove(fs FS, name string) error

Nice to Have: Be able to define if a filesystem is readable, writeable or read-writable.

func Open(fs fs.FS) (*DB, error)   // Readable
func Open(fs fs.WFS) (*DB, error)  // Writable
func Open(fs fs.RWFS) (*DB, error) // Read-Writable

Thanks for your consideration!

@gopherbot gopherbot added this to the Proposal milestone Apr 25, 2021
@matthewmueller matthewmueller changed the title proposal: add Writable interfaces to io/fs proposal: add writable interfaces to io/fs Apr 25, 2021
@ianlancetaylor ianlancetaylor changed the title proposal: add writable interfaces to io/fs proposal: io/fs: add writable interfaces Apr 25, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 25, 2021
@coder543
Copy link

coder543 commented Apr 27, 2021

I'm currently experimenting with writing a file format encoding/decoding/mutating package that is intended to work with files that aren't guaranteed to easily fit in memory.

I would like to implement it in terms of fs.FS, which would make it so the library doesn't have to care whether these files actually exist on the local filesystem, in memory, or stored somewhere else. In point of fact, this is intended to be an archival format that distributes the contents of a single archive over a configurable number of actual files, and these files might be distributed geographically across different regions for redundancy.

This codec package doesn't want to care about supporting all the different places that the underlying files could be stored. It just wants to take in an fs.FS and a list of paths.

Additionally, to make this package more testable, using fs.FS would make it trivial to write tests without having to actually read and write files on disk.

Unfortunately, since fs.FS is read-only, I'm sitting here thinking up complicated ways that I could support fs.FS and somehow still support actually writing and mutating files on disk.

@DeedleFake
Copy link

DeedleFake commented Apr 27, 2021

I've got a similar issue with my p9 package. It attempts to implement a 9P client and server in a high-level way, similar to net/http, and while I'd like to rework the file abstraction to use fs.FS, it currently would result in odd things due in part to Open() returning an fs.File, which is then read-only by design. For now, what I'm leaning towards is just having a function that takes my package's filesystem type and returns an fs.FS that abstracts it away, but the read-only problem will still be there.

Maybe something like this could work?

type RWFS interface {
  FS
  WriteFS
}

type WriteFS interface {
  // Create creates a new file with the given name.
  Create(string) (WFile, error)

  // Modify modifies an existing file with the given name.
  Modify(string) (WFile, error)
}

type WFile interface {
  Write([]byte) (int, error)
  Close() error
  // Maybe also some kind of WStat() method?
}

Then the returned types would only have to expose either reading or writing methods, and the interface would just handle it transparently.

Persionally, I think that it would be a lot better if there was some way to abstract away the specific requirement of an fs.File as the returned type so that either Open(string) (*os.File, error) or Open(string) (SomeCustomFileType, error), but that would require language changes and that seems like overkill. It could be partially done with generics, such as with type FS[F File] interface { ... }, but it has some odd potential complications, and it wouldn't be fully backwards compatible at this point.

@dhemery
Copy link

dhemery commented May 8, 2021

To me, this looks like the minimum requirement:

package fs

type WFile interface {
    Stat() (FileInfo, error)
    Write(p []byte) (n int, err error)
    Close() error
}

type WriteFS interface {
    OpenFile(name string, flag int, perm FileMode) (WFile, error)
}

And another (ugh) for making dirs:

type MkDirFS interface {
    MkDir(name string, perm FileMode) error
}

And some helper functions for convenience:

func Create(fsys WriteFS, name string) (WFile, error) {
    // Use fsys.OpenFile ...
}

func WriteFile(fsys WriteFS, name string, data []byte, perm FileMode) error {
    // Use fsys.OpenFile, Write, and Close ...
}

func MkDirAll(fsys MkDirFS, path string, perm FileMode) error {
    // Use fsys.MkDir to do the work.
    // Also requires either Stat or Open to check for parents.
    // I'm not sure how to structure that either/or requirement.
}

@kylelemons
Copy link
Contributor

kylelemons commented May 9, 2021

I think that we should lean more heavily on io and os, rather than making top-level WFile types. In particular, I think we should basically just define some top-level functions that can fall back all the way to Open, and not have a WFile interface at all.

Summary

  • ErrUnsupported for when implementations are not available
  • Optional FS methods:
    • WriteFile(name, data, perm) (error)
    • OpenFile(name, perm) (File, error)
    • Create(name) (File, error)
  • Optional File methods:
    • Write(data) (int, error)
    • Truncate(size) error for use when FS.Create is being emulated
      • Only used if the file has nonzero size in Create
    • Chmod(FileMode) error for use when FS.OpenFile or FS.WriteFile are being emulated
      • Only used if the mode does not match after Open
  • Helpers
    • Create(fs, name): try CreateFS, then Open+Stat+Truncate
    • OpenFile(fs, name, perm): try OpenFileFS, then Open+Stat+ChmodFile
    • WriteFile(fs, name, data, perm): try WriteFileFS, then OpenFile()+writeContents
    • Write(File, data) (int, error): calls Write or returns ErrUnsupported

Detail

See sketch of an implementation here:

https://gist.github.com/kylelemons/21539a152e9af1dd79c3775ca94efb60#file-io_fs_write_sketch-go

This style of implementation appeals to me because:

  • You can check for mutability of a file with a familiar type: io.WriteCloser (or just io.Writer, but File requires Close)
  • File mutability, metadata mutability, and FS mutability are all orthogonal
    • Mutable file trees can store immutable files
    • Immutable file trees can store mutable files
    • Not all files in a FS need to have the same mutability constraints
    • Subs could be mutable even though the FS is not
  • This keeps the "primary" top-level interfaces the same: FS and File
  • Truncate is not required if Create is never used on nonzero-length files
  • Chmod is not required if the permissions are correct by default (e.g. by exposing a FixedMode from your fs package)

I think the same patterns can be used to implement Mkdir and MkdirAll on a filesystem as well.

@geoffroy-perso
Copy link

I think that also having a Remove method that can work with a fs.FS interface would be a great addition (similar to the current os.Remove function).

Just a small contribution as it feels to me in the scope of this proposal, and since no one mentionned file removal 🙂

@sbourlon
Copy link

sbourlon commented Mar 24, 2022

@kylelemons just to be consistent with fs.ReadDirFile, the only File extension at the moment, and iofs draft/The File interface:

type ReadDirFile interface {
  File
  ReadDir(n int) ([]DirEntry, error)
}

type WriteFile interface {
  File
  Write(p []byte) (n int, err error)
}

There is a comment from Russ Cox about this exact change as well.

@hairyhenderson
Copy link

hairyhenderson commented Mar 24, 2022

There's an interesting package https://pkg.go.dev/github.com/hack-pad/hackpadfs that's got some interfaces defined for a writable version of io/fs... The interfaces "feel" pretty much like they should, and are effectively the same as much as what's been discussed here already. I'm in no way associated with that project, just thought it was some interesting work that might inform how writable file interfaces fit into a future release.

@cristaloleg
Copy link

Looks like this proposal has label Proposal but isn't a part Review Meeting #33502

@rsc can you move it into https://golang.org/s/proposal-status#active column please?

@rsc
Copy link
Contributor

rsc commented May 25, 2022

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

@rsc rsc moved this from Incoming to Active in Proposals (old) May 25, 2022
@frioux
Copy link

frioux commented May 25, 2022

I haven't thought as in depth as some of the other folks here, but I came up with a similar set of interfaces and shortcuts for an FS backed by both the local filesystem and backed by the dropbox API. Here's the API (obviously not all of this is so general purpose:)

type CreateFS interface {
        fs.FS

        Create(string) (FileWriter, error)
}

type FileWriter interface {
        fs.File

        Write([]byte) (int, error)
}

type RemoveFS interface {
        fs.FS
        Remove(string) error
}

type WatchFS interface {
        fs.FS

        Watch(context.Context, string) (chan []fsnotify.Event, error)
}

type WriteFileFS interface {
        fs.FS

        WriteFile(string, []byte, fs.FileMode) error
}

func OpenDirFS(d string) fs.FS
func Remove(fss fs.FS, path string) error
func Watch(ctx context.Context, fss fs.FS, dir string) (chan []fsnotify.Event, error)
func WriteFile(fss fs.FS, name string, contents []byte, mode fs.FileMode) (err error)

@bcmills
Copy link
Contributor

bcmills commented May 25, 2022

If we add a WriteFile, I suggest that it should deviate from io.WriteFile to fix what can otherwise be a subtle concurrency bug.

os.WriteFile truncates the existing file before writing, which causes otherwise-idempotent writes to race. I suggest that fs.WriteFile should instead truncate after writing, so that an idempotent file can be rewritten arbitrarily many times without corrupting its contents.

@adamluzsi
Copy link

adamluzsi commented May 29, 2022

Hello everyone!

Recently I encountered a similar use case where I wanted to dependency inject a filesystem,
and my consumer is required to create, modify or remove files on the fsys.

I came up with the following header interface that closely follows the os stdlib:

type FileSystem interface {
	Stat(name string) (fs.FileInfo, error)
	OpenFile(name string, flag int, perm fs.FileMode) (File, error)
	Mkdir(name string, perm fs.FileMode) error
	Remove(name string) error
}

type File interface {
	fs.File
	fs.ReadDirFile
	io.Writer
	io.Seeker
}

I made an interface testing suite to cover roughly the ~127 most common filesystem interactions from a behavioural point of view.
Then I made two suppliers for this interface:

  • filesystems.Local
    • local file system with optional jail root path.
    • it uses the os package functions
  • filesystems.Memory
    • in-memory file system variant that could be used for testing purposes.

The same interface testing suite tests both.

You can use them as a drop-in replacement for places where you had to use os the package.
The filesystems package also supplies similar functions as the os such as Open, Create, ReadDir, and WalkDir.

I plan to use and maintain this until an easy to use replacement comes out in the std library.
I just wanted to share this with you all, hoping it helps someone out.

Cheers!

@zeho11
Copy link

zeho11 commented Jul 10, 2022

I'd write like this:

type WritableFS interface {
	fs.FS
	OpenFile(name string, flag int, perm fs.FileMode) (WritableFile, error)
}

type WritableFile interface {
	fs.File
	io.Writer
}

func Create(fsys WritableFS, name string) (WritableFile, error)

func WriteFile(fsys WritableFS, name string, data []byte, perm fs.FileMode) error

...

@chrisguiney
Copy link
Contributor

On top of a +1 for having the need of a writable fs interface, I'd like to enter into the conversation that files are not inherently readable or seekable. for example, files may be opened with fs.ModeAppend, or os.Stdin.

That is , when designing the interface, I suggest keeping the writer interface disjoint from the reader interface, and instead having a third interface to union them.

Any writable file implementation could return an error for read/seek operations...but it'd be nice to express that they're not necessary

@hairyhenderson
Copy link

That is , when designing the interface, I suggest keeping the writer interface disjoint from the reader interface, and instead having a third interface to union them.

@chrisguiney there are already io.Writer, io.Seeker, and io.WriteSeeker interfaces that should meet this need. IMHO using those interfaces best expresses the ability to write and/or seek.

@fgm
Copy link

fgm commented Jul 20, 2022

That is , when designing the interface, I suggest keeping the writer interface disjoint from the reader interface, and instead having a third interface to union them.

@chrisguiney there are already io.Writer, io.Seeker, and io.WriteSeeker interfaces that should meet this need. IMHO using those interfaces best expresses the ability to write and/or seek.

These interfaces are for writable "things" like files, but the requirement in this issue is for a file-system-level interface, not for a "file"-level interface, so this does not seem to answer the need, does it ?

@Merovius
Copy link
Contributor

Personally, I don't think there is a practical way to support a writable fs.FS. Or it's at least extremely non-trivial.

When it comes to reading files, OS-dependent semantic differences can mostly be papered over by handwavingly assuming the fs.FS is immutable. Once you actively encourage writing to the underlying filesystem, though, you open an uncontainable can of worms of semantic deviations between different operating systems and even filesystems on the same OS.

Questions which immediately arise are

  1. What happens if a file is opened for reading and for writing simultaneously? Windows (AFAIK) refuses this, returning an error, while most unix-like OSes will allow it. If we say it is forbidden, how do we forbid it under Linux? If we say it is allowed, how do we allow it under Windows? If we shrug and pass the buck, how are programmers supposed to deal with it?
  2. What kind of atomicity guarantees are given? When a power-loss (or the process is killed by a signal) happens during a Write, what are the allowed states for the resulting file to be in? This is entirely unspecified, even within a single OS. It heavily depends on the FS and the settings its mounted with.
  3. Similarly, what about Flush? Again, in practice it is mostly unspecified what this actually does (in the presence of a crash) but it's most likely going to need to be supported.
  4. How do renames behave if the target file exists, or source and target are the same file, or one is a symlink to/below the other?
  5. Speaking of symlinks, what about them? What about unix permission bits and what on systems lacking those? What about extended attributes? What about SELinux etc.?
  6. What about flock?
  7. How do you handle differences in allowed file names on different OSes?
  8. An interface like WriteFile might be reasonable to support, but even that is difficult to do cross-platform and even then, requires different APIs than just a simple "dump this please" to be reliable.

As far as I can tell, there just is no sound way to build an abstraction over filesystems that allows writing. The best advice I've heard so far on this topic is "if you care about data being written, use a database".

Of course, it is possible to just provide some API and tell the programmers that they can't actually rely on the data actually being written, so it shouldn't be used in any production use case. But that just feels irresponsible.

@hherman1
Copy link

@Merovius isn’t the os package such an abstraction? Would you consider that irresponsible/not safe for prod?

@coder543
Copy link

coder543 commented Jul 27, 2022

Yeah, I’m not convinced by any of that @Merovius.

The write interface doesn’t need to support every imaginable operation to be useful, and of the operations it would support, Go is well known for taking a pragmatic approach, even when it leads to correctness issues in some circumstances, and this pattern is all over the file interfaces Go already provides. Windows doesn’t support Unix permission bits, so Go just… does whatever it feels like on Windows.

Maybe we shouldn’t use Go in production? I disagree.

A lot of your questions would be up to the implementation to decide. If the implementation is transparently delegating to a real OS filesystem, the answers would mirror what we currently see in the os package… it would be nothing shocking at all. If the implementation is more abstract, it can and will do whatever seems reasonable, and people will report bugs against that package’s repo if they disagree. That’s how all interfaces work.

@duckbrain
Copy link

I'll share my experience with using HackPadFS for a fairly complex use-case. My application is exposing a filesystem through various protocols (WebDAV, SFTP, custom API, etc) as well as providing individual users file systems with "mounts" on directories similar to a Unix filesystem. So I'm using HackPadFS to provide various writeable filesystems in combination and mount them together, but I also have application-specific abstractions that warp fs.FS that can do user-level permissions and such.

HackPadFS uses an interface called type MountFS interface { Mount(name string) (FS, string) } to act as an "unwrap" extension for fs.FS. Because it unwraps a path and provides a new name, it's flexible for path transformations or different underlying fs.FS at different paths.

Similar to the errors.As unwrap functionality, I've implemented functions to unwrap my application-specific extensions. Mine use parameterized types, but it should be doable with reflection if we'd want to avoid that for some reason.

// As unwraps the underlying fs.FS responsible for name that implements T and invokes fn.
// So long as fn returns fs.ErrNotImplemented, it will continue to unwrap and reinvoke fn.
// If fn returns a fs.PathError, the Path will be adjusted to make up for the input name.
// If As returns an fs.ErrNotImplemented error, a fallback may be attempted.
//
// Example:
//     var foo Foo
//     err := fs.As(fsys, name, func(fsys FooFS, name string) (err error) {
//         foo, err = fsys.OpenFoo(name)
//         return
//     })
func As[T FS](fsys FS, name string, fn func(fsys T, name string) error) error

// As2 is similar to As, but it supports two file names that need to both be unwrapped.
// fn will only be called if both paths resolve to the same underlying fs.FS.
func As2[T FS](fsys FS, leftName, rightName string, fn func(fsys T, leftname, rightname string) error) error

Instead of Mount I'd call have an interface like interface { Unwrap(name string) (FS, string, error) }, but the concept is the same. As2 is for Rename/Symlink/Copy, I'd leave it out of the standard lib, but implement it in a library like HackPadFS that provides those extension methods.

In my application metadata is optionally added to existing files (stored separately from FS), keyed by path.
I maintain it through the RenameFS/RemoveFS HackPad extenions, with a private extension for CRUD. The abstraction layer doesn't need to implement other HackPad extensions, since the HackPadFS helper methods will unwrap past it. Other layers can provide caching, case-insensitivity, or hiding files.


The largest challenge with all of this is alternate implementations of fs.FS that don't know about hackpadfs.MountFS. For example, HackpadFS had to reimplement fs.Sub to implement hackpad.MountFS. A common unwrap interface in the standard library would help with this like it did for error/context.

Regarding the actual writable interface, ReaderFrom seems like a good conservative first step toward a writable interface. In the case of libraries that currently extend fs.FS for a writeable interface, they could fall back to a ReaderFrom interface for actions like Create/Mkdir/Chtimes. If it's defined as "make this FS exactly like this other one", it could also simulate Rename/Remove. It would require complex wrapper fs.FS to pass in the original with simple modifications, with hidden checks in the implementation, but it may be possible to make it kinda effecient, while leaving the door open for 3rd party extensions to make the truely efficient ones. I'd have to play around with the idea more.

@warpfork
Copy link

Hi friends,

I've taken a crack at writable FS interfaces... and decided the invest the time into publishing it as a module. Here it is: https://github.com/warpfork/go-fsx .

It adds OpenFile, Mkdir, LStat, Readlink, and MkSymlink. All are based on interfaces and feature-detection. (I haven't yet added Chown or Chtimes; PR's welcome.)

It's following the golang standard library fs (and riffing also from the signatures in os) as faithfully as possible, in style, and also in guarantees (or lack thereof). It's also using aliases heavily, to remain usable when intermixed with fs and os.

I don't know if I will claim it's perfect -- probably not! -- but it's fairly fully worked, and I think it's usable. And it's fairly conservative and close to what people have described throughout this issue.

If anyone would like to use it, hope it helps! I wrote it for myself to scratch the itch, but if it saves anyone else time as well, then that's awesome.

If it advances the conversation, that's also awesome. However, I think it's not even that interesting ;) It's roughly the ideas already discussed earlier.

@JohnStarich
Copy link

I created hackpadfs to use file systems in places they were impossible before or did not have the required performance characteristics. I've written about hackpadfs before, which covers why it was needed and some concrete use cases.

Strong guarantees on several writable interfaces' behavior is certainly valuable, even though it may be difficult to create precise definitions. The most difficult problem I needed to solve was compliance with os package behavior. Running the real Go compiler in the browser with my own FS implementations required near-identical behavior to os or Go would fail in unexpected ways. A rigorous compliance test suite was necessary for regression tests.

While difficult, only a few minor differences appeared across the major OSes with regard to os package behavior. Keeping those differences in mind, a small set of well-defined writable operations would be fantastic. I recommend checking out the source of hackpadfs/fstest for inspiration to precisely define interface behavior.

As a small start, I tend to agree with @ianlancetaylor's suggestion for something like an io.WriteFS with a Create() method returning a file. The tricky parts may be the file permissions it uses and the return type. In hackpadfs, we set the permission to 0666 (before umask) and the return type to the io/fs.File interface. We left it up to the client to check its interface implementations, either manually or via package helpers.

An excerpt from hackpadfs/fs.go, file.go:

package hackpadfs

// FS provides access to a file system and its files.
// It is the minimum functionality required for a file system, and mirrors Go's io/fs.FS interface.
type FS = gofs.FS

// File provides access to a file. Mirrors io/fs.File.
type File = gofs.File

// CreateFS is an FS that can create files. Should match the behavior of os.Create().
type CreateFS interface {
	FS
	Create(name string) (File, error)
}

(Note: I did my best to read through all of the comments above, though I may have missed some.)

@rsc
Copy link
Contributor

rsc commented Jun 21, 2023

Returning to this issue. Back on April 21, @bcmills suggested 7 new interfaces in #45757 (comment).

type OpenFileFS interface { FS; OpenFile(name string, flag int, perm FileMode) (File, error) }
type ChtimesFS interface { FS; Chtimes(name string, atime, mtime time.Time) error }
type RemoveFS interface { FS; Remove(name string) error }
type MkdirFS interface { FS; Mkdir(name string, perm FileMode) error }
type ChmodFile interface { File; Chmod(mode FileMode) error }
type ChownFile interface { File; Chown(uid, gid int) error }
type TruncateFile interface { File; Truncate(size int64) error }

The usual advice is that if you have 7 of something you probably missed one. The hackpadfs post linked in #45757 (comment) notes 27 new interfaces:

// New FS interfaces:
type SubFS interface       { FS; Sub(dir string) (FS, error) }
type OpenFileFS interface  { FS; OpenFile(name string, flag int, perm FileMode) (File, error) }
type CreateFS interface    { FS; Create(name string) (File, error) }
type MkdirFS interface     { FS; Mkdir(name string, perm FileMode) error }
type MkdirAllFS interface  { FS; MkdirAll(path string, perm FileMode) error }
type RemoveFS interface    { FS; Remove(name string) error }
type RemoveAllFS interface { FS; RemoveAll(name string) error }
type RenameFS interface    { FS; Rename(oldname, newname string) error }
type StatFS interface      { FS; Stat(name string) (FileInfo, error) }
type LstatFS interface     { FS; Lstat(name string) (FileInfo, error) }
type ChmodFS interface     { FS; Chmod(name string, mode FileMode) error }
type ChownFS interface     { FS; Chown(name string, uid, gid int) error }
type ChtimesFS interface   { FS; Chtimes(name string, atime time.Time, mtime time.Time) error }
type ReadDirFS interface   { FS; ReadDir(name string) ([]DirEntry, error) }
type ReadFileFS interface  { FS; ReadFile(name string) ([]byte, error) }
type SymlinkFS interface   { FS; Symlink(oldname, newname string) error }
type MountFS interface     { FS; Mount(name string) (mountFS FS, subPath string) }
// New File interfaces:
type ReadWriterFile interface { File; io.Writer }
type ReaderAtFile interface   { File; io.ReaderAt }
type WriterAtFile interface   { File; io.WriterAt }
type DirReaderFile interface  { File; ReadDir(n int) ([]DirEntry, error) }
type SeekerFile interface     { File; io.Seeker }
type SyncerFile interface     { File; Sync() error }
type TruncaterFile interface  { File; Truncate(size int64) error }
type ChmoderFile interface    { File; Chmod(mode FileMode) error }
type ChownerFile interface    { File; Chown(uid, gid int) error }
type ChtimeserFile interface  { File; Chtimes(atime time.Time, mtime time.Time) error }

The 'er' in the File interface names is non-idiomatic by the way. And I also don't understand why ReadDirFS, StatFS and SubFS are listed as new, when fs.ReadDirFS, fs.StatFS, and fs.SubFS already exist. But those quibbles aside, with 27 new interfaces the chance of having missed one increases exponentially.

These illustrate the fundamental problem. We are not going to add 27 new interfaces to io/fs. Even 7 new ones seems like a bit much. If there is a path forward here, it needs to be a more general path. I don't think that general path has been discovered yet.

@bobg
Copy link

bobg commented Jun 21, 2023

if you have 7 of something you probably missed one

When you state it this way - by haphazardly listing a bunch of proposed interfaces - I completely agree. But restated as a simple rule:

For each function in the os package that interprets an argument as a filesystem path, create a filesystem abstraction

you can reduce the chances of "missing one" back down to zero.

@bcmills
Copy link
Contributor

bcmills commented Jun 21, 2023

The hackpadfs interfaces seem too redundant to me: many of the variants provide the same operations on FS and File, or provide All variants that could instead be implemented (perhaps less efficiently) as standalone functions in terms of more primitive calls, at least assuming a simple concurrency model (equivalent to the one supported by the os package today). Many of the File variants are redundant with the interfaces defined in the io package; it isn't at all clear to me why they would need to be redundantly defined with File methods added in.

Moreover, LstatFS and SymlinkFS are specific to symlinks (which are outside the scope of this proposal), and MountFS also seems orthogonal.

It may be that my list is incomplete or deficient (for example, perhaps ChtimesFS should be a File variant instead, with os.File gaining a Chtimes method), but I think a realistic set of orthogonal, non-redundant interfaces is closer to my seven than to the 27 listed above for hackpadfs.

@JohnStarich
Copy link

@rsc All fair criticisms. You're quite right about the "new" interfaces bit, I'll need to fix that.
Regarding the sheer number, it's almost certainly too many (and overlapping) interfaces to include in the standard library. HackPad needed them, but that's only one use case. I hoped sharing the why for the project may help inform a solution, since hackpadfs was mentioned here. However, I may have muddied the waters rather than clear them – apologies.

I agree a smaller, focused selection would be more appropriate here for the first blush. A useful and writable hierarchical file system could be reduced to creating both directories and writable files. In other words, these two:

type OpenFileFS interface  { FS; OpenFile(name string, flag int, perm FileMode) (File, error) }
type MkdirFS interface     { FS; Mkdir(name string, perm FileMode) error }

@JohnStarich
Copy link

JohnStarich commented Jun 29, 2023

@bcmills I like your initial selection of interfaces, and agreed on reducing redundancy.
ChmodFile and TruncateFile overlap a tad in behavior with other interfaces. Do you think they could (very roughly) be implemented with calls to OpenFileFS and existing interfaces?

type ChmodFile interface { File; Chmod(mode FileMode) error }
    Roughly equivalent to ReadFile(name) + OpenFile(name, O_RDWR|O_TRUNC, mode) + file.Write(previousContents)
type TruncateFile interface { File; Truncate(size int64) error }
    Roughly equivalent to Open(name) + Read() up to size + OpenFile(name, O_RDWR|O_TRUNC, mode) + file.Write(previousContents)

They aren't completely equivalent of course, but it might be good enough to reduce the set of new interfaces. I'm curious what you think.

@bokwoon95

This comment was marked as outdated.

@rsc
Copy link
Contributor

rsc commented Jul 18, 2023

The io/fs package was intended to be a useful intersection of things people might want, with the expectation that people would define their own extension interfaces above it, using io/fs as the interoperable core. So far that seems to be happening, including in large examples like HackPadFS. My comment above still seems accurate, that there's no clear convergence happening otherwise.

The main general use case I have seen raised since that comment is being able to extract a file system tree, which would require Create and Mkdir. Maybe that is compelling, but maybe not. It might make more sense to let writable file systems implement their own CopyFS functions instead, like

package os

// CopyFS copies all files in fsys into dir.
func CopyFS(dir string, fsys fs.FS) error

The nice thing about os.CopyFS is that it can decide how much information to take from fs.FS and what to leave out. I remain skeptical about a general-purpose directory copy function working. os.CopyFS is not general-purpose - it knows how to write to os files.

Similarly, maybe it makes sense to add a CopyFS method to zip.Writer. That seems entirely workable, whereas zip.Writer cannot support an arbitrary file system create interface, since it cannot have more than one file open for create at a time.

So if the goal is to supporting copying of fs.FS, perhaps we should establish that pattern instead of adding something new to io/fs.

What are the other general-purpose routines that would be enabled by extending the io/fs interfaces?

@smasher164
Copy link
Member

with the expectation that people would define their own extension interfaces above it, using io/fs as the interoperable core

This is basically the route I went down. I wanted to test a project's write operations as well. There were a few hitches though:

  1. There was no way to extend DirFS with operations like Create and Mkdir. Once DirFS is called, you lose access to the directory it's rooted under. So I ended up re-implementing DirFS for my needs there.
  2. However, in doing so, I needed access to safefilepath.FromFS, which I ended up using go:linkname for. It would be nice if this package's behavior were exported in some capacity.
  3. Given that fstest.MapFS synthesizes directories and MapFile doesn't correspond 1:1 to either openMapFile or mapDir, I ended up building my own treeFS/treeFile data structure for testing a writeable fs.

So yeah, ultimately this is doable, but this process could be made slightly easier.

Sidenote, the interfaces I ended up with were basically

type WriteableFile interface { fs.File; io.Writer }
type CreateFS interface { fs.FS; Create(name string) (WriteableFile, error) }
type MkdirFS interface { fs.FS; Mkdir(name string, perm fs.FileMode) (fs.FS, error) }

@hherman1
Copy link

I have a routine which reads files from disk, analyzes them, makes some changes, and writes them back. I’d like to make it take an interface fs so that I can test it more easily.

@paralin
Copy link
Contributor

paralin commented Jul 20, 2023

I'm implementing a networked virtual filesystem and want to have a common write interface for components to use, at present I have to implement adapters to the different abstractions available (which there are many) which reminds me of how reading was before the readable FS was added.

@rsc
Copy link
Contributor

rsc commented Jul 21, 2023

Even if we do figure out writable FS, it still may not be worth trying to do a general fs.Copy. In the meantime, I wrote this trivial CopyFS that could be added to package os and have already found a few uses for it:


func CopyFS(dir string, fsys fs.FS) error {
	return fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error {
		targ := filepath.Join(dir, filepath.FromSlash(path))
		if d.IsDir() {
			if err := os.MkdirAll(targ, 0777); err != nil {
				return err
			}
			return nil
		}
		r, err := fsys.Open(path)
		if err != nil {
			return err
		}
		defer r.Close()
		info, err := r.Stat()
		if err != nil {
			return err
		}
		w, err := os.OpenFile(targ, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0666|info.Mode()&0777)
		if err != nil {
			return err
		}
		if _, err := io.Copy(w, r); err != nil {
			w.Close()
			return fmt.Errorf("copying %s: %v", path, err)
		}
		if err := w.Close(); err != nil {
			return err
		}
		return nil
	})
}

@dpifke
Copy link
Contributor

dpifke commented Jul 21, 2023

Is interface conversion from the os.DirFS return value to a string, in order to get the original pathname, something which can be considered supported by compatibility guarantees?

Raising my use case from #45757 (comment), if we're not going to add os equivalents to fs.FS, I'd like a way to go back to a pathname from a fs.FS (if supported), so that I don't need to have separate method signatures for read-only vs. writable directory trees. I think this would currently work, but it feels hacky:

func Foo() {
    Bar(os.DirFS("/foo/bar"))
}

func Bar(fsys fs.FS) error {
    p, isDirFS := fsys.(string)
    if !isDirFS {
        return errors.New("fsys is not writable")
    }
    f, err := os.Create(path.Join(p, "baz"))
    // ...
}

An alternative might be to add a method to os (or maybe an interface to io/fs) to get the "on-disk pathname", but maybe that would be redundant.

@smasher164
Copy link
Member

@dpifke Even if you recover the original path, you also need join to implement Create/Mkdir/etc (see here). But yes if you wanted to extend the stdlib's dirfs, at minimum, some way to recover the path would be nice--like func DirPath(fs.FS) (string, error) or something.

@mojatter
Copy link

Here https://github.com/jarxorg/wfs is a package I made a few years ago and it works fine on the small systems. So far it has 4 filesystems osfs, memfs, s3fs, gcsfs and it passes TestFS in "testing/fstest".

I hope a simple writable interface added to "io/fs".

@rsc
Copy link
Contributor

rsc commented Aug 11, 2023

@bcmills and I looked at this, in particular at whether a step forward would be to adopt the approach in #45757 (comment). Unfortunately, the very first interface poses a problem: what are the open modes and where are they defined? I'm sure existing implementations assume the "os" modes, but io/fs is meant to be a completely portable API, independent of "os". So probably we'd have to define a new set of modes, and code would have to translate those modes into whatever they need. That's doable, but it's a sign that maybe we don't have the right answers yet.

I still haven't seen a compelling reason this needs to be in the standard library now. Maybe one will arise later, or maybe not. Either way, the extension interface pattern means that people can define these in their own packages and even interoperate with no problems. The only problems arise when the extensions refer to private types, and none of the proposed extensions do.

So it seems like we should probably decline this proposal and maybe circle back in a year or two.

@rsc
Copy link
Contributor

rsc commented Aug 16, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests