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 for #139 #140

Merged
merged 1 commit into from
Dec 18, 2014
Merged

Conversation

bastimeyer
Copy link
Contributor

This fixes #139...
handleMacApp and handleWinApp haven't been upgraded after the platform name changes.

@bastimeyer
Copy link
Contributor Author

Please don't merge this yet, there is an issue regarding the package.json and utils.getFileList which needs to be fixed first...
I'm using a seperate package.json for node-webkit other than the package.json for the main project. This file lies in a subfolder, in addition to the one in the root folder. For some reason, the file inside the subfolder is being read from while building the application. This of course raises some errors because I don't have copyright informations set in this file (e.g. the osx-plist file is failing).

@adam-lynch
Copy link
Contributor

@bastimeyer brilliant that you're helping with this!

One thing, can you not introduce changes to whitespace? See our .editorconfig, indentation should be four spaces. EditorConfig is really handy, if you install the plugin for your editor it'll automatically pick up what the indentation should be :).

@bastimeyer
Copy link
Contributor Author

Where did I change the indentation style?

@adam-lynch
Copy link
Contributor

Ugh sorry 👍, I thought those four spaces were tabs because of how they look in the diff

@bastimeyer
Copy link
Contributor Author

Please ignore what I was talking about earlier. I wasn't aware, that this "package.json bug" was working as intended for the whole time.
The issue I had was caused by npm, which didn't resolve the dependencies of the grunt plugin properly, so I was using an old version. My PR from a couple of days ago (1447f08) already should have caused my own builds to fail, because of the way I am using a second package.json as I mentioned above.
The dependency issues are resolved now since the upgrade to 1.0.0 though... The sad thing for me is, that I released a bugged osx version of my application during this time span and didn't even notice 😞

Should I squeeze these commits or is this ok?

The handleWinApp and handleMacApp methods now both work again after
the recent platform changes.
@bastimeyer bastimeyer force-pushed the multi-win-osx-platforms-fix branch from 233afda to 9a9971c Compare December 15, 2014 22:52
@adam-lynch
Copy link
Contributor

Oh, lucky you :/.

So this is ready yeah?

@bastimeyer
Copy link
Contributor Author

Nevermind... squeezed it...

What I also wanted to ask: should the copyright informations be mandatory? Right now they are and this is what caused my earlier confusion (paired the the npm issue and the package.json stuff ofc)

@bastimeyer
Copy link
Contributor Author

Oh, didn't see your post.. It should be good now

@lucienimmink
Copy link

This does fix the winIco but breaks compiling osx on 1.0.0; paths can't be resolved correctly when using the macIcns option; omitting that option results in an error that the plist file can't be found :s

@bastimeyer
Copy link
Contributor Author

I can't reproduce any of these said errors. Yes, these methods are not covered by any test cases, but I tested it manually by using different configurations, and even though, all I did was adding the _forEachPlatforms call. So the build logic shouldn't be affected by these changes.
Did you pull the recent commit? As you can see, I have pushed multiple commits - my first one was a mistake and only fixed the windows builds when building one architecture at a time, so please don't use that...

@adam-lynch
Copy link
Contributor

@lucienimmink did you see the reply above?

@adam-lynch adam-lynch merged commit 62a47d9 into nwutils:master Dec 18, 2014
@adam-lynch
Copy link
Contributor

I assume it's fixed, we'll patch if not ASAP

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.

winIco doesn't work in version 1.0.0
3 participants