Fix openNoInt building error after android ndk r21 #1593
Conversation
|
Thanks for this PR @Kudo! Our team has a React Native app with some C++ code mixed in, and we use the Folly library. We've had to patch around this issue for a while, so will be great to have a long-term fix. It's something that a lot of other people seem to be experiencing too! Excited to see a Folly release with this and #1584 merged, so we can get rid of our custom patches. |
|
@yfeldblum Can you review this ? Thanks. |
| // The solution is referenced from | ||
| // https://github.com/llvm/llvm-project/commit/0a0e411204a2baa520fd73a8d69b664f98b428ba | ||
| // | ||
| auto Open = [&]() { return open(name, flags, mode); }; |
There was a problem hiding this comment.
Let's write this as:
auto openWrap = +[](const char* name_, int flags_, mode_t mode_) {
return open(name_, flags_, mode_);
}
auto openFunc = kIsAndroid ? openWrap : open;
return int(wrapNoInt(openFunc, name, flags, mode));
There was a problem hiding this comment.
I think it would be simpler to just use the lambda in all cases, and don't bother checking kIsAndroid. The use of the lambda doesn't appear to affect the code generated by either gcc or clang in optimized builds.
I think we can just say
auto openWrapper = [&] { return open(name, flags, mode); };
return int(wrapNoInt(openWrapper));
There was a problem hiding this comment.
thanks for the review.
should i revise changes in this pr, as imported to facebook internal phabricator already?
There was a problem hiding this comment.
Is the import blocked due to some reason ? I see it has been blocked for two days.
There was a problem hiding this comment.
This is merged internally. The syncing from the internal repo to github is not working at the moment, but once it is working again this change will be merged on github as well.
There was a problem hiding this comment.
When this synced to github. Can you draft a new release for this ? react-native has been stuck for a while.
There was a problem hiding this comment.
@simpkins Looks the import is stuck.
|
@simpkins has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: This PR bumps NDK_VERSION to 21.4.7075529, and patches FileUtil.cpp from folly based on patch from facebook/folly#1593. We can remove the patch once PR lands in Folly and bump Folly version in RN. FYI, NDK 20 is deprecated and 21 is LTS release. ## Changelog [Android] [Changed] - Bump NDK to 21.4.7075529 Pull Request resolved: #31731 Reviewed By: mdvacca Differential Revision: D29166690 Pulled By: ShikaSD fbshipit-source-id: 0792691404f718aaf5af1369f66f0cba046b4e20
Summary: Upgrade folly for the facebook/folly#1593 fix for NDK 21 failure ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Changed] - Upgrade folly to 2021.06.28.00 Pull Request resolved: #31802 Test Plan: `./gradlew :ReactAndroid:installArchives` `./gradlew packages:rn-tester:android:app:installJscRelease` `./gradlew packages:rn-tester:android:app:installHermesRelease` Reviewed By: RSNara Differential Revision: D29547027 Pulled By: ShikaSD fbshipit-source-id: a10c7c65f459091bd0e7cca750a9b9e067189b73
Android NDK bionic with FORTIFY will override original
open()definition and making follywrapNoInttemplate failed to deduct.The issue may happen only after NDK r21 because this commit landed after r21
References:
android/ndk#1328
llvm/llvm-project@0a0e411