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

Rename, C API, minimum iOS target, and j make arg #18

Merged

Conversation

ian-mcdowell
Copy link

  • Renamed libnode.framework to Node.framework
    Apple framework naming conventions declare that framework names should start with a capital letter, and the lib prefix is reserved for static/dynamic libraries.

  • Moved the node::Start method to a node_start C method, allowing Swift compatibility
    Swift does not support C++ interop, and importing the libnode framework into an iOS Swift project causes a build error. To fix this, I've wrapped the node::Start method into an equivalent C method, and exposed that in a C header.

  • Set minimum iOS target to iOS 9.0
    There is no reason to only target the latest version of iOS, as this library does not use new APIs introduced in iOS 11.
    I arbitrarily set the target to iOS 9, which is a typical minimum version that you'll find in apps that are backwards compatible.

  • Add -j8 to make arguments for iOS target
    Adding the j flag to make in the ios_framework_prepare script speeds up builds immensely on multi-core machines, by building in parallel, with a maximum of 8 concurrent build runners.

Checklist

[x] Built iOS target using ./tools/ios_framework_prepare.sh
[x] Tested Node.js framework in a Swift application

- Renamed libnode.framework to Node.framework
Apple framework naming conventions declare that framework names should start with a capital letter, and the `lib` prefix is reserved for static/dynamic libraries.

- Moved the node::Start method to a node_start C method, allowing Swift compatibility
Swift does not support C++ interop, and importing the libnode framework into an iOS Swift project causes a build error. To fix this, I've wrapped the node::Start method into an equivalent C method, and exposed that in a C header.

- Set minimum iOS target to iOS 9.0
There is no reason to only target the latest version of iOS, as this library does not use new APIs introduced in iOS 11.
I arbitrarily set the target to iOS 9, which is a typical minimum version that you'll find in apps that are backwards compatible.

- Add -j8 to make arguments for iOS target
Adding the `j` flag to make in the ios_framework_prepare script speeds up builds immensely on multi-core machines, by building in parallel, with a maximum of 8 concurrent build runners.
@jaimecbernardo
Copy link
Member

Thanks for the PR @IMcD23 .
These are valuable contributions.

Regarding iOS 9, it should be noticed that the project only builds binaries for 64 bits, so Applications won't be able to use it if they're 32 bits.

I'll take some more time to do some tests but I'd like to suggest some changes in the meanwhile:

  • Changing the framework's name to NodeMobile.framework and the header file to NodeMobile.h. This is to avoid confusion with Node's node.h.
  • Replacing make -j8 for make -j $(getconf _NPROCESSORS_ONLN) . That will make the number of threads adapt to the caller's CPU.

Copy link
Member

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @IMcD23. Thanks for your contribution.
This PR can be merged after the requested changes:

  • Changing the framework's name to NodeMobile.framework and the header file to NodeMobile.h. This is to avoid confusion with Node's node.h.
  • Replacing make -j8 for make -j $(getconf _NPROCESSORS_ONLN) . That will make the number of threads adapt to the caller's CPU.

@jaimecbernardo
Copy link
Member

Hi, @IMcD23, sorry for the delay.
Your changes look good and will be merged after some tests.
Thank a lot for your contribution.

@jaimecbernardo jaimecbernardo merged commit ec36bb6 into JaneaSystems:mobile-master Dec 15, 2017
@jaimecbernardo
Copy link
Member

Landed in ec36bb6
Thank you for your contribution.

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

Successfully merging this pull request may close these issues.

2 participants