install: Update fs.access to allow io.js versions w/ fixed windows ver #9038
Conversation
| @@ -2,17 +2,9 @@ | |||
| var fs = require('fs') | |||
| var inflight = require('inflight') | |||
| var accessError = require('./access-error.js') | |||
| var isFsAccessAvailable = require('./is-fs-access-avalable.js') | |||
There was a problem hiding this comment.
avalable should have been available?
iarna
force-pushed
the
iarna/allow-fs-access-in-new-iojs-on-win
branch
from
b773b5d
to
4ac2a1e
Compare
iarna
force-pushed
the
iarna/allow-fs-access-in-new-iojs-on-win
branch
from
4ac2a1e
to
7c1674f
Compare
This also pushes them off to their own file, so we can stop doing the copy-pasta dance. It also makes explaining the rational of the check cleaner I think. PR-URL: #9038
iarna
force-pushed
the
iarna/allow-fs-access-in-new-iojs-on-win
branch
from
7c1674f
to
25d4906
Compare
|
🐑 |
| module.exports = true | ||
| } else { | ||
| // The Windows implementation of `fs.access` has a bug where it will | ||
| // sometimes return access errors all the time for directories, even |
There was a problem hiding this comment.
sometimes returns ... all the time?
There was a problem hiding this comment.
Under some circumstance that I don't understand, it always claims directories to be unwritable. These circumstances don't show up all the time, but when they do, they always result in failures.
At least, that was how I understood someone else's explanation of the bug. TBH, I haven't read the source material enough to summarize it directly.
There was a problem hiding this comment.
No problem. We can leave it as it is then.
There was a problem hiding this comment.
Under some circumstance that I don't understand, it always claims directories to be unwritable. These circumstances don't show up all the time, but when they do, they always result in failures.
from trial and error: if directories would require admin access they are reported as unwritable even if running as admin
There was a problem hiding this comment.
does it matter that the fix in io. js was to always return true in windows for directories?
Are you just relying on someone adding functionality to fs.access? what happens if it doesnt detect access.. will npm not roll back correctly?
There was a problem hiding this comment.
@lukeapage
If fs.access is just blindly returning true on Windows, then yes, that is a problem. I'm going to have to go digging through the libuv source, clearly.
The only problem with the permissions checks being too permissive, is that you get failures later rather than earlier. It shouldn't otherwise cause issues, but I'd still like to fix it. My fallback code just tries making a file in the directory to see if it has permission, and perhaps we'll have to permanently use that.
There was a problem hiding this comment.
There was a problem hiding this comment.
That would be much appreciated, but I don't see any links?
There was a problem hiding this comment.
Ok, I dug up the source, and yeah, I see it. Blerg. Ok, I guess I'll just ditch using access entirely.
|
LGTM with a nit |
| @@ -0,0 +1,24 @@ | |||
| 'use strict' | |||
| var fs = require('fs') | |||
| var process = require('process') | |||
There was a problem hiding this comment.
This is not necessary.
Edit: Ah, I think this is how the version is injected in the tests. Nvm then.
There was a problem hiding this comment.
Yup. I didn't even know I could require process until I went to write these tests =D
|
Thanks. Learned a lot from this PR. LGTM, if that matters :) |
This also pushes them off to their own file, so we can stop doing the copy-pasta dance. It also makes explaining the rational of the check cleaner I think. PR-URL: #9038
This also pushes them off to their own file, so we can stop doing the copy-pasta dance. It also makes explaining the rational of the check cleaner I think. PR-URL: #9038
|
@iarna fyi => https://github.com/sindresorhus/fs-access Many Node.js polyfills here: https://github.com/sindresorhus/awesome-nodejs#polyfills |
|
@sindresorhus You're just stating and checking the results there? Is that actually all the underlying mechanism does? I guess I should go look at My main concern is that there are a lot of other, OS specific, ways of making a directory unwritable that can't be sussed from unix-style permissions. My fallback approach is to actually try to make a file and see if that works, which I guess is the duck-typing of folder permission checking. If |
|
Thanks for the critical eye here @thefourtheye – This landed as 28ebc6c with some changes, as it turns out you can only I was able to instrument things in a different way for node 0.12 and io.js, but 0.10 eluded me and ultimately I ended up making the test skip on that target. =/ |
This also pushes them off to their own file, so we can stop doing
the copy-pasta dance. It also makes explaining the rational of the
check cleaner I think.