search

nodejs-mobile / nodejs-mobile-react-native

Node.js for Mobile Apps React Native plugin
https://nodejs-mobile.github.io
MIT License
174 stars 42 forks source link

ABI detection doesn't work properly with more recent versions of Gradle #11

Closed achou11 closed 1 year ago

achou11 commented 2 years ago

The null check here needs to be updated to something that also checks for an empty collection (or list or whatever Groovy calls it), as the check to calculate nativeModulesABIs returns [] in more recent versions of Gradle.

https://github.com/nodejs-mobile/nodejs-mobile-react-native/blob/86863719faed3bcd8b9a48807bac71f2afed1813/android/build.gradle#L239

Proposed fix: check if nativeModulesABIs is falsy value or add an explicit [] check (e.g. here)

achou11 commented 1 year ago

Closing since this was fixed in 2fa781d836aa794c6b3809db58bdf0f417859f01

achou11 commented 1 year ago

Looks like fix applied in 2fa781d was reverted by https://github.com/nodejs-mobile/nodejs-mobile-react-native/commit/66f83b86ff84e92e887d9a9ad2dee781544aae21#diff-197b190e4a3512994d2cebed8aff5479ff88e136b8cc7a4b148ec9c3945bd65aL241. Haven't tested but I'd imagine that this issue would reappear as a result. Was that revert intentional?

staltz commented 1 year ago

Hey Andrew, I think that change was in order to support Mac M1 with a recent Gradle version and recent RN version (not sure which of these 3 was essential but I would guess Gradle version). I didn't realize it was reverting a change you made before.

Can you tell me versions of tools you use where the previous if in gradle was working well?

Would be good to know exact versions (or range versions) of gradle where one approach works and the other doesn't.

achou11 commented 1 year ago

@staltz here's my best attempt at a breakdown:

My suspicion is that the last one is the culprit for the issue. On our end, I ran into the original issue after I upgraded these tools in https://github.com/digidem/mapeo-mobile/pull/867, which includes going from com.android.tools.build:gradle:3.5.4 to com.android.tools.build:gradle:4.2.2. There's no publicly available API reference for v3 but for v4, [v7](https://developer.android.com/reference/tools/gradle-api/7.4/com/android/build/api/dsl/Ndk#abifilters()), and [v8 (current)](https://developer.android.com/reference/tools/gradle-api/8.0/com/android/build/api/dsl/Ndk#abiFilters()), the android.defaultConfig.ndk.abiFilters seems to have a return signature of MutableSet<string> (notice no ?), suggesting that it will at least return [] and not null. If I had to guess, maybe v3 had a signature like MutableSet<string>??

My original assessment of this issue is probably inaccurate after looking into this some more - I had originally blamed a more recent Gradle version, rather than the more recent Android Gradle Plugin version.

achou11 commented 1 year ago

just saw https://github.com/nodejs-mobile/nodejs-mobile-react-native/commit/f1ed0a95563303482d91d4e10e15d32d65c8eec5 and I'm wondering if the check in question can be removed entirely? Don't have the fullest understanding of all things Gradle, but that commit seems like it would prevent that abifilters code path from being run