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

Fix copyFile method #148

Merged
merged 2 commits into from
Jan 28, 2015
Merged

Fix copyFile method #148

merged 2 commits into from
Jan 28, 2015

Conversation

bambooCZ
Copy link
Contributor

@bambooCZ bambooCZ commented Jan 7, 2015

When file doesn't exist after copy, DO NOT COPY IT AGAIN. We will only check its existence again.

fixes #147

@adam-lynch
Copy link
Contributor

We've been having some serious problems around this. See #142.

When you say "Fix copyFile method", what exactly does this fix? Because I think there might be two / three problems overall.

@bambooCZ
Copy link
Contributor Author

bambooCZ commented Jan 9, 2015

Acording #142, I had the same problem with .icns file. This is fixed in this fix.

IMO, the problem is that when a file is not in destination after copy, another copying is executed.
Solution is not to execute copying again, but only check its existence later.

And another minor fix is that the method doesn't print something like chmod failed everytime it is executed (#147).

It works for me. I am using my modification and I haven't encountered this (or any other) problem again.

@bambooCZ
Copy link
Contributor Author

bambooCZ commented Jan 9, 2015

But yes this is just a workaround. This should be fixed in graceful-fs-extra.copy() method. It should not call the callback before the file is truly there.

@adam-lynch
Copy link
Contributor

Ok cool, I'll test this today

@swissmanu
Copy link

this fix work for me on osx 10.10 as also on my travis ci build... would really appriciate a merge to the official version of node-webkit-builder

@dubcanada
Copy link

This fixed my issue on Windows.

@steida
Copy link

steida commented Jan 27, 2015

Can be this PR merged pls?

@bambooCZ
Copy link
Contributor Author

Are there any problems with it? Adam, have you tested it? Do you need any explanation? Or what is taking you so long?

@adam-lynch
Copy link
Contributor

Completely forgot to test it. I'm crazy busy. I'll try test this tonight I swear 😄.

@steida
Copy link

steida commented Jan 27, 2015

Awesome.

adam-lynch added a commit that referenced this pull request Jan 28, 2015
@adam-lynch adam-lynch merged commit 413df87 into nwutils:master Jan 28, 2015
@adam-lynch
Copy link
Contributor

I still get the EMFILE myself but this does look like an improvement so I'm merging it in.

@steida
Copy link

steida commented Jan 28, 2015

emfile? ulimit -n 10000 does not work?

@adam-lynch
Copy link
Contributor

@steida there's no ulimit command on Windows.

@bambooCZ bambooCZ deleted the fuckedupCopyFile branch January 28, 2015 09:44
@jminnick
Copy link

jminnick commented Feb 7, 2015

It still seems like there is some type of threshold limit on windows. I can reproduce this "Error: EMFILE, open" issue reliably.

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

Successfully merging this pull request may close these issues.

Utils.js: function copyFile always prints "chmod <perms> on <file> failed after copying, ignoring".
6 participants