use fs-extra copy instead of ncp #318

develar · 2016-04-10T14:09:21Z

electron-builder tests passed as well.

malept · 2016-04-10T16:06:14Z

docs/api.md

@@ -220,7 +220,7 @@ Object (also known as a "hash") of application metadata to embed into the execut

 ### `err`

-*Error* (or *Array*, in the case of an `ncp` error)
+*Error* (or *Array*, in the case of an `copy` error)


Does copy actually create an Array on erroring? Because that would be very unfortunate.

I answered my own question below.

We can pass stopOnError: true to stop on first error.

We should probably update all of the copy calls to use that (in a separate PR). Having err be potentially more than one type is problematic for users.

malept · 2016-04-10T16:19:06Z

@develar I looked into how fs-extra implemented copy, because you didn't change any of the options passed. It turns out that it's just a wrapper for ncp. Could you explain how this change would fix all the issues you mention?

malept · 2016-04-10T16:33:51Z

Hm. I just realized that it uses a forked copy of ncp. But just because it may have some fixes in it, doesn't necessarily mean that all of those issues are resolved. We also don't have any tests for #150 or #213. If you want to assert that those are resolved, please write testcases for them.

malept · 2016-04-10T16:50:31Z

test/fixtures/basic/package.json

@@ -4,6 +4,8 @@
  "dependencies": {
    "run-series": "^1.1.1"
  },
+  "//": "ncp used to test https://github.com/electron-userland/electron-packager/pull/186",
+  "///": "(a module (with zero dependencies) that creates a file in node_modules/.bin)",


That is a really unfortunate way to annotate JSON files.

Yes, but npm doesn't support other ways (e.g. typescript supports comments in the tsconfig.json) :(

malept · 2016-04-10T16:56:33Z

By the way, it might be better to squash your commits when the PR has been approved. It makes it easier for reviewers to see what changed, especially now that GitHub has added features to the pull request UI.

develar · 2016-04-10T16:57:50Z

There is a comment AvianFlu/ncp#98 (comment) so, I think, it is definitely fixed in the fs-extra. I don't see any particular file sets in the #150 and #213 so, I am not sure how I can test it. May be we can trust @jprichardson and existing fs-extra tests?

develar · 2016-04-10T16:59:56Z

it might be better to squash your commits

Ok, I will not amend commits anymore.

malept · 2016-04-10T17:00:51Z

I'm not so sure the ncp issue you mention is fixed in fs-extra. If it is, it wasn't obvious from the commit log. My reading of his comment was more along the lines of "here's a maintained version of ncp".

jprichardson · 2016-04-10T20:32:54Z

I'm not so sure the ncp issue you mention is fixed in fs-extra. If it is, it wasn't obvious from the commit log. My reading of his comment was more along the lines of "here's a maintained version of ncp".

To my knowledge, I've fixed most of the outstanding issues in my forked copy of ncp. If there's anything you're unsure of, let me know and I can take a second look.

malept · 2016-04-10T20:41:05Z

@jprichardson the issue I'm unsure that it fixes is AvianFlu/ncp#98. Another look through the history (GitHub does not make that easy due to the multiple renames) indicates that it may be the same as jprichardson/node-fs-extra#98 (coincidentally enough)?

jprichardson · 2016-04-10T20:51:23Z

AvianFlu/ncp#98 leaves out a lot of details, so it's unclear of the problem. The commenter just states that the callback is never being called with no code to reproduce. Who knows why. I put a ton of work into fixing the issues in ncp (as seen in jprichardson/node-fs-extra#98) and others.

