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 file permissions APIs #651

Open
nex3 opened this issue Dec 9, 2020 · 3 comments
Open

Add file permissions APIs #651

nex3 opened this issue Dec 9, 2020 · 3 comments

Comments

@nex3
Copy link
Member

nex3 commented Dec 9, 2020

I'm looking at using this package to represent file structures that will be encoded to and from TAR files. The TAR format can represent full UNIX permissions, and in practice this is important when using them for packaging software—we want to ensure that executables in archives are marked as executable. Although this package can represent permissions through the FileStat object, it provides no way of setting them when using something like MemoryFileSystem which means I'd have no way of creating a TAR with a certain file marked as executable.

Of course, this is particularly tricky because dart:io itself doesn't support controlling permissions in any way, and this package's API is closely based on that interface. Since dart-lang/sdk#15078 has been open for seven years now, it doesn't seem likely that that's going to change any time soon. Given that, I see two potential options (although I may be missing others):

  1. Take the initiative and add FileSystemEntity.setMode() and .setModeSync() methods to the package:file API and implement them in MemoryFileSystem. This would immediately satisfy my requirements, but it has a few downsides:
  • These methods would need to throw UnsupportedErrors for LocalFileSystem, which could confuse users. This is already true for some APIs and some filesystems, though, so it may not be that big of a deal.
  • There's a possibility that these would conflict with dart:io's mode-setting APIs, if those APIs do get added. This could be mitigated by getting the dart:io team to sign off on the specific APIs we use.
  • Any external implementations of FileSystem will break, since they don't implement the new methods. That said, implementors of this library are at high risk of breakage anyway since any new dart:io method will inherently break them.
  1. Add an optional FileSystemEntityWithPermissions interface that adds .setMode() and .setModeSync() methods, and that individual implementors can opt into. This avoids breaking existing implementors and makes it explicit that some FileSystems support this API and others do not. However, it has its own issues:
  • It still runs the risk of being broken by dart:io down the line.
  • If an implementation like MemoryFileSystem wants to statically declare that it supports permissions, it has to update every reference to File, Directory, and Link to permissions-enabled variants.
  • If an implementation doesn't do the above, all its users users have to write as FileSystemEntityWithPermissions every time they want to set permissions.

My preference would be option 1. I'm happy to contribute an implementation if that sounds good.

@dnfield
Copy link
Contributor

dnfield commented Dec 9, 2020

What about creating a TarFileSystem class that derives from MemoryFileSystem and adds the desired attributes?

@dnfield
Copy link
Contributor

dnfield commented Dec 9, 2020

I would be pretty hesitant to add surface API to this library that is not supported on dart:io - as noted in the linked bug there, getting this right in a cross platform way is fairly complicated, and unless someone is willing to own the work to make that happen it'd be better to avoid having it here. It would almost certainly cause confusion for contributors to the Flutter tool, which extensively uses this package in both the implementation and testing.

If someone is willing to own that, the work may as well be done in dart:io rather than here. But if there's just an implementation of TarFile that extends MemoryFile that can do this, it's not quite as big a deal (and implementers will be more aware that if they want to write that file to the local fs they'll need to implement permissions on their own).

@nex3
Copy link
Member Author

nex3 commented Dec 9, 2020

It's not easy to create a custom filesystem implementation just to add a few methods on top of MemoryFileSystem—there's no way to make targeted additions like that, so we'd have to wrap every layer of the interface hierarchy manually, which is a considerable amount of work. It's also not a very general solution. Tar files are far from the only filesystems that support writeable permissions, even in the file ecosystem—the node_io package's Node filesystem would support them if it had the APIs to do so.

Given the glacial rate that dart:io evolves, I think it'll be necessary sooner or later for a generic filesystem API to be able to represent concepts beyond what the core libraries can do. File permissions are crucial for how UNIX users work with the filesystem, as are owners and groups which dart:io also doesn't provide access to. I don't think it's feasible to require that all of these changes be made upstream given how much more difficult it is to contribute to dart:io (needing C++ and native OS API knowledge) than it is to a package that's designed to work across multiple FS representations.

I think there are ways to mitigate the confusion for users, such as those in the Flutter ecosystem, who are working with filesystems that don't support certain APIs. That's already the case with watchable versus unwatchable filesystems, for example. Clear documentation and error messages will go a long way, and if it turns out that LocalFileSystem users end up trying to use permissions enough to be a serious problem then that's really good evidence that the dart:io team should prioritize that work and get us into a better state all around.

as noted in the linked bug there, getting this right in a cross platform way is fairly complicated, and unless someone is willing to own the work to make that happen it'd be better to avoid having it here

If you'll forgive me for nitpicking a bit: I don't think designing this is really all that hard. There's prior art for this in every other widely-used language, all the way down to Windows's emulation of POSIX filesystem APIs. But it's even easier for the file package, because you don't need to implement it for LocalFileSystem in the first place.

@devoncarew devoncarew transferred this issue from dart-archive/file.dart Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants