Skip to content
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: fixed binary for react native #288

Merged

Conversation

wadeking98
Copy link
Contributor

the async_trait macro appeared to be crashing Bifold in react native. I've refactored the code to remove the async_trait macro and made some changes to the code to compensate

@wadeking98
Copy link
Contributor Author

@genaris @TimoGlastra @andrewwhitehead could I get a quick review on this please?

@berendsliedrecht
Copy link
Contributor

the async_trait macro appeared to be crashing Bifold in react native. I've refactored the code to remove the async_trait macro and made some changes to the code to compensate

Do you have some of the logs for this? We have used async_trait since the beginning and it didn't really causes any crashes then. Curious to see what broke it.

@wadeking98
Copy link
Contributor Author

I'd get this error message on the build with async trait after fetching a schema definition from the ledger:
Fatal signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x76d39d6fa0 in tid 15686 (mqt_js), pid 15558 (bc.gov.BCWallet)

@berendsliedrecht
Copy link
Contributor

I'd get this error message on the build with async trait after fetching a schema definition from the ledger:

Fatal signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x76d39d6fa0 in tid 15686 (mqt_js), pid 15558 (bc.gov.BCWallet)

But there is no indication of async_trait breaking anything here, correct?

I am not aware of the technical implications of removing async_trait and moving to sync, so it might be fine.

https://github.com/dtolnay/async-trait?tab=readme-ov-file#explanation just shows that it adds a box+pin to it, which shouldn't segfault, right?

@wadeking98
Copy link
Contributor Author

it shouldn't segfault because it doesn't in node... but for some reason it does on react native maybe because it's compiled for mobile architectures (arm64 and eabi). My hunch is that box+pin isn't getting properly translated when cross compiling so maybe things are moving around in memory when they should be pinned. That's just a hunch though, I haven't proven it.

Either way I discovered that the async_trait wasn't needed anyway and I was able to do the same functionality using core rust libraries. So I view this PR as a kind of refactoring/code cleanup with the added bonus of fixing a bug for mobile users

@andrewwhitehead
Copy link
Member

andrewwhitehead commented May 27, 2024

From a quick search it looks like SEGV_ACCERR usually indicates a stack overflow, which could be due to a smaller stack being configured on the mobile builds. async_trait would not be the problem so much as running the cache operations within the async executor, although I don't really see why that should cause any issues. If it works then this fix seems fine.

The other build errors seem to have started with the dependabot update PR, but are likely due to a new release of some dependency (cmake?). On Darwin I see it trying to link an arm64 build of libzmq during the x64_64 build.

Edit: the build issues seem to be mostly due to GHA updating to MacOS 14, the workflow is adjusted to use macos-12 for now.

@cvarjao
Copy link
Contributor

cvarjao commented May 27, 2024

@wadeking98, #292 is merged. Try rebasing your PR.

@wadeking98 wadeking98 force-pushed the fix-binary-react-native branch from 7ac4901 to 203c122 Compare May 28, 2024 00:00
@wadeking98
Copy link
Contributor Author

@andrewwhitehead @cvarjao the checks completed successfully

@andrewwhitehead andrewwhitehead merged commit bcf9070 into hyperledger:main May 28, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants