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

Functionality missing on Windows: should it throw or noop? #4306

Closed
dubiousjim opened this issue Mar 9, 2020 · 8 comments
Closed

Functionality missing on Windows: should it throw or noop? #4306

dubiousjim opened this issue Mar 9, 2020 · 8 comments

Comments

@dubiousjim
Copy link
Contributor

dubiousjim commented Mar 9, 2020

The codebase isn't consistent about what happens when requesting functionality on Windows that isn't yet (or can't be) implemented there.

  • chown -- currently throws
  • chmod -- currently noop
  • symlink -- currently throws
  • kill(pid, signal) -- currently noop
  • mkdir with non-default mode -- currently noop

Some functionality that I've pushed or have queued to push is also undecided:

  • umask(mask) (Add op_umask #4290) -- currently throws
  • fchmod -- queued implementation will throw
  • futime -- queued implementation will throw
  • fstat -- queued implementation will throw
  • open with non-default mode (Add mode option to open/create #4289) -- currently noop
  • truncate with non-default mode (Add mode to truncate #4288) -- currently noop
  • writeFile with non-default mode -- queued implementation uses open(..., {mode:...})

What do people think we should do in these cases? Throw? or noop? I hope we can settle on a consistent pattern... though there might turn out to be corner cases where it makes sense to diverge from it.

This was referenced Mar 9, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Mar 9, 2020

For anything in the Deno namespace, I think it should throw. Ideally it would be a consistent error type, if we don't have it already like Deno.errors.OsUnsupported or something like that.

For things in std... I think it "depends"... Obviously for things in std/node it should behave like Node.js does. For other things, I think it really depends upon the API and the use case. Failing softly can be a good API design for some use cases.

@dubiousjim
Copy link
Contributor Author

@ry preferred for open with non-default mode and mkdir with non-default mode to just silently ignore the mode; but made umask(mask) throw. I guess the underlying pattern is: there's something for open and mkdir still to do, they just ignore the keyword argument. Whereas umask has nothing to do on Windows in the current implementation, and would have to ignore a positional argument: umask() just returns a constant 0 on Windows.

Continuing that pattern, I'm thinking truncate should silently ignore a non-default mode, and chmod and kill should be made to throw. It's OK to leave fchmod, futime and fstat throwing.

Or do others think differently?

@bartlomieju
Copy link
Member

Just hit this problem with kill which is silently a no-op: https://github.com/denoland/deno/pull/4397/checks?check_run_id=513464845

I'd say, that should be a hard error - it's better than silently hanging in some situation because op had no effect on Windows.

@dubiousjim
Copy link
Contributor Author

@kitsonk, recently added to Rust there's crate::op_error::OpError::not_implemented; and we have notImplemented() in cli/js/util.ts. But that's not exported to the Deno namespace, so not available outside of cli.

@dubiousjim
Copy link
Contributor Author

Whereas umask has nothing to do on Windows in the current implementation, and would have to ignore a positional argument: umask() just returns a constant 0 on Windows.

Slight error in what I wrote there. The version of umask that ended up in master throws on Windows regardless of whether a mask is supplied or not.

@kitsonk
Copy link
Contributor

kitsonk commented Mar 17, 2020

Having a Deno.notImplemented() would seem a little odd to me. Having an Deno.errrors.NotImplemented would. People don't need an exposed function to be able to throw something, do they?

@dubiousjim
Copy link
Contributor Author

Having a Deno.notImplemented() would seem a little odd to me.

I agree

dubiousjim added a commit to dubiousjim/deno that referenced this issue Mar 18, 2020
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.
@dubiousjim
Copy link
Contributor Author

Given recent commits, chmod and kill now throw on Windows. So now everything looks systematic:

  • chown, chmod, kill, umask: all throw
  • symlink: now throws, but this can be implemented later
  • fchmod, futime, fstat, etc: these will throw
  • supplying non-default mode to open, writeFile, truncate, mkdir: these are just ignored

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

No branches or pull requests

3 participants