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

1.0.2 seems to be broken somewhat #142

Closed
adam-lynch opened this issue Dec 18, 2014 · 13 comments
Closed

1.0.2 seems to be broken somewhat #142

adam-lynch opened this issue Dec 18, 2014 · 13 comments
Labels
bug Priority: 1

Comments

@adam-lynch
Copy link
Contributor

It seems #140 did break something after all... my head is pounding since I was up til 4am working on something. Could you see what's up with it @bastimeyer?

When building on Mac, I get an ENOENT error when its copying the .icns icon.

When building on Windows, fs.copy in utils.copyFile is giving an EMFILE error.

In both cases above I'm just trying to build for osx32 only. It builds fine for Windows I think, at least when building on Windows.

Going back to 1.0.0 when building on Mac means it builds fine but there's no icon, I think. And going back to 1.0.0 has no effect on Windows, I think.

The windows problem may be a separate issue altogether but both are problems with copying. It's very strange.

cc @lucienimmink, @cbabos

@adam-lynch adam-lynch added the bug Priority: 1 label Dec 18, 2014
@bastimeyer
Copy link
Contributor

It's not #140 which is causing the issue here. It's most likely the missing changes from the win64+osx64 platforms merge, because until #140, you just didn't do anything inside the handleWinApp and handleMacApp methods (see the platform name).

I don't know what's causing the issue, but all build options worked fine for me while building on linux (checked the icon of the win apps in a VM - compared the files of the osx apps in the respecting app folder).

Hadn't had a deeper look into it (don't have the time right now), but my first thoughts are, that it might be because of the value of platform.releasePath inside the platforms loop in handleWinApp and handleMacApp... createReleaseFolder is called before both methods and doesn't return a promise while creating the folder and setting the property. Doing this asynchronously while returning a proper Promise could fix the issue, I think.

@adam-lynch
Copy link
Contributor Author

@bastimeyer thanks. I'll look into it a bit today

@adam-lynch
Copy link
Contributor Author

Update

Building on Windows... if macIcns is set, fs.copy's callback is called for copying the .icns but the field doesn't actually exist at dest. Seems to be because of jprichardson/node-fs-extra#98.

I'm working away on a fixing-1.0.x branch for now.

@adam-lynch
Copy link
Contributor Author

Meant to say, the problem with that fs.copy problem then is that we try and chmod a file which doesn't exist.

@adam-lynch
Copy link
Contributor Author

Oh and 4143a5e made no difference @bastimeyer :/.

@adam-lynch
Copy link
Contributor Author

Update

If the callback for fs.copy is called but the file doesn't exist, I retry the coping (up to 3 times). If it still fails, a handwritten error is bubbled up.

Also, even when the file does exist eventually after copying, fs.chmodSync can throw an ENOENT error, so I now use fs.chmod (async) and on error, I emit a log event, ignore it and move on. (The weird thing is that even trying to get the stats of the dest file when it's definitely there throws an ENOENT like that too).

It would be good if someone could figure out why an .icns file is different. I don't know why the fs.copy problem seemed only to happen with the .icns. I'm not sure if the chmod problem was isolated to just the .icns on Windows though, might've been more/all files.

These changes seem to help a lot. I think it builds fine now on Mac. But on Windows I'm still seeing an EMFILE error.

@adam-lynch
Copy link
Contributor Author

Update

Just as an experiment (because @4ver mentioned it), I tried enforcing that osx32 and osx64 are not done in parallel in handleMacApp but it makes no difference. My hacks to utils.copyFile are still needed.

@adam-lynch
Copy link
Contributor Author

Ok, this is still inconsistent :/. Sometimes I get the error that the copied file doesn't exist (after 3 re-tries), sometimes I don't. I still can't build my own app on Windows because of the EMFILE error, but a hello world app builds fine.

@adam-lynch
Copy link
Contributor Author

It's still not 100% right but I'm going to release 1.0.3 with the commits I've on the fixing-1.0.x branch and #143 because it would definitely be better than what we have out there now.

adam-lynch added a commit that referenced this issue Dec 21, 2014
@bastimeyer
Copy link
Contributor

@adam-lynch Is 1.0.4 working now? If so, could you please upgrade the grunt plugin, too? thanks

@adam-lynch
Copy link
Contributor Author

@bastimeyer sorry, was very busy and sick. I assume your npm cache didn't hold up that long and running npm install grunt-node-webkit-builder now would give you the latest node-webkit-builder as a dependency?

@adam-lynch
Copy link
Contributor Author

1.0.8 published. It helps but I don't think it's 100% fixed. I still get EMFILE errors personally.

@adam-lynch
Copy link
Contributor Author

Closing. I'll reopen if this happens for anyone still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Priority: 1
Projects
None yet
Development

No branches or pull requests

2 participants