Build: Add OSX build #116
Build: Add OSX build #116
Conversation
gabrielschulhof
left a comment
gabrielschulhof
left a comment
There was a problem hiding this comment.
@Wind-rider looks good overall (aside from the few comments) and I assume this makes it build for you on your version of OS X.
Do you think you could also try to add osx_image: xcode8.2 below all instances of os: osx in file .travis.yml? The Travis documentation claims that that'll result in the OSX test suite being run on OS X 10.12.
| var os = require( "os" ); | ||
|
|
||
| function run( command, arguments, options ) { | ||
| function run( command, args, options ) { |
There was a problem hiding this comment.
Please use full words for variable names. arguments was fine.
There was a problem hiding this comment.
eslinter or jshint warned that arguments is a reserved word, that's why I replaced it. Do you have any suggestions?
There was a problem hiding this comment.
Weird! I never got any complaints, but, of course, you're right. Let's keep args.
build-scripts/build-csdk.js
Outdated
| "x64": "x86_64", | ||
| "arm": "arm" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Please reduce the indentation such that the brace aligns with the double quote before the word "linux" above.
There was a problem hiding this comment.
Done, I'll commit it after getting feedback on the other parts.
| localArch = userAgentParts[ 3 ]; | ||
|
|
||
| targetArch = | ||
| archMap[ platform ] ? archMap[ platform ][ localArch ] : null; |
There was a problem hiding this comment.
Declaring variable localArch is superfluous if you're only going to use it once. Just put userAgentParts[ 3 ] directly into the brackets and remove the variable localArch entirely.
There was a problem hiding this comment.
I did that on purpose for code readability, so that everybody understands what is happening there. Are you very strongly in favor of using archMap[ platform ][ serAgentParts[ 3 ] ]? It doesn't have my favor.
There was a problem hiding this comment.
Alright, I guess. Let's keep it.
|
|
||
| var binariesSource, installWinHeaders, architecture, tinycborPath; | ||
| var binariesSource, installWinHeaders, tinycborPath, | ||
| userAgentParts, platform, localArch, targetArch, sysVersion; |
There was a problem hiding this comment.
Please rename variable targetArch to architecture, as it was.
There was a problem hiding this comment.
I found targetArch(itecture) clearer since it matches the parameter for Scons. Are you're sure about this?
|
@gabrielschulhof I modified |
| var os = require( "os" ); | ||
|
|
||
| function run( command, arguments, options ) { | ||
| function run( command, args, options ) { |
There was a problem hiding this comment.
Weird! I never got any complaints, but, of course, you're right. Let's keep args.
|
|
||
| var binariesSource, installWinHeaders, architecture, tinycborPath; | ||
| var binariesSource, installWinHeaders, tinycborPath, | ||
| userAgentParts, platform, localArch, targetArch, sysVersion; |
| localArch = userAgentParts[ 3 ]; | ||
|
|
||
| targetArch = | ||
| archMap[ platform ] ? archMap[ platform ][ localArch ] : null; |
There was a problem hiding this comment.
Alright, I guess. Let's keep it.
|
Looks like OS X 10.12 with XCode 8.2 works. Nice! |
|
@gabrielschulhof Thank you for your quick replies! I fixed the indentation. |
3 participants
This adds build options for OSX; I had to find a way to fill SYS_VERSION from node.js.