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: os: add FS() (filesystem simply using os functions) #47803

Closed
twmb opened this issue Aug 19, 2021 · 13 comments
Closed

proposal: os: add FS() (filesystem simply using os functions) #47803

twmb opened this issue Aug 19, 2021 · 13 comments

Comments

@twmb
Copy link
Contributor

twmb commented Aug 19, 2021

With the advent of fs.FS, I prefer to use that interface where possible. One paper cut that I've run into a few times is that there no way to use the os package outright as an fs.FS. Today, there is only os.DirFS, which returns a filesystem anchored at the given directory.

This is great for something like a file server, where we only want to serve files under a given directory and not provide access to anything outside that directory, but is less great for when we want to provide arbitrary access to the file system. In particular, if I have a program that accepts flags for where to load files, I want to accept both a local path (cmd -path localfile), or a fully specified path (cmd -path /absolute/directory/file). Where I use fs.FS within this program, I have to specify a thin wrapper around os, because DirFS does not provide the necessary flexibility.

The proposal is to add the following function and implementation:

var osFS = new(hostFS)

func FS() fs.FS { return osFS } // or `return new(hostFS)` and nix the global var

type hostFS struct{}

func (*hostFS) Open(name string) (fs.File, error)          { return Open(name) }
func (*hostFS) ReadDir(name string) ([]fs.DirEntry, error) { return ReadDir(name) }
func (*hostFS) Stat(name string) (fs.FileInfo, error)      { return Stat(name) }
@twmb twmb changed the title os: add FS() (filesystem simply using os functions) proposal: os: add FS() (filesystem simply using os functions) Aug 19, 2021
@gopherbot gopherbot added this to the Proposal milestone Aug 19, 2021
@mpx
Copy link
Contributor

mpx commented Aug 19, 2021

These functions already exist in a different form:

import "io/fs"
...
root := os.DirFS("/")
...
f, err := root.Open(path)
dirs, err := fs.ReadDir(root, path)
fi, err := fs.Stat(root, path)

Note, all fs.FS paths are "unrooted" so there is still a fundamental difference with os.Open, etc.. The docs for fs.ValidPath have more details.

@twmb
Copy link
Contributor Author

twmb commented Aug 19, 2021

The different form is exactly what is the limiting factor today, which is that DirFS is unrooted and does not allow access outside of the initial directory. If I start with os.DirFS("/"), then that is not the directory I am currently in -- os.Open handles either a rooted path or a cwd path.

Given the current unrooted aspect, this proposal is likely asking to relax that restriction. There's likely a good reason for the restriction that I'm unaware of, though.

@mpx
Copy link
Contributor

mpx commented Aug 19, 2021

From the draft design:

The use of unrooted names—x/y/z.jpg instead of /x/y/z.jpg—is meant to make clear that the name is only meaningful when interpreted relative to a particular file system root, which is not specified in the name. Put another way, the lack of a leading slash makes clear these are not host file system paths, nor identifiers in some other global name space.

OS paths and fs.FS paths are fundamentally different. fs.FS doesn't have concepts like "relative to current working directory" and "OS root directory". Relaxing the API to accept "/" would lead to the confusion described above. #42716 exists to make these differences clearer to developers.

It's probably better/less buggy to translate paths to fs.FS style before using the API. Eg, convert to an absolute path and remove the "/" prefix. Perhaps this translation functionality could be added to the os package?

@josharian
Copy link
Contributor

Somewhat related: #44279.

@mpx
Copy link
Contributor

mpx commented Aug 19, 2021

#44286 also highlights some difficulty using relative paths.

@twmb
Copy link
Contributor Author

twmb commented Aug 19, 2021

Perhaps this translation functionality could be added to the os package?

This runs into the problem of dealing with the current working directory, which would be at a different path prefix. Rereading the proposal and ValidPath documentation, maybe fs.FS is not meant to replace os in many use cases (e.g. CLIs), which is an unfortunate limitation.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 19, 2021
@earthboundkid
Copy link
Contributor

I wanted to write an FS for CLIs that would be able to open files or URLs, e.g. my-cli -open example.txt or my-cli -open http://example.com . I ran into the problem that there's no great way to do the translations, especially around absolute paths. In the end, I think the trouble was an FS represents a collection of files, but a CLI probably only wants one file (e.g. an io.ReadCloser), so it's not a good fit.

@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

fs.FS very clearly disallows paths starting with / or with ../ or even with ./ - they must be rejected by any valid Open implementation.
That ship has sailed and cannot be turned back.

@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

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) Aug 25, 2021
@twmb
Copy link
Contributor Author

twmb commented Aug 25, 2021

Is there any way to make fs.FS broadly useful for rooted filepaths, or is that a nonstarter? If it's a nonstarter, this proposal should be closed.

@mpx
Copy link
Contributor

mpx commented Aug 26, 2021

Existing fs.FS implementations are incompatible with "rooted" paths, I can't see how this could be changed without breakage.

This issue, and a number of issues linked here (#44279 in particular) highlight the difficulty of translating between OS and fs.FS paths.

It may make sense to provide functionality to sanitise OS paths so they are suitable for fs.FS. Eg, via some combination of filepath.Abs and filepath.Rel. This is a very common need for anyone wanting to use fs.FS to abstract working with a collection of files.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Sep 1, 2021
@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

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

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Sep 8, 2021
@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

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

anderseknert pushed a commit to ear7h/opa that referenced this issue Sep 6, 2022
There's no correct way to provide the behavior of native `os` functions
with an `fs.FS` since `fs.FS`s should reject rooted paths and only use
unix path separators ("/"). Initially I created a `rawFS` that directly
forwarded calls to the `os` package but it felt more wrong the more I
looked at it.

Relevant issues:
* golang/go#47803
* golang/go#44279

closes open-policy-agent#5066

Signed-off-by: julio <julio.grillo98@gmail.com>
anderseknert pushed a commit to anderseknert/opa that referenced this issue Sep 6, 2022
There's no correct way to provide the behavior of native `os` functions
with an `fs.FS` since `fs.FS`s should reject rooted paths and only use
unix path separators ("/"). Initially I created a `rawFS` that directly
forwarded calls to the `os` package but it felt more wrong the more I
looked at it.

Relevant issues:
* golang/go#47803
* golang/go#44279

closes open-policy-agent#5066

Signed-off-by: julio <julio.grillo98@gmail.com>
anderseknert pushed a commit to open-policy-agent/opa that referenced this issue Sep 6, 2022
There's no correct way to provide the behavior of native `os` functions
with an `fs.FS` since `fs.FS`s should reject rooted paths and only use
unix path separators ("/"). Initially I created a `rawFS` that directly
forwarded calls to the `os` package but it felt more wrong the more I
looked at it.

Relevant issues:
* golang/go#47803
* golang/go#44279

closes #5066

Signed-off-by: julio <julio.grillo98@gmail.com>
@golang golang locked and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants