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
Updates to fix android builds failing in Expo #676
Conversation
applied patch to CMakeList.txt provided by Marc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haibert thank you for the PR bro! 🙏
While that makes it build for Expo (RN 0.64?), it has some problems in current RN versions. We need to make it conditional, fall back to compiling jsi.cpp if RN version is older. Then it works for both.
@@ -22,6 +22,7 @@ add_library( | |||
src/main/cpp/java-bindings/JFrameProcessorPlugin.cpp | |||
src/main/cpp/java-bindings/JImageProxy.cpp | |||
src/main/cpp/java-bindings/JHashMap.cpp | |||
${NODE_MODULES_DIR}/react-native/ReactCommon/jsi/jsi/jsi.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be avoided if possible. Does it really not build if that line is removed? If not, then please make this conditional (if
+ set
), so it is not compiled in RN versions that support the JSI_LIB
library below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aka: On RN 0.64 and below, compile jsi.cpp
here. On RN 0.65 and above, do NOT compile jsi.cpp
here, but find the JSI_LIB
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason for that is, that on RN 0.65 and above, jsi.cpp
is compiled multiple times - aka we have duplicate symbols. This is a problem in C++, since JSI now exists more than once. My jsi::JSError
for example is not the same as jsi::JSError
in React Native, therefore the app hard-crashes. So we need to make that conditional. Check out Reanimated codebase on how they do it
find_library( | ||
JSI_LIB | ||
jsi | ||
PATHS ${LIBRN_DIR} | ||
NO_CMAKE_FIND_ROOT_PATH | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional
@@ -125,7 +120,6 @@ message(WARNING "VisionCamera linking: FOR_HERMES=${FOR_HERMES}") | |||
target_link_libraries( | |||
${PACKAGE_NAME} | |||
${LOG_LIB} | |||
${JSI_LIB} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional
And afaik this will break Hermes, right? |
Why would it break Hermes? |
@mrousavy that was the latest info I got from @haibert regarding this comment. Haven't verified it yet |
@mrousavy will test this in 30 minutes and report back |
@mrousavy I need to dig around this issue first, looks like monorepos aren't still fully supported
|
@mrousavy it is not working unfortunately. (installed 2.11.0)
I truncated the output, since its just complaining about the kotlin versions. |
@mrousavy alright, after some digging, I found that I had a config-plugin, which inserted following:
I had to patch the value from 1.4.10 to 1.5.10 (because 1.6.0 would break expo-dev-launcher) and it looked like that everything will finally compile, but ended up with this error in the last 1% :D
|
Okay thanks for the detailed feedback @hirbod!
|
You can also fix that duplicate lib error yourself in your build.gradle btw :) |
@mrousavy I agree with everything you said. We should just state a little info for the expo users, that they might need a config-plugin which bumps 30 to 31 and add some docs for it. Regarding pickFirst etc: not exactly sure where to do that + it has to be patched in build.gradle, which would need a config-plugin again. Because that |
@mrousavy Hi Marc! You are the man!
|
@haibert are using a monorepo like me? I had to patch-package like 6 paths to get rid of this. (node_modules/rnvc/android/build.gradle) But the first error also occured on the first install, because the gradle just got updated after the 2nd build |
where's your node_modules? |
its in the root of my project directory |
@haibert just try |
@hirbod Hi Hirbod, I am using eas build, I run the following command "eas build --profile development --platform android" |
@haibert ok, I see. Well, I will end up there eventually, when my dev process is done and I need to create a release. @mrousavy I monkey patched my android/app/build.gradle and added every duplicate that was being reported like so:
My app finally was building and the development client booted, but ended up like so: |
@mrousavy alright, while I do not consider this a solution, FOR NOW, my app is building (but still crashing due to reanimated and dev tools). I had to patch it like so:
@haibert no, I never used "eas" cloud stuff so far. EDIT: IT IS FINALLY WORKING. As said, with the patch, but I have a running android app, cant believe it! |
@hirbod seems like you figured it out already.
The So in conclusion, what needs to be done is:
|
Hi Marc, thanks for the clarification. So I would say we need to provide / update the config plug-in and add the above to /android/app/build.gradle This could be a workaround. Shall we open an issue upstream or is this something already known? Edit: |
@mrousavy Hi Marc, So 2.11.1 got rid of the monorepo issue. I had to use a plugin to set the compileSdkVersion to 31, but I got the build error bellow
|
I've added two new options for android (disable frame proceccsors, if you don't need them + fixing the sharedlibrary issue that you also encountered). Thanks for your report, this helped me to change the glob pattern. |
@hirbod awesome! Thank you I will try this out! |
@haibert my PR will now be able to disable frame processors also for iOS. |
This PR is obsolete. Since my dangerouslyHandleAndroidSharedLibrary prop was removed, people will most likely encounter issues with expo-dev-client. So it is necessary to use the minSdkVersion plugin (bump to 31) and the pickFirst config plugin. (see below) const {
withAppBuildGradle
} = require('@expo/config-plugins')
const addPickFirst = (buildGradle, paths) => {
const regexpPackagingOptions = /\bpackagingOptions\s*{[^}]*}/
const packagingOptionsMatch = buildGradle.match(regexpPackagingOptions)
const bodyLines = []
paths.forEach((path) => bodyLines.push(`pickFirst '${path}'`))
const body = bodyLines.join('\n ')
if (packagingOptionsMatch) {
console.warn(
'WARN: withPickFirst: Replacing packagingOptions in app build.gradle'
)
return buildGradle.replace(
regexpPackagingOptions,
`packagingOptions {
${body}
}
`
)
}
const regexpAndroid = /\nandroid\s*{/
const androidMatch = buildGradle.match(regexpAndroid)
if (androidMatch) {
return buildGradle.replace(
regexpAndroid,
`
android {
packagingOptions {
${body}
}
`
)
}
throw new Error('withPickFirst: Could not find where to add packagingOptions')
}
module.exports = (config, paths) => {
if (!paths) {
throw new Error('withPickFirst: No paths specified!')
}
return withAppBuildGradle(config, (config) => {
if (config.modResults.language === 'groovy') {
config.modResults.contents = addPickFirst(
config.modResults.contents,
paths
)
} else {
throw new Error(
"withPickFirst: Can't add pickFirst(s) because app build.grandle is not groovy"
)
}
return config
})
} and install it like this ['./plugins/withPickFirst', ['lib/x86/libc++_shared.so', 'lib/x86_64/libc++_shared.so', 'lib/armeabi-v7a/libc++_shared.so', 'lib/arm64-v8a/libc++_shared.so']], |
Thanks for the explanation @hirbod! |
What
This PR applies all the changes from the following comment which did fix the android build issue when using expo custom-dev-client.
Changes
Even with these changes applied the user will need to use the following config plugin (Courtesy of AdamMercy) to get a successful build.
Then inside of app.json
//---------------IN APP.JSON-----------------
//---------------IN APP.JSON-----------------
Tested on
Related issues