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

Update emscripten [2 XMR] #144

Open
woodser opened this issue Oct 3, 2023 · 19 comments · May be fixed by #198 or #206
Open

Update emscripten [2 XMR] #144

woodser opened this issue Oct 3, 2023 · 19 comments · May be fixed by #198 or #206
Assignees
Labels
💰bounty There is a bounty on this issue

Comments

@woodser
Copy link
Owner

woodser commented Oct 3, 2023

This issue requests updating the build to use the latest version of emscripten.

The library is currently using version 3.1.10.

@woodser woodser added the 💰bounty There is a bounty on this issue label Mar 26, 2024
Copy link

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.

@woodser woodser changed the title Update emscripten Update emscripten [2 XMR] Mar 26, 2024
@woodser
Copy link
Owner Author

woodser commented Mar 26, 2024

See the work in progress draft PR as a starting point: #192

@NorrinRadd
Copy link

I will take this. Please assign.

@woodser
Copy link
Owner Author

woodser commented Mar 29, 2024

Great, I've assigned to you @NorrinRadd

@woodser
Copy link
Owner Author

woodser commented Mar 29, 2024

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.

@NorrinRadd
Copy link

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

@woodser
Copy link
Owner Author

woodser commented Apr 2, 2024

I have all the builds working except for with the new version of emscripten.

Great to hear.

To future-proof it, threads seem important. Threads were disabled somehow with the previous version? @woodser

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.

@woodser
Copy link
Owner Author

woodser commented Apr 2, 2024

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.

@NorrinRadd
Copy link

yeah the monero project seems to require threads: https://github.com/monero-project/monero/blob/master/src/wallet/node_rpc_proxy.h#L32

@woodser
Copy link
Owner Author

woodser commented Apr 4, 2024

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.

@NorrinRadd
Copy link

@woodser i got it to build with the latest emscripten. Attempting to run tests now. Your npm test requires that testnet be fully synced and not pruned?

@NorrinRadd NorrinRadd linked a pull request Apr 5, 2024 that will close this issue
@NorrinRadd
Copy link

@woodser PR submitted

@NorrinRadd NorrinRadd linked a pull request Apr 21, 2024 that will close this issue
@NorrinRadd
Copy link

NorrinRadd commented Apr 21, 2024

Along with #206,

@NorrinRadd
Copy link

@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.

ignoring nonexistent directory "/Users/user/code/emsdk/upstream/emscripten/cache/sysroot/include/ wasm32-emscripten/c++/v1"
ignoring nonexistent directory "/Users/user/code/emsdk/upstream/emscripten/cache/sysroot/include/ wasm32-emscripten"
#include "..." search starts here:
#include <...> search starts here:
 .
 /Users/user/code/emsdk/upstream/emscripten/cache/sysroot/include/fakesdl
 /Users/user/code/emsdk/upstream/emscripten/cache/sysroot/include/compat
 /Users/user/code/emsdk/upstream/emscripten/cache/sysroot/include/c++/v1
 /Users/user/code/emsdk/upstream/lib/clang/19/include
 /Users/user/code/emsdk/upstream/emscripten/cache/sysroot/include
End of search list.
In file included from libs/thread/src/pthread/thread.cpp:9:
In file included from ./boost/thread/detail/config.hpp:13:
In file included from ./boost/thread/detail/platform.hpp:17:
./boost/config/requires_threads.hpp:29:4: error: "Threading support unavaliable: it has been      explicitly disabled with BOOST_DISABLE_THREADS"
   29 | #  error "Threading support unavaliable: it has been explicitly disabled with             BOOST_DISABLE_THREADS"
      |    ^   
1 error generated.

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.

@woodser
Copy link
Owner Author

woodser commented Apr 22, 2024

Thanks for the update. Maybe multithreading is the way forward, though will require changes to any downstream dependents.

@NorrinRadd
Copy link

NorrinRadd commented Apr 22, 2024 via email

@woodser
Copy link
Owner Author

woodser commented Apr 22, 2024

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.

@NorrinRadd
Copy link

@woodser please what is the update? emscripten has been updated as far as possible. Please evaluate this pull request.

@woodser
Copy link
Owner Author

woodser commented May 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💰bounty There is a bounty on this issue
Projects
None yet
2 participants