-
Notifications
You must be signed in to change notification settings - Fork 72
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
Update emscripten [2 XMR] #144
Comments
There is a bounty on this issue. The amount is in the title. The reward will be awarded to the first person or group of people who resolve this issue. If you are starting to work on this bounty, please write a comment so that we can assign the issue to you. We expect contributors to provide a PR in a reasonable timeframe or, in case of an extensive work, updates on their progress. We will unassign the issue if we feel the assignee is not responsive or has abandoned the task. Read the full conditions and details of the bounty system. |
See the work in progress draft PR as a starting point: #192 |
I will take this. Please assign. |
Great, I've assigned to you @NorrinRadd |
I forgot to mention, the upgrade should not use multithreading or pthreads, like I was experimenting with in the draft PR. Instead, it should still be single-threaded. |
I have all the builds working except for with the new version of emscripten. To future-proof it, threads seem important. Threads were disabled somehow with the previous version? @woodser |
Great to hear.
Yes agreed, we do want threads, but it requires additional configuration to make it work in the browser. So I'd prefer we complete the upgrade to the latest version of emscripten first, to avoid breaking dependent applications. And then with the latest version of emscripten, we can add threading support. |
But if you can get it working with threads (since the new emscripten seems to really prefer that), then that's great progress, and we could attempt to remove threads after. |
yeah the monero project seems to require threads: https://github.com/monero-project/monero/blob/master/src/wallet/node_rpc_proxy.h#L32 |
This project builds and runs now against monero-project without pthreads, so it's not required that multiple threads are enabled, even with the imports. You should be able to confirm the working build with emscripten 3.1.10. |
@woodser i got it to build with the latest emscripten. Attempting to run tests now. Your |
@woodser PR submitted |
Along with #206,
|
@woodser Update from emscripten team: They're of the belief that this is deliberate from Boost; that if certain classes are used, then multi-threading must be enabled. It also seems like it was an error that Emscripten < 3.1.26 was avoiding this somehow, but I have not received a reply from them on that yet.
You can check the first PR (regarding multi-threading support) that it has been updated with a fix for the shared buffer issue, as well as a web worker issue. The remainder of the work seems to be updating monero-ts to actually use the web workers. |
Thanks for the update. Maybe multithreading is the way forward, though will require changes to any downstream dependents. |
@woodser, Have you seen the update that denotes that Emscripten 3.1.26 can
be used? It gets the upgrade part of the way there.
…On Mon, Apr 22, 2024 at 9:44 AM woodser ***@***.***> wrote:
Thanks for the update. Maybe multithreading is the way forward, though
will require changes to any downstream dependents.
—
Reply to this email directly, view it on GitHub
<#144 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHNEHQCCRP3WS667Y5ZET3Y6TEYTAVCNFSM6AAAAAA5RLGSOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRYHAZTENJUHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, that's definitely a step in the right direction. I'll need to play with it, and either we update to that intermediately or go for the big update. |
@woodser please what is the update? emscripten has been updated as far as possible. Please evaluate this pull request. |
Ideally we can update to the latest emscripten, built with pthreads, but with pthreads disabled as a final step in the linker flags, before a major update to support real threading. It's limited use to update halfway. |
Resolved by @mainnet-pat in #258 |
This issue requests updating the build to use the latest version of emscripten.
The library is currently using version 3.1.10.
The text was updated successfully, but these errors were encountered: