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

Add fs.FS compatibility converter #184

Merged
merged 8 commits into from
Jun 6, 2023
Merged

Conversation

AndreRenaud
Copy link
Contributor

@AndreRenaud AndreRenaud commented Jun 3, 2023

This is pretty rudimentary, but it appears to be enough to server the image via http.FileServer.

There is some poor naming in the code. If this approach appears to be basically acceptable, I'd welcome some feedback on cleaning it up a bit.

@AndreRenaud AndreRenaud mentioned this pull request Jun 3, 2023
@deitch
Copy link
Collaborator

deitch commented Jun 4, 2023

This looks pretty good.

I do wish we could have each implementation return FS, so we could get a clean-looking:

fs, err := disk.CreateFilesystem(0, diskfs.TypeFat32)
fsys := fs.FS()

But the reality is that would be wasteful. It would require every filesystem implementation to implement FS(), when they all already have an interface they implement. So this approach you have here is superior:

fs, err := disk.CreateFilesystem(0, diskfs.TypeFat32)
fsys := filesystem.FS(fs)

As for naming and structure, I see no need for this to be in its package converter. I would place it right at the root of filesystem/, so you can do what is above: filesystem.FS(fs).

So maybe move this into filesystem. The rest works fine.

It does bother me that we cannot just have the returned filesystem from CreateFIlesystem be fs.FS compatible, but we cannot, as discussed here, so let's stick with your wrapper.

Finally, I am confused by the need for HTTPFS(). That seems extraneous to me. http package provides all we need, so just use it:

fs, err := disk.CreateFilesystem(0, diskfs.TypeFat32)
fsys := filesystem.FS(fs)
httpfs := http.FS(fsys)

Adding the complexity by saving a single call doesn't buy us much.

@AndreRenaud
Copy link
Contributor Author

Another option might be to move this to an internal directory, and then add the FS method to each implementation, which calls this. So we don't add a new exposed function, just expand the FileSystem interface? If we put it in an internal directory however, that does mean that 3rd parties wouldn't be able to re-use it. Is that a concern?

This would mean you'd get something like:

fs, err := disk.CreateFilesystem(0, diskfs.TypeFat32)
fsys := fs.FS()

But the fat32 implementation's function would just be (using the current naming - this will be changed soon):

fat32.go:
func (fat32 *FilSsystem) FS() fs.ReadDirFS {
   return converter.FS(fat32)
}

The HTTPFS() function was just to avoid the extreme nesting in the following long line:

http.Handle("/", http.FileServer(http.FS(converter.FS(fs))))

But I agree, it serves very little purpose, and the naming is horrible (HTTPFS is weirdly all caps). I'll drop it.

@deitch
Copy link
Collaborator

deitch commented Jun 5, 2023

This ready for review?

@AndreRenaud
Copy link
Contributor Author

Yes, I think so. The only question is whether it's worth adding the func FS() fs.ReadDirFS function to the FileSystem interface, and then implementing it using filesystem.FS for each of the 3 current implementations?


type fsFileWrapper struct {
File
stat os.FileInfo
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been using this with read-only images. But if this is to support read/write, then caching the stat info is probably a mistake.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't. fs.FS currently is read-only. Even its few sub-variants are read-only. If it helps, I have implemented a full read-write FS variant (fs.FS compatible) which includes support for case-sensitivity and hardlinks and chmod and chown, even on underlying filesystems that do not support it. If you want a link, just ask.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, fs.FS is read-only, but diskfs.FileSystem can be read/write, so this abstraction could break?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how. fs.FS is just a subset of what is here. I am comfortable with what it is.

@deitch
Copy link
Collaborator

deitch commented Jun 6, 2023

Your contribution is appreciated, @AndreRenaud !

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

Successfully merging this pull request may close these issues.

2 participants