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

Use prebuilt toolchain to compile native modules #21

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

gmaclennan
Copy link

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?)

android/build.gradle Outdated Show resolved Hide resolved
@gmaclennan
Copy link
Author

@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?
Related to this PR I was also testing using the llvm-ar instead of the binutils ar since binutils is deprecated and is removed from NDK r23. I confess that I know very little about C++ compilation and am only guessing at things.

@staltz
Copy link
Contributor

staltz commented Oct 1, 2021

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.

@gmaclennan
Copy link
Author

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.

@jaimecbernardo
Copy link
Member

@gmaclennan @staltz

Thank you for the interest.
Unfortunately, we don't expect to be able to support and maintain this project for the foreseeable future.
Trying to transform the project in a way that the community can maintain is something that we're willing to look into.
Is this something that you'd be interested in doing?

@staltz
Copy link
Contributor

staltz commented Oct 5, 2021

@jaimecbernardo Yes! :)

@jaimecbernardo
Copy link
Member

@staltz @gmaclennan ,
One way of doing that I see that this could be done would be forking the repos into a different org ( https://github.com/nodejs-mobile ) and having community members be able to admin that org.
We'd link to the new repos from these as well.
Does this sound like an acceptable way forward for you?

@gmaclennan
Copy link
Author

@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.

@staltz
Copy link
Contributor

staltz commented Oct 6, 2021

Yes, the new org makes a lot of sense since JaneaSystems has many other repos.

@jaimecbernardo
Copy link
Member

@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.
Please let us know if you need anything else.

We'll add a mention to the repos in janeasystems to link to the new org.

@jaimecbernardo
Copy link
Member

And thank you for picking this up :)
It's a good prospect to have this be a community driven project, since it's mostly used by other open source projects!

@staltz
Copy link
Contributor

staltz commented Oct 21, 2021

Hey @jaimecbernardo, thank you! I trust @gmaclennan (worked with/for him before) so I'll add him as admin too.

Please let us know if you need anything else.

I would need publish access to the npm packages, too, my username on npm is also staltz.

And it seems nodejs-mobile-gyp also has to be moved to the new org. Maybe nodejs-mobile-module-compat too.

@staltz
Copy link
Contributor

staltz commented Oct 21, 2021

Update: it also seems that I need to be made an Owner of the org, otherwise I can't add others, see screenshot

Screenshot from 2021-10-21 11-39-22

@jaimecbernardo
Copy link
Member

Hey @jaimecbernardo, thank you! I trust @gmaclennan (worked with/for him before) so I'll add him as admin too.

Please let us know if you need anything else.

I would need publish access to the npm packages, too, my username on npm is also staltz.

And it seems nodejs-mobile-gyp also has to be moved to the new org. Maybe nodejs-mobile-module-compat too.

Thank you!
I've added the repos and gmaclennan to the org.
Working on getting access details figured out.

@jaimecbernardo
Copy link
Member

Update: it also seems that I need to be made an Owner of the org, otherwise I can't add others, see screenshot

You're an owner now.
Figuring out that npm access still, now.

@staltz
Copy link
Contributor

staltz commented Oct 21, 2021

Thanks!

Figuring out that npm access still, now.

NPM also has orgs, that may be useful, but not necessary.

@gmaclennan
Copy link
Author

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.

@staltz
Copy link
Contributor

staltz commented Oct 21, 2021

there are no issues because everything is a fork.

Perhaps one solution to this is:

  1. Delete the recently made forks on nodejs-mobile org
  2. Transfer the repos from JaneaSystems org to nodejs-mobile org
  3. (If you want:) On JaneaSystems org, fork the "main" repo on nodejs-mobile org and keep at it its previous state for archival purposes

@jaimecbernardo
Copy link
Member

@staltz @gmaclennan ,

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.
My idea here will be to close every issue on the original repos (most of them are really outdated and asking for support, to be honest) and direct the users to the new repos.
How does this sound?

@jaimecbernardo
Copy link
Member

I would need publish access to the npm packages, too, my username on npm is also staltz.

@staltz , I think access has been given. Could you please verify?

@staltz
Copy link
Contributor

staltz commented Oct 26, 2021

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.
My idea here will be to close every issue on the original repos (most of them are really outdated and asking for support, to be honest) and direct the users to the new repos.
How does this sound?

Understandable, the solution you propose sounds fine by me!

@staltz , I think access has been given. Could you please verify?

Yes! Thank you. Verified for nodejs-mobile-react-native and nodejs-mobile-cordova.

However, not for nodejs-mobile-gyp, though. Have you done that one too? Sometimes npm takes some time to rerender, so it might not be your fault if you've done it already.

@staltz
Copy link
Contributor

staltz commented Oct 26, 2021

@gmaclennan Can you replicate this PR on https://github.com/nodejs-mobile/nodejs-mobile-react-native ? I'm going to make a release of nodejs-mobile-react-native that uses the latest nodejs-mobile 0.3.3 (quite important release to support Android 11 which now puts restrictions on os.networkInterfaces()).

@jaimecbernardo
Copy link
Member

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.
My idea here will be to close every issue on the original repos (most of them are really outdated and asking for support, to be honest) and direct the users to the new repos.
How does this sound?

Understandable, the solution you propose sounds fine by me!

@staltz , I think access has been given. Could you please verify?

Yes! Thank you. Verified for nodejs-mobile-react-native and nodejs-mobile-cordova.

However, not for nodejs-mobile-gyp, though. Have you done that one too? Sometimes npm takes some time to rerender, so it might not be your fault if you've done it already.

Our mistake.
We've added you to nodejs-mobile-gyp now.

@gmaclennan
Copy link
Author

nodejs-mobile/nodejs-mobile-react-native#1

Thanks all!

@jaimecbernardo
Copy link
Member

@gmaclennan Can you replicate this PR on https://github.com/nodejs-mobile/nodejs-mobile-react-native ? I'm going to make a release of nodejs-mobile-react-native that uses the latest nodejs-mobile 0.3.3 (quite important release to support Android 11 which now puts restrictions on os.networkInterfaces()).

Yes, I think there was a fix for that committed (?) : JaneaSystems/nodejs-mobile@73d5d37

@staltz
Copy link
Contributor

staltz commented Oct 26, 2021

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. 🎉

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.

3 participants