-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix: build xcframeworks to fix linking issue in Playground App #654
Conversation
fd3ce3e
to
662c1e4
Compare
Hey, I've opened a PR to RNTA to add support for this feature: microsoft/react-native-test-app#2204 |
I'm working on fixing the build so we can continue testing this PR. #657 |
@okwasniewski I just merged the build fix PR. You should be able to update your PR and kick a new build. I'll check its progress |
04d33dd
to
944b1ab
Compare
I'm still encountering some issues with react-native-permissions when launching the app. Im going to go back to this tomorrow, so let's wait with merging 🙏 (At least the app is now properly built) |
Okay, so I got this to work on the simulator, there was an issue with The only thing left is that the Playground is still not building properly for real device (same issue as before). Going to focus on this one now. |
ed445f2
to
f130f89
Compare
Hey @CedricGuillemet @ryantrem I've made it work with xcframeworks. So here #658, I've mentioned building a single .xcframework to fix the issue, but unfortunately, this wasn't a viable option. I wanted to merge all of the static libs that Babylon currently builds into one and then build an xcframework for each platform and architecture, but there were bunch of linking issues resulting from the merge. So I decided to create multiple XCFrameworks for all of our static libraries. I've integrated it into gulpfile.js and run it on postinstall of RNTA. (This can be further expanded to build on the CI, but for the sake of keeping things simple in the first iteration I created a local script). So now when you run So currently, we build for This can be easily expanded for other platforms like macOS and visionOS later. It would be great if you could test out the script locally and let me know what you think. "Build from source" stays as an option but only for CMake < 3.24 (as stated in README) |
Also to address any performance concerns, dynamic linking of xcframeworks add some overhead to the app startup, but for frameworks that only ship static There is an additional build script being executed "[CP] Copy XCFrameworks" that copies proper static libraries for current build and only they end up in app binary |
fde80a7
to
45bb1d1
Compare
Hey @ryantrem @CedricGuillemet @bghgary I wanted to check in and ask if you are okay with this approach? The build is still failing but I want to ensure I'm on the right track before I spend time fixing it. I think that we need to build the XCFrameworks only when BabylonNative version is updated. This should significantly speed up the CI builds |
This looks fine to me. So, building the xcrframework means doing the same thing as |
We can call When it comes to building locally, you can run the Regarding publishing @babylonjs/react-native package, we can ship xcframeworks as is to NPM (same as However, I didn't plan to change the publish scripts in this PR as my goal is to get RNTA working, and publishing can be addressed in upcoming PRs |
54d046e
to
feea1c5
Compare
Hey, @ryantrem @CedricGuillemet I've stripped almost everything from the PR except build and linking of XCFrameworks. Let me provide a bit more detail of what is actually happening in
const buildCommand = `xcodebuild -sdk ${platform} -arch ${arch} -configuration Release -project ReactNativeBabylon.xcodeproj -scheme BabylonNative build CODE_SIGNING_ALLOWED=NO BUILD_LIBRARY_FOR_DISTRIBUTION=YES`;
|
Modules/@babylonjs/react-native-iosandroid/react-native-babylon.podspec
Outdated
Show resolved
Hide resolved
feea1c5
to
78ff316
Compare
78ff316
to
e44b245
Compare
Hey @ryantrem @CedricGuillemet @bghgary I've applied the changes we talked about yesterday. Let me know if I should address something else |
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.
Looks good to me!
f2b5e9e
to
e44b245
Compare
Describe the change
This PR fixes RNTA linking issues for newer versions of CMake, this PR is still a draft as I needed to patch-package the react-native-test-app. I need to make sure this is the correct solution to solve this issue and contribute the fix back to RNTA.
Screenshots
If applicable, add screenshots to help explain your change. Consider using tools like Giphy Capture or ScreenToGif to capture animated gifs. If you are using physical devices rather than emulators/simulators, consider using tools like Vysor or scrcpy to project your device's display to your dev machine to allow for screen capture.
Documentation
Have you updated the documentation to reflect your changes? (Yes or N/A)
Testing
Have you tested the final iteration of your changes? (Yes or N/A or I need help testing on [Android|iOS|Windows])