Skip to content
This repository has been archived by the owner on Jun 19, 2019. It is now read-only.

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

Closed
yeputons opened this issue Jan 7, 2014 · 6 comments

Comments

@yeputons
Copy link
Contributor

yeputons commented Jan 7, 2014

Here's the two-years old issue: nodejs/node-v0.x-archive#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
Copy link
Contributor Author

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

@seanmwalker
Copy link
Contributor

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
Copy link
Owner

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@ryanmcgrath @seanmwalker Your new tests actually do not check anything: old version passes them too. I've fixed it here: yeputons@fd7461d . The rest looks OK, I've created a new PR: #74

@ryanmcgrath
Copy link
Owner

Merged, this should all be good I think.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants