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

[feat] [discussion] Extending filesystem interface #4017

Closed
11 of 19 tasks
dubiousjim opened this issue Feb 17, 2020 · 10 comments
Closed
11 of 19 tasks

[feat] [discussion] Extending filesystem interface #4017

dubiousjim opened this issue Feb 17, 2020 · 10 comments
Labels

Comments

@dubiousjim
Copy link
Contributor

dubiousjim commented Feb 17, 2020

I have a queue of refinements/extensions to submit for Deno's interface to the filesystem (especially for Unix).

Except where noted, I have working implementations of all of these and will be submitting PRs over the next few days, as I get them rebased and cleaned up for review.

This issue will summarize what's coming, and house discussion/feedback.

The core changes are:

  • exposing umask Add op_umask #4290

  • exposing fsync and fdatasync

  • exposing fstat, ftruncate, fchmod, and futimes (that is, versions of Deno's stat, truncate, chmod, and utime functions, and their Sync counterparts, that take open rids/fds as arguments instead of paths)

  • putting ctime into the responses from stat

  • making seek return the ending position of the file cursor (see want: Deno.seek/seekSync to return position in file #3826) (This was already fixed by sync/async seek returns cursor position #4211.)

  • adding a mode: option to truncate, and open, to be used when these functions create a new file. mkdir and writeFile already have such functionality (mkdir as mode, writeFile as perm). (Addressed by Add mode to truncate #4288, Add mode option to open/create #4289.)

  • Together with the previous, changed the implementation of writeFile in such a way that its permissions changes are only applied when a file is newly created, and honor the system's umask.

  • added create and createNew fields to truncate, copyFile, and writeFile (the last one already had create). create defaults to true and allows creating a new file if necessary. createNew defaults to false and fails if there's already a file there. (Same behavior as with open.)

Along the way, I accumulated a number of smaller changes that seemed worthwhile to commit, especially in light of the changes proposed above, and in light of the fact that API renaming etc would be better before Deno 1.0.

  • there's a lot of variation in the doc comments and internal code about whether the 0o777-style file permissions are called mode or perm (see mkdir and writeFile, mentioned above). This is exacerbated by mode also being used in other ways, e.g., to label whether a file is being opened as r+, w, etc. PR Rename mode to perm #4245 made the 0o777-things consistently be called perm, but mode was more popular, so Rename perm to mode #4276 instead makes them consistently be called mode.

  • there's a lot of variation in the doc comments and the internal code about whether the first parameter to file system calls is path, or name or filename. I made it consistently path. (Addressed by Rename name/filename arguments to path #4227.)

  • I added an isDir field to the StatResponses. Deno's existing behavior of saying that isDir == !isFile && !isSymlink is broken. That counts fifos, block devices, and so on as directories. Also, I've added a FileType enumeration and field FileInfo.type. So statSync(path).type might be FileType.TYPE_FIFO.

  • given that the file type is now exposed by FileInfo.type, I masked the FileInfo.mode field to 0o7777.

  • made sure that mode arguments (e.g., in chmod, mkdir, writeFile, and the extensions to truncate, and open) were always masked by 0o777 before being applied. (Partly addressed by Tweaks to permission handling in Rust filesystem modules #4228 More careful permissions handling #4285; and by Add mode to truncate #4288, Add mode option to open/create #4289; writeFile still in queue.)

  • Made makeTempDir use 0o700 permissions, and makeTempFile use 0o600 (Addressed by Stricter permissions for make_temp #4318.)

  • Since we're renaming things, I figured we might as well go ahead and rename readDir to readdir, as comments in the code threaten might be coming. (readdir is the spelling that Node uses; addressed by Rename readDir -> readdir #4225.)

  • I also changed MkdirOption to MkdirOptions (it was hard to remember which interfaces were Options and which were Option, and anyway this one contained two options); then for consistency I renamed RemoveOption to RemoveOptions. (It was the last Option[s] interface still written in the singular. (Addressed by Rename Option -> Options #4226.)

  • I also changed the FileInfo.len field to FileInfo.size, as comments in the code threaten might be coming, and better fits other parts of the Javascript API. (Addressed by FileInfo len->size #4338.)

  • improved the way that permissions were set by cli/fs.rs:write_file (this isn't used to implement Deno.writeFile, but internally by the fetcher, compiler, disk_cache). (Addressed by Tweaks to permission handling in Rust filesystem modules #4228 More careful permissions handling #4285.)

  • MkdirOptions, unlike analogous interfaces, wasn't being exported in cli/js/deno.ts (Addressed by Rename Option -> Options #4226.)

@ak-1
Copy link

ak-1 commented Feb 23, 2020

I would like to see the Deno.readDir (or readdir) API improved upon.
In Typescript strict mode (or after upgrading to deno 0.34) you have to unnecessarily check for fileInfo.name != null after Deno.readDir.
If you look at https://golang.org/pkg/os/#FileInfo there FileInfo is an interface.
You call fileInfo.Name(). You don't have to deal with null values.
Since deno tries to emulate golang API this is probably the way to go.

@dubiousjim
Copy link
Contributor Author

Ugh. Days of rebasing to clean up history, and catch up with denoland/master, and then just as I'm about to post, find more changes to lib.deno.ns.d.ts to synch up with... :-( I'll get there soon.

This was referenced Feb 29, 2020
@dubiousjim
Copy link
Contributor Author

Posted implementations as PRs #4192, #4193, #4195, #4196.

@cknight
Copy link
Contributor

cknight commented Mar 1, 2020

@dubiousjim Will your change introduce permission setting for the scenario where Deno.open() creates a new file (via create or createNew options)? E.g.

Deno.openSync('does_not_exist_yet.txt', {create: true, write:true, read: true});

will create a file with 0o644 permissions, but I don't see any way to influence the permissions used on creation. I was about to raise an issue when I remembered you were doing a lot of work here.

@dubiousjim
Copy link
Contributor Author

Yes, that's one of the things I implemented. Default is to use 0o666, subject to the process's umask, so you get 0o644. But you can supply anything you like. Currently the supplied perm is masked to 0o777, but we could do 0o7777 instead. The kernel always applies the umask, so you can't get around that except with an explicit chmod call.

@dubiousjim
Copy link
Contributor Author

The complete sequence can be found at this branch; but I can only push the PRs into this repository gradually, as some of them need to wait upon others being merged into master first.

ry pushed a commit that referenced this issue Mar 20, 2020
This a complex boring PR that shifts around code (primarily) in cli/fs.rs and
cli/ops/fs.rs. The gain of this refactoring is to ease the way for #4188 and
#4017, and also to avoid some future development pain.

Mostly there is no change in functionality. Except:
* squashed bugs where op_utime and op_chown weren't using `resolve_from_cwd`
* eliminated the use of the external `remove_dir_all` crate.
* op_chmod now only queries metadata to verify file/dir exists on Windows (it
  will already fail on Unix if it doesn't)
* op_chown now verifies the file/dir's existence on Windows like chmod does.
dubiousjim added a commit to dubiousjim/deno that referenced this issue Mar 20, 2020
Currently chmod is a noop on Windows, while analogous functions like
chown and umask throw (see denoland#4306). Symlink also currently throws, but
that can be implemented on Windows, it just hasn't been done yet
(see denoland#815, an implementation was crafted but there were issues that
haven't been resolved yet).

It's possible to implement chmod in Windows to a limited degree: Rust
lets one make Windows files read-only. But the API for how this should
work hasn't been settled (see denoland#4357). Until we do implement chmod on
Windows, perhaps we should make it throw like the other
Windows-unimplemented functions in /cli/ops/fs.rs do?

If so, this PR provides an implementation, and fixes tests accordingly.

Note that the current implementation of writeFile applies its `mode`
option to the file regardless of whether the file was created (and does
so using chmod, so it has to be touched by this PR). A forthcoming PR in
the denoland#4017 series will make writeFile instead use the new `mode` option
in open for new files, and so no longer rely on chmod.
dubiousjim added a commit to dubiousjim/deno that referenced this issue Mar 20, 2020
Currently chmod is a noop on Windows, while analogous functions like
chown and umask throw (see denoland#4306). Symlink also currently throws, but
that can be implemented on Windows, it just hasn't been done yet
(see denoland#815, an implementation was crafted but there were issues that
haven't been resolved yet).

It's possible to implement chmod in Windows to a limited degree: Rust
lets one make Windows files read-only. But the API for how this should
work hasn't been settled (see denoland#4357). Until we do implement chmod on
Windows, perhaps we should make it throw like the other
Windows-unimplemented functions in /cli/ops/fs.rs do?

If so, this PR provides an implementation, and fixes tests accordingly.

Note that the current implementation of writeFile applies its `mode`
option to the file regardless of whether the file was created (and does
so using chmod, so it has to be touched by this PR). A forthcoming PR in
the denoland#4017 series will make writeFile instead use the new `mode` option
in open for new files, and so no longer rely on chmod.
@lucacasonato
Copy link
Member

@dubiousjim Deno.fsync, Deno.fstat and Deno.ftruncate are now supported.

@caspervonb
Copy link
Contributor

Also tracking futimes in #6563

@caspervonb
Copy link
Contributor

caspervonb commented Sep 1, 2020

#6563 has been closed.

I think I've added everything that this was missing and this issue can be closed.

If we're still missing syscall ops, i'd prefer to track them individually.

@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 6, 2021
@stale stale bot closed this as completed Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants