-
Notifications
You must be signed in to change notification settings - Fork 67
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
Use prebuilt toolchain to compile native modules #21
base: unstable
Are you sure you want to change the base?
Use prebuilt toolchain to compile native modules #21
Conversation
@jaimecbernardo looks like you have limited time for this module right now. What's the maintenance plan going forwards? Maybe key users like ourselves @digidem and @staltz could help? |
PR looks good but I don't have a lot of confidence with this file specifically, and I haven't tested it but I trust that @gmaclennan has. I also agree that we need active maintenance for nodejs-mobile, it's a critical component for https://manyver.se and for @digidem software as well. I would be willing to be a maintainer but ideally someone with C++ and compilers expertise should lead, although I can help with basic housekeeping and the easy PRs. |
I've tested in so far as Mapeo Mobile has been built with this change for a while now using [email protected]. We are compiling leveldown, and it is working. |
Thank you for the interest. |
@jaimecbernardo Yes! :) |
@staltz @gmaclennan , |
@jaimecbernardo yes that makes sense to me. I don't think I would have the time to manage a community around this, but I could review and merge pull requests, especially for keeping this working in new React Native releases. I don't know enough about the patches / modifications to Node in the nodejs-mobile repo, but have some idea of the nodejs-mobile-react-native code and how that works, on Android, at least. Having reviewed a few different options going forwards (Java/Swift, C++, Rust), re-using the same Node Javascript code across desktop and mobile makes the most sense for us, so we'll likely be sticking with nodejs-mobile for a while. |
Yes, the new org makes a lot of sense since JaneaSystems has many other repos. |
@staltz , Sorry for taking this long. We've added you to admin a team in https://github.com/nodejs-mobile that can admin those repos and should be able to create other repos as well. You should be able to add more people as needed and do any required operation on the repos. We'll add a mention to the repos in janeasystems to link to the new org. |
And thank you for picking this up :) |
Hey @jaimecbernardo, thank you! I trust @gmaclennan (worked with/for him before) so I'll add him as admin too.
I would need publish access to the npm packages, too, my username on npm is also And it seems nodejs-mobile-gyp also has to be moved to the new org. Maybe nodejs-mobile-module-compat too. |
Thank you! |
You're an owner now. |
Thanks!
NPM also has orgs, that may be useful, but not necessary. |
Thanks all! @jaimecbernardo how do you want to handle issues for nodejs-mobile going forwards? I see they are still in https://github.com/JaneaSystems/nodejs-mobile and in the @nodejs-mobile org there are no issues because everything is a fork. |
Perhaps one solution to this is:
|
For contractual reasons, we have to keep the original repos in our org for some time and that's why we can't transfer the repos at this moment. |
@staltz , I think access has been given. Could you please verify? |
Understandable, the solution you propose sounds fine by me!
Yes! Thank you. Verified for However, not for |
@gmaclennan Can you replicate this PR on https://github.com/nodejs-mobile/nodejs-mobile-react-native ? I'm going to make a release of |
Our mistake. |
nodejs-mobile/nodejs-mobile-react-native#1 Thanks all! |
Yes, I think there was a fix for that committed (?) : JaneaSystems/nodejs-mobile@73d5d37 |
Yes, precisely that. I was about to (attempt to) fix it myself and was glad it was already fixed. @gmaclennan You can close this PR now. 🎉 |
The Android build step was making a standalone toolchain for compiling native modules. Since NDK r19 standalone toolchains are obsolete since prebuilt toolchains are included in the NDK distribution.
This PR changes the build configuration to use the NDK prebuilt toolchains when compiling native modules. It requires using NDK r19 or newer (which I think is required for building this module now anyway?)