interp: Initial support for fs.FS. #799

alecthomas · 2022-01-25T09:47:36Z

I haven't added any tests yet, this is just a first pass to see if this
is a good idea.

I converted all the obvious filesystem I could find that made sense.
There are a few issues though:

Yet another variant of LookPath called LookPathDirFS was added.
fs.FS is read-only, so there will still be a split of responsibilities between the FS and OpenHandler
fs.FS does not allow "rooted" paths, so these paths are remapped, eg. "/" => ".", "/etc/passwd" => "etc/passwd" - returned errors that include the path will use these remapped forms.

I haven't added any tests yet, this is just a first pass to see if this is a good idea. I converted all the obvious filesystem I could find that made sense. There are a few issues though: 1. Yet another variant of LookPath called LookPathDirFS was added. 2. fs.FS is read-only, so there will still be a split of responsibilities between the FS and OpenHandler 3. fs.FS does not allow "rooted" paths, so these paths are remapped, eg. "/" => ".", "/etc/passwd" => "etc/passwd" - returned errors that include the path will use these remapped forms.

alecthomas · 2022-01-25T09:49:32Z

interp/interp_test.go

@@ -2903,7 +2903,7 @@ var runTestsUnix = []runTest{
 	},
 	{
 		"cd /; sure/is/missing",
-		"stat /sure/is/missing: no such file or directory\nexit status 127 #JUSTERR",
+		"stat //sure/is/missing: no such file or directory\nexit status 127 #JUSTERR",


It's not yet clear to me why this is happening.

mvdan · 2022-01-25T14:22:55Z

Yet another variant of LookPath called LookPathDirFS was added.

Entirely reasonable. We could clean those up with v4.

fs.FS is read-only, so there will still be a split of responsibilities between the FS and OpenHandler

Good catch. I've subscribed to golang/go#45757, though I don't think that will arrive soon, so we shouldn't just wait around.

We could consider designing our own extra methods on top of fs.FS, but my worry is that when upstream ends up adding their own, they could end up clashing. So I'd rather avoid that.

Your idea to keep using OpenHandler for write access seems fine. It still means that full coverage of the filesystem requires using two handlers rather than one, but I don't see another way until the upstream issue above is resolved.

fs.FS does not allow "rooted" paths, so these paths are remapped, eg. "/" => ".", "/etc/passwd" => "etc/passwd" - returned errors that include the path will use these remapped forms.

Also sounds reasonable.

mvdan · 2022-01-25T14:28:26Z

I wonder if replacing / with . somehow breaks Windows, given that their absolute paths also specify a volume.

mvdan

I think my biggest concern here is Windows; otherwise looks good!

mvdan · 2022-01-25T14:23:30Z

interp/api.go

@@ -59,6 +61,9 @@ type Runner struct {

 	alias map[string]alias

+	// Filesystem to use. Defaults to os.DirFS("/")


Suggested change

// Filesystem to use. Defaults to os.DirFS("/")

// Filesystem to use. Defaults to os.DirFS("/").

mvdan · 2022-01-25T14:25:25Z

interp/api.go

+// FS sets the fs.FS used for filesystem file and directory read operations.
+func FS(fs fs.FS) RunnerOption {
+	return func(r *Runner) error {
+		r.fs = fs


I would mimic what StdIO does with io.Discard; otherwise you can end up with a nil r.fs if the user provides FS(nil).

This also enables the user to overwrite a previous option with whatever our default is, without having to write out os.DirFS("/") themselves.

mvdan · 2022-01-25T14:28:47Z

interp/handler.go

+}
+
+// LookPathDirFS is like LookPathDir but retrieves all state from the Runner.
+func LookPathDirFS(vfs fs.FS, cwd string, env expand.Environ, file string) (string, error) {


but retrieves all state from the Runner

Hmm, what runner?

I think you should also document that you expect vfs to correspond to the root of the filesystem. Perhaps call it rootFS to clarify. I'd also document the same assumption in lookPathDir.

whoops, this is a leftover comment from a previous version of the function.

mvdan · 2022-01-25T14:29:39Z

interp/runner.go

+	if path == "/" {
+		return "."
+	}
+	return strings.TrimPrefix(path, "/")


This also sounds like it won't support Windows :)

mvdan · 2022-01-25T14:30:33Z

interp/runner.go

+	if err != nil {
+		return nil, err
+	}
+	infos := make([]os.FileInfo, len(dirents))


I'd perhaps add here:

// TODO(v4): use io/fs directly in ReadDir

mvdan · 2022-01-25T14:38:15Z

I understand that OpenHandler will still be used for reading files; that seems fine as well, and it's probably more consistent with what users will expect.

alecthomas · 2022-01-25T22:51:23Z

Mmm yeah I wonder how we can deal with Windows in a consistent manner.

mvdan · 2022-01-25T23:17:51Z

Perhaps document that, on Windows, the paths will be of the form C/foo/bar to correspond to the absolute path C:\foo\bar. We need the volume name to be there somewhere, I think.

alecthomas · 2022-01-25T23:55:06Z

How do multiple drives currently work in the interpreter? Are they in the form /<drive>/<path>?

alecthomas · 2022-01-26T00:53:42Z

It looks like, again, there isn't a great solution for Windows at the moment.

mvdan · 2022-01-26T09:02:13Z

Thanks for looking; looks like I was already subscribed, but I forgot that the issue existed.

How about we simply don't support Windows on this handler for the time being, and we cite golang/go#44279 as the reason why?

I only see two alternatives, neither of which are particularly better:

We only support using a rootFS on Windows which hard-codes a volume name, such as C:\. Perhaps enough for some basic use cases, but certainly limited.
We invent a way to bake the volume name into fs.FS paths, like the C/foo/bar I mentioned above. This is more flexible, but worries me because when os: hard to use DirFS for cross-platform root filesystem golang/go#44279 ultimately gets fixed, we'd probably be incompatible.

mvdan · 2022-01-26T09:02:53Z

How do multiple drives currently work in the interpreter? Are they in the form /<drive>/<path>?

The interpreter uses paths in the native form; so it can have Dir be C:\foo\bar or D:\etc on Windows.

mvdan · 2022-01-26T09:08:03Z

How about we simply don't support Windows

As I re-read my own comment, I realize that "simply" is incorrect. Your refactor swaps us to using io/fs.FS everywhere; so we'd have to instead do something like:

if runtime.GOOS == "windows" {
    os.Foo(absolutePath, ...)
} else {
    r.fs.Foo(relativePath, ...)
}

mvdan · 2022-01-26T09:13:36Z

Yet another option is to give up, say that io/fs isn't ready for our use per the reasons you listed in #783 (comment), and go back to implementing os-API-specific handlers such as ReadDirHandler.

It doesn't feel like a good solution long-term, as I'm pretty confident that we'll get some solution to io/fs for writing files and absolute roots on Windows. Still, that won't happen in the next 6-12 months, and I do plan to do a v4 of this module sometime in 2023, so perhaps that would be a better time to attempt to design around io/fs - at which point we could also tweak the API to align with it more neatly.

I slightly lean towards giving up for the time being. I really appreciate all the effort and patience you've invested here :) I would probably not have suggested that we try a io/fs handler had I realized that we'd run into its two shortcomings.

alecthomas · 2022-01-26T09:57:59Z

Yeah, I agree with that assessment.

I really appreciate all the effort and patience you've invested here :) I would probably not have suggested that we try a io/fs handler had I realized that we'd run into its two shortcomings.

No problem at all, it wasn't obvious at all to me that we'd hit these issues either. It was only a couple of hours of work, and it was worth a shot :)

DO NOT MERGE: keeping for future reference. See #799 as to why io/fs isn't ready. -- I haven't added any tests yet, this is just a first pass to see if this is a good idea. I converted all the obvious filesystem I could find that made sense. There are a few issues though: 1. Yet another variant of LookPath called LookPathDirFS was added. 2. fs.FS is read-only, so there will still be a split of responsibilities between the FS and OpenHandler 3. fs.FS does not allow "rooted" paths, so these paths are remapped, eg. "/" => ".", "/etc/passwd" => "etc/passwd" - returned errors that include the path will use these remapped forms.

mvdan · 2022-01-26T10:03:57Z

I've pushed your commit to a branch in this repo, to ensure we don't lose it for future reference in v4: https://github.com/mvdan/sh/tree/interp-handler-fs

Sounds like we can close this, and go back to reviewing your original PR. Thanks again!

alecthomas · 2022-01-26T10:30:48Z

BTW while doing this, it did cross my mind that perhaps it would be cleaner to just put all the different OS interactions behind a single Sandbox interface, rather than pass in an FS, an ExecHandler, etc.

type Sandbox interface {
  Open(ctx context.Context, path string, flag int, perm os.FileMode) (io.ReadWriteCloser, error)
  ReadDir(ctx context.Context, path string) ([]fs.DirEntry, error)
  Stat(ctx context.Context, path string) (fs.FileInfo, error)
  Exec(ctx context.Context, args []string) error
  Call(ctx context.Context, args []string) ([]string, error)
}

alecthomas changed the title ~~Initial support for fs.FS.~~ interp: Initial support for fs.FS. Jan 25, 2022

alecthomas commented Jan 25, 2022

View reviewed changes

mvdan reviewed Jan 25, 2022

View reviewed changes

mvdan closed this Jan 26, 2022

mvdan mentioned this pull request Jan 26, 2022

v4: all: consider io/fs changes to our APIs #630

Open

alecthomas deleted the aat/interp-fs branch January 26, 2022 10:30

codefromthecrypt mentioned this pull request Mar 18, 2022

wasi: allow users to configure a filesystem that can accept new files tetratelabs/wazero#390

Closed

mvdan mentioned this pull request Apr 14, 2022

interp: use custom handler for os.stat/os.Lstat operations #848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interp: Initial support for fs.FS. #799

interp: Initial support for fs.FS. #799

alecthomas commented Jan 25, 2022

alecthomas Jan 25, 2022

mvdan commented Jan 25, 2022

mvdan commented Jan 25, 2022

mvdan left a comment

mvdan Jan 25, 2022

mvdan Jan 25, 2022

mvdan Jan 25, 2022

alecthomas Jan 25, 2022

mvdan Jan 25, 2022

mvdan Jan 25, 2022

mvdan commented Jan 25, 2022

alecthomas commented Jan 25, 2022

mvdan commented Jan 25, 2022

alecthomas commented Jan 25, 2022

alecthomas commented Jan 26, 2022

mvdan commented Jan 26, 2022

mvdan commented Jan 26, 2022

mvdan commented Jan 26, 2022 •

edited

Loading

mvdan commented Jan 26, 2022 •

edited

Loading

alecthomas commented Jan 26, 2022

mvdan commented Jan 26, 2022

alecthomas commented Jan 26, 2022

		@@ -59,6 +61,9 @@ type Runner struct {

		alias map[string]alias

		// Filesystem to use. Defaults to os.DirFS("/")

interp: Initial support for fs.FS. #799

interp: Initial support for fs.FS. #799

Conversation

alecthomas commented Jan 25, 2022

alecthomas Jan 25, 2022

Choose a reason for hiding this comment

mvdan commented Jan 25, 2022

mvdan commented Jan 25, 2022

mvdan left a comment

Choose a reason for hiding this comment

mvdan Jan 25, 2022

Choose a reason for hiding this comment

mvdan Jan 25, 2022

Choose a reason for hiding this comment

mvdan Jan 25, 2022

Choose a reason for hiding this comment

alecthomas Jan 25, 2022

Choose a reason for hiding this comment

mvdan Jan 25, 2022

Choose a reason for hiding this comment

mvdan Jan 25, 2022

Choose a reason for hiding this comment

mvdan commented Jan 25, 2022

alecthomas commented Jan 25, 2022

mvdan commented Jan 25, 2022

alecthomas commented Jan 25, 2022

alecthomas commented Jan 26, 2022

mvdan commented Jan 26, 2022

mvdan commented Jan 26, 2022

mvdan commented Jan 26, 2022 • edited Loading

mvdan commented Jan 26, 2022 • edited Loading

alecthomas commented Jan 26, 2022

mvdan commented Jan 26, 2022

alecthomas commented Jan 26, 2022

mvdan commented Jan 26, 2022 •

edited

Loading

mvdan commented Jan 26, 2022 •

edited

Loading