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

Using archiver.bulk & graceful-ncp over ncp to avoid EMFILE errors #123

Merged
merged 8 commits into from
Dec 12, 2014

Conversation

adam-lynch
Copy link
Contributor

@steffenmllr, I'll leave this for you to merge

Thanks, @felixSchl.

Closes #92, #118

adam-lynch and others added 5 commits November 16, 2014 16:02
@adam-lynch
Copy link
Contributor Author

Oh wait, hold off... I just saw this in the package.json:

"fs-extra": "git://github.com/adam-lynch/node-fs-extra#graceful-ncp",

@adam-lynch
Copy link
Contributor Author

Ok, I've created a PR; jprichardson/node-fs-extra#95. We could wait until it's merged or else just merge and then change it back to using node-fs-extra from npm once it is merged.

@adam-lynch
Copy link
Contributor Author

@felixSchl One of the tests were actually failing (see https://travis-ci.org/mllrsohn/node-webkit-builder/jobs/41839735#L245) but it wasn't failing the build because of #126.

The issue was with archive.bulk. The test is now passing with my changes in b69eca8. Could you have another look and make sure everything is ok?

@steffenmllr
Copy link
Contributor

@adam-lynch You know the codebase better than me at this point :) so feel free to merge this when you think it's right - I would however wait for the fs-extra thing to be merged.

I have to check why we even need fs-extra ...

@adam-lynch
Copy link
Contributor Author

@steffenmllr I think I'm going to go ahead and merge this later. We could be waiting forever on other people and it'll be a big benefit to merge (along with 64-bit support for 1.0.0).

Instead of waiting on a PR, I can create another npm package which just makes sure node-fs-extra uses graceful-ncp instead of ncp. It's only a one line package and would be better than having a github branch as a dependency since it'll only be updated when node-fs-extra publish a new release, not whenever they commit to master.

When the time comes, I'll switch the dependency to node-fs-extra instead if they merge my PR or if ncp merges my PR (AvianFlu/ncp#70), then I'll just close my node-fs-extra PR and revert our dependency to ncp instead of my graceful-ncp. Nothing much to worry about because my changes to all packages concerned are tiny, no real maintenance involved

Long story short, I'll probably just merge this and it'll be fine 😄.

adam-lynch added a commit that referenced this pull request Dec 12, 2014
Using archiver.bulk & graceful-ncp over ncp to avoid EMFILE errors
@adam-lynch adam-lynch merged commit fe0d686 into master Dec 12, 2014
@adam-lynch
Copy link
Contributor Author

Thanks a lot @felixSchl. I'd appreciate if you could test master (npm i -D mllrsohn/node-webkit-builder) before I release 1.0.0. I seem to getting an EMFILE error even with these changes 😢

@adam-lynch
Copy link
Contributor Author

Nevermind what I said about the EMFILE error, it was just a typo in my stubbing of ncp in graceful-fs-extra.

@adam-lynch adam-lynch deleted the graceful-ncp branch December 12, 2014 21:47
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.

EMFILE error if there are too many files
3 participants