search

ryanmcgrath / wrench-js

Recursive file operations in Node.js
MIT License
435 stars 71 forks source link

`fs.unlink` behaves differently on Windows and Linux, and so `rmdirSyncRecursive` does #70

Closed yeputons closed 10 years ago

yeputons commented 10 years ago

Here's the two-years old issue: https://github.com/joyent/node/issues/3006 , still no fix. This affects rmdirSyncRecursive, for example.

However, I can suggest a dirty workaround: when on Windows, change this:

            fs.unlinkSync(_path.join(path, files[i]));

into this:

            fs.chmodSync(_path.join(path, files[i]), '0666');
            fs.unlinkSync(_path.join(path, files[i]));

Unfortunatelly, this affects a real project - Meteorite, which currently does not work on Windows. This bug is one of stones that bother me.

yeputons commented 10 years ago

@ryanmcgrath Would you mind taking a look at this issue?

seanmwalker commented 10 years ago

I've submitted a pull request with this solution, really looking forward to having the published module to finish testing the meteorite fixes. Thanks Ryan for doing what you can to process this while traveling!

ryanmcgrath commented 10 years ago

@yeputons Appreciate the effort/attention, apologies for lack of a response. As @seanmwalker indicated I'm tied up a bit at the moment, but will be sparing some time for pull requests/etc. Will definitely see if we can't get everything righted here.

Thanks guys!

seanmwalker commented 10 years ago

@ryanmcgrath It's in now. I've added a number of tests, canceled the first pull request so I could add a few more to be sure it's validated. It's looking good on windows and ubuntu at this time. Not much for changes, just a file var, a flag for windows, and some extra if statements to chmod if it's windows. Let me know if there are any concerns or if there is anything else I can do. I'm heading out for the evening, its 1:40 AM here. But I'll be on tomorrow again if anything comes up. Thanks again!

yeputons commented 10 years ago

@ryanmcgrath @seanmwalker Your new tests actually do not check anything: old version passes them too. I've fixed it here: https://github.com/yeputons/wrench-js/commit/fd7461dc446af9e34c592ab51168b529b04a2527 . The rest looks OK, I've created a new PR: https://github.com/ryanmcgrath/wrench-js/pull/74

ryanmcgrath commented 10 years ago

Merged, this should all be good I think.