Convert react-native-quick-crypto to Cxx turbo module. #327
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
|
|
||
| RNTQuickCryptoCxx::RNTQuickCryptoCxx(std::shared_ptr<CallInvoker> jsInvoker): NativeQuickCryptoCxxCxxSpec<RNTQuickCryptoCxx>(std::move(jsInvoker)) { | ||
|
|
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
|
|
||
|
|
||
| RNTQuickCryptoCxx::RNTQuickCryptoCxx(std::shared_ptr<CallInvoker> jsInvoker): NativeQuickCryptoCxxCxxSpec<RNTQuickCryptoCxx>(std::move(jsInvoker)) { | ||
|
|
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Redundant blank line at the start of a code block should be deleted. [whitespace/blank_line] [2]
|
|
||
|
|
||
| RNTQuickCryptoCxx::RNTQuickCryptoCxx(std::shared_ptr<CallInvoker> jsInvoker): NativeQuickCryptoCxxCxxSpec<RNTQuickCryptoCxx>(std::move(jsInvoker)) { | ||
|
|
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Redundant blank line at the end of a code block should be deleted. [whitespace/blank_line] [3]
| } | ||
|
|
||
| double RNTQuickCryptoCxx::install(jsi::Runtime &rt) { | ||
|
|
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
| } | ||
|
|
||
| double RNTQuickCryptoCxx::install(jsi::Runtime &rt) { | ||
|
|
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Redundant blank line at the start of a code block should be deleted. [whitespace/blank_line] [2]
| auto hostObject = std::static_pointer_cast<jsi::HostObject>( | ||
| std::make_shared<::margelo::MGLQuickCryptoHostObject>(this->jsInvoker_, workerQueue)); | ||
| auto object = jsi::Object::createFromHostObject(rt, hostObject); | ||
| rt.global().setProperty(rt, "__QuickCryptoProxy", std::move(object)); |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Add #include for move [build/include_what_you_use] [4]
|
|
||
| #pragma mark - implementation | ||
| class RNTQuickCryptoCxx : public NativeQuickCryptoCxxCxxSpec<RNTQuickCryptoCxx> { | ||
| public: |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
public: should be indented +1 space inside class RNTQuickCryptoCxx [whitespace/indent] [3]
| #pragma mark - implementation | ||
| class RNTQuickCryptoCxx : public NativeQuickCryptoCxxCxxSpec<RNTQuickCryptoCxx> { | ||
| public: | ||
| RNTQuickCryptoCxx(std::shared_ptr<CallInvoker> jsInvoker); |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
| public: | ||
| RNTQuickCryptoCxx(std::shared_ptr<CallInvoker> jsInvoker); | ||
| virtual ~RNTQuickCryptoCxx() = default; | ||
|
|
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
|
|
||
| double install(jsi::Runtime &rt); | ||
| }; | ||
| } |
There was a problem hiding this comment.
[cpplint] reported by reviewdog 🐶
Namespace should be terminated with "// namespace margelo" [readability/namespace] [5]
| "${NODE_MODULES_DIR}/react-native/ReactCommon/jsi" | ||
| "${NODE_MODULES_DIR}/react-native/ReactCommon/turbomodule/core" | ||
| "${NODE_MODULES_DIR}/react-native/ReactCommon/react/nativemodule/core" | ||
| "./build/generated/source/codegen/jni/react/renderer/components/RTNQuickCryptoCxxSpec" |
There was a problem hiding this comment.
that's really not a good idea...
There was a problem hiding this comment.
I think this is something we need to talk to Meta about to hear their opinions.
We depend on OpenSSL and currently install it through Gradle (add the dep to build.gradle), but since a CxxTurboModule doesn't have a build.gradle anymore (it's built through the CodeGen'd CMakeList) we don't have the dependency anymore.
So now we add a build.gradle that builds our CxxTurboModule with externalNativeBuild, adds the OpenSSL prefab, and then additionally we have a dummy CMakeLists just for autolinking... I don't think this is a good idea, and we all agree that this is a temporary hack that we should get rid off asap.
Maybe two questions for different people;
- @Szymon20000 can we add OpenSSL somehow differently? Then we don't need that dummy CMakeList
- @philIip / @cortinico do you think that this is the future for all CxxTurboModules? No build.gradle, just only a CMakeLists?
There was a problem hiding this comment.
but since a CxxTurboModule doesn't have a build.gradle anymore (it's built through the CodeGen'd CMakeList) we don't have the dependency anymore.
You should be able to configure the react-native.config.js to use your own CMakeLists.txt file + include external dependencies and access them via Prefab.
Using the codegen-ed one is the 'easiest' approach as it saves you from configuring extra build logic.
There was a problem hiding this comment.
How would we add the codegen'd specs then?
Do we add that generated path in /build/... to our CMake and assume codegen always generated it into that path?
There was a problem hiding this comment.
@cortinico That's exactly what I tried. I passed my own cmake that relied on codegened Cxx spec and prefab openssl. But because that cmake was build together outside of my build process the prefab wasn't available there.
How we can make prefabs available to autolink_cmake if they are only listed as dependencies in library build.gradle?
There was a problem hiding this comment.
But because that cmake was build together outside of my build process the prefab wasn't available there.
that cmake <- Which CMake file are you referring to? Yours or the one generated by Codegen? Yours should have access to the prefab declared in build.gradle. The codegen one should not.
There was a problem hiding this comment.
Hey!
Thanks for finding time.
So there are 3 cmakes (in the new approach where we don't build ourselves in build.gradle) that I'm aware of:
- codegen generated with specs/nodes/props
- library cmake (it relies on openSSL prefab)
- autolinking cmake generated by react Gradle plugin
If I understand correctly in autolinking cmake the plugin includes first 2 cmakes. But the whole build process happens now per application (not per library). After adding openSSL as a prefab in library's build Gradle I got an error that cmake is not able to find openssl prefab.
I can try it again and post the error that I got here.
There was a problem hiding this comment.
autolinking cmake generated by react Gradle plugin
There is not entirely correct. The Gradle plugin generates some .cmake files that contains variables used by the App's CMake file (to know which libraries to link against).
Are we able to create a PR against:
https://github.com/cortinico/reproducer-cxx-tm-autolinking
to show me your setup with the link against openssl and I can look into it?
#Hack alert
I tried to make use of auto linking but turned out that our cmake included in generated autolink cmake was not able to find our deps. most likely because those are our dependencies added in our build Gradle. So For now I just added dummy cmake that is being included and it doesn't contain any symbols that we have in our final so.