Also, I can state that I use electron-packager for my startup (http://www.exodus.io - depends upon Electron), so I'm heavily invested in the Electron ecosystem (if it matters).

malept · 2016-04-10T20:56:26Z

@jprichardson @develar OK, sounds good to me. At minimum, it gets rid of an extra dependency (I've already gotten rid of direct production dependencies on mv, rimraf, and mkdirp in favor of fs-extra since this PR was filed).

@develar once you rebase, LGTM.

Closes #213, #150, #151

malept · 2016-04-11T15:09:27Z

@develar FYI your build failed, seems you missed some ncp references.

jprichardson · 2016-04-11T15:14:57Z

Oh, I wanted to add one additional point. If it matters, fs-extra will be dropping support for Node v0.10 within the next few months. I wouldn't think this would be a big deal, but if you're looking to continue with Node v0.10 support (I couldn't imagine why), you'll be stuck in time with the last version of fs-extra to support Node v0.10. I don't see how this situation is any worse than the unmaintained ncp though. I just wanted to make sure you had all info up front before you make your decision.

malept · 2016-04-11T15:18:09Z

It's been a while since we've supported 0.10 officially. I think I had to remove support for it in Travis CI for some feature, and to my knowledge there have been no complaints. At any rate, I filed a PR the other day that definitely removes support for Node < 4.0. So we're OK 😄

Closes #213, #150, #151

develar · 2016-04-11T18:27:39Z

Sorry, fixed.

max-mapper · 2016-04-11T19:13:17Z

Just wanted to point out this doesnt close #151, that issue is about using tar-fs, which IMO is a lot more dependable performance wise than fs-extra/ncp.

malept · 2016-04-11T19:57:11Z

Removed #151 from the issue summary.

malept · 2016-04-11T19:58:13Z

@develar squash and fix the commit message to omit #151 and I will merge.

jprichardson · 2016-04-11T20:42:56Z

which IMO is a lot more dependable performance wise than fs-extra/ncp.

Would love to know why so that I can improve fs-extra.

Overall though, tar-fs, cpy, fs-extra... puh-tay-toe/puh-tah-toe. I don't care much as long as we get a better electron-packager and I'm happy to assist.

develar · 2016-04-11T20:52:20Z

which IMO is a lot more dependable performance wise than fs-extra/ncp.

It is a bit illogical for me. How does tar work — read file and append content to resulting (total) file. And then we will read total file and split it to destination files.

So, why it will be faster? copy operation just transfer bytes from one file to another.

Or do you mean that copy is not reliable? Well... if nobody can write reliable copy implementation on node platform... But I don't think so — I have tried 3 different implementations and only fs-extra copy works for me (electron-builder tests — copying test fixtures). And recently windows-installer got rid of jetpack and switched to fs-extra.

So, #151 was mentioned not by mistake.

develar · 2016-04-11T20:54:48Z

Apart of this, I am going to get rid of copy phase (copy app files) at all :) Because if prune is not specified, we can build asar archive directly, without intermediate copy (asar is default in the electron-builder, so, it is important for me).

malept · 2016-04-12T16:15:59Z

That "squash and merge" feature in GitHub's UI is pretty great. Won't work all the time, but worked well in this instance.

malept reviewed Apr 10, 2016
View reviewed changes

malept added the tests-needed Pull request needs tests label Apr 10, 2016

develar force-pushed the remove-ncp branch from 741dc93 to 162948d Compare April 10, 2016 16:46

malept reviewed Apr 10, 2016
View reviewed changes

malept removed the tests-needed Pull request needs tests label Apr 10, 2016

use fs-extra copy instead of ncp

e502853

Closes #213, #150, #151

develar force-pushed the remove-ncp branch from 162948d to e502853 Compare April 11, 2016 06:02

use fs-extra copy instead of ncp

6e6e3bc

Closes #213, #150, #151

develar mentioned this pull request Apr 12, 2016

Remove temp fork dependencies electron-userland/electron-builder#195

Closed

malept merged commit d9ef3f5 into electron:master Apr 12, 2016

malept added this to the 7.0.0 milestone Apr 12, 2016

develar deleted the remove-ncp branch April 12, 2016 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use fs-extra copy instead of ncp #318

use fs-extra copy instead of ncp #318

develar commented Apr 10, 2016

malept Apr 10, 2016

malept Apr 10, 2016

develar Apr 10, 2016

malept Apr 10, 2016

malept commented Apr 10, 2016

malept commented Apr 10, 2016

malept Apr 10, 2016

develar Apr 10, 2016

malept commented Apr 10, 2016

develar commented Apr 10, 2016

develar commented Apr 10, 2016

malept commented Apr 10, 2016

jprichardson commented Apr 10, 2016

malept commented Apr 10, 2016

jprichardson commented Apr 10, 2016

malept commented Apr 10, 2016

malept commented Apr 11, 2016

jprichardson commented Apr 11, 2016

malept commented Apr 11, 2016

develar commented Apr 11, 2016

max-mapper commented Apr 11, 2016

malept commented Apr 11, 2016

malept commented Apr 11, 2016

jprichardson commented Apr 11, 2016

develar commented Apr 11, 2016

develar commented Apr 11, 2016

malept commented Apr 12, 2016

use fs-extra copy instead of ncp #318

use fs-extra copy instead of ncp #318

Conversation

develar commented Apr 10, 2016

malept Apr 10, 2016

Choose a reason for hiding this comment

malept Apr 10, 2016

Choose a reason for hiding this comment

develar Apr 10, 2016

Choose a reason for hiding this comment

malept Apr 10, 2016

Choose a reason for hiding this comment

malept commented Apr 10, 2016

malept commented Apr 10, 2016

malept Apr 10, 2016

Choose a reason for hiding this comment

develar Apr 10, 2016

Choose a reason for hiding this comment

malept commented Apr 10, 2016

develar commented Apr 10, 2016

develar commented Apr 10, 2016

malept commented Apr 10, 2016

jprichardson commented Apr 10, 2016

malept commented Apr 10, 2016

jprichardson commented Apr 10, 2016

malept commented Apr 10, 2016

malept commented Apr 11, 2016

jprichardson commented Apr 11, 2016

malept commented Apr 11, 2016

develar commented Apr 11, 2016

max-mapper commented Apr 11, 2016

malept commented Apr 11, 2016

malept commented Apr 11, 2016

jprichardson commented Apr 11, 2016

develar commented Apr 11, 2016

develar commented Apr 11, 2016

malept commented Apr 12, 2016