Several changes to help with adding 64bit support. #117
Conversation
| return 'win'; | ||
| return 'win32'; | ||
|
|
||
| case 'win64': |
There was a problem hiding this comment.
There is no win64 platform (see here)... This needs to be changed to
case 'win32':
return process.arch === 'x64' ? 'win64' : 'win32';
If you're running on Windows, they're probably just permissions tests failing, that's ok. We probably should add tests here though for 64-bit builds because the last PR was passing 100% successfully (on my Mac) & there were clearly a good few things missing. |
|
Nice PR! One thing: what do you guys think about this config:
Otherwise we would make a breaking change and all users have to change their config |
|
I agree, breaking changes are generally a bad thing and I like @steffenmllr's suggestion. To implement this, I added a little logic to do a quick check on the passed platforms if they are provided and if it finds 'osx' or 'win' they are removed from the list and replaced with 'osx32', 'osx64', or 'win32', 'win64' respectively. |
|
I think for this to be added, then the platform overrides feature needs to support it too. So that means that adam-lynch/platform-overrides needs to support |
|
Also worth noting is that if |
|
@adam-lynch Agree on Linux. That would be best practice. |
|
What's the status of this? I can't remember. |
|
I believe it is waiting on adam-lynch/platform-overrides#4 to be merged in and then afaik it's good to go. |
|
Ah shit ok, I'll give it one more proper test (with the actual app I'm working on) tomorrow and merge both then. |
|
Wait, had a quick look at the changes there again... isn't this missing support for passing |
|
Maybe it's just missing from the readme then? |
|
That is probably the case. I don't believe I updated the README at all. |
| case 'win32': | ||
| return 'win'; | ||
| return process.arch === 'x64' ? 'win64' : 'win32'; |
There was a problem hiding this comment.
@danthewolfe can you apply the same fix as adam-lynch/platform-overrides@d41555c#diff-66d14bf3d12add1ab55262539d9889e0R14 here? Just for Windows
|
What I think is left to be done on this:
@danthewolfe are you ok to do the first three? |
|
Just built a 64-bit Windows app 😀 |
|
Applied the three changes described above. |
|
like so? Should that change also be made to osx and linux? |
Yes.
No. |
|
👍 |
|
Would you mind squashing and rebasing these commits into one commit? I'll merge then and release 1.0.0 or maybe I'll merge #123 (as it is) and then release 1.0.0. |
|
Forgot to rebase. Hang on. |
|
Squashed and rebased. |
|
Shouldn't the tests be updated as well before merging these changes? Also (wrong thread, I know): |
Some are but yeah, we missed those. @danthewolfe, I guess that should be: t.deepEqual(x._zips.osx32, x._zips.osx64, 'OSX 32-bit ZIP should be equal to the OSX 64-bit one');
t.deepEqual(x._zips.osx64, x._zips.win32, 'OSX 64-bit ZIP should be equal to the Windows 32-bit one');
t.deepEqual(x._zips.win32, x._zips.win64, 'Windows 32-bit ZIP should be equal to the Windows 64-bit one');
t.deepEqual(x._zips.win64, x._zips.linux32, 'Windows 64-bit ZIP should be equal to the Linux 32-bit one');
t.deepEqual(x._zips.linux32, x._zips.linux64, 'Linux 32-bit ZIP should be equal to the Linux 64-bit one');
Oh, yes please if you can. |
* Change osx to osx32 * Change win to win32 * Change defaults from ['osx', 'win'] to ['osx32', 'osx64', 'win32', 'win64'] * Tried to hit some checks I missed. * Change osx.json to osx32.json and win.json to win32.json * Got failing tests down to 5.... * Re-adding support for 'osx' and 'win' to avoid breaking changes * Check for 'osx' and 'win' in the platforms list and replace them with 'osx32', 'osx64', and 'win32', 'win64' respectively. * Fix tests. * Add linux shorthand to compliment win and osx shorthand. * Add fix for detecting Windows 64-bit. * Update platform-overrides dependency to ~1.0.0 * Add win, osx, linux and description of what they do to readme. Add additional tests.
|
@danthewolfe ah just saw you updated the tests or else @bastimeyer was looking at the wrong branch or something. Merging. |
Several changes to help with adding 64bit support.
|
Thanks again @danthewolfe and @bastimeyer. I'd appreciate if you could test master ( |
|
Don't forget to update the grunt repo as well |
|
@bastimeyer ugh yeah, I was going to get to that. Do you know what needs to be changed? I'm not familiar with it at all. Hopefully it just needs to be changed to: // maintain backward compatibility by supporting old platform style
switch(opt){
case 'win':
if(!!options[opt]) {
addPlatform(nwOptions, 'win32');
addPlatform(nwOptions, 'win64');
}
break;
case 'win32':
case 'win64':
if(!!options[opt]) {
addPlatform(nwOptions, opt);
}
break;
case 'osx':
if(!!options[opt]) {
addPlatform(nwOptions, 'osx32');
addPlatform(nwOptions, 'osx64');
}
break;
case 'osx32':
case 'osx64':
if(!!options[opt]) {
addPlatform(nwOptions, opt);
}
break;
case 'linux':
if(!!options[opt]) {
addPlatform(nwOptions, 'linux32');
addPlatform(nwOptions, 'linux64');
}
break;
case 'linux32':
case 'linux64':
if(!!options[opt]) {
addPlatform(nwOptions, opt);
}
break;
case 'mac':
if(!!options[opt]) {
addPlatform(nwOptions, 'osx32');
addPlatform(nwOptions, 'osx64');
}
break; |
|
The old config format was { win: true, mac: true } which is now { platforms: ['win','osx'] } case 'win':
case 'win32':
case 'win64':
case 'osx':
case 'osx32':
case 'osx64':
case 'linux':
case 'linux32':
case 'linux64':
if(!!options[opt]) {
addPlatform(nwOptions, opt);
}
break;
case 'mac':
if(!!options[opt]) {
addPlatform(nwOptions, 'osx');
}
break;
case 'mac32':
if(!!options[opt]) {
addPlatform(nwOptions, 'osx32');
}
break;
case 'mac64':
if(!!options[opt]) {
addPlatform(nwOptions, 'osx64');
}
break; |
|
K I'll add this in awhile and see if it's ok. |
|
Ok, done. |
|
Thanks! |
I'm not really familiar with node unit testing, so I'm unsure how to
handle the rest of these failures.