monero-ts icon indicating copy to clipboard operation
monero-ts copied to clipboard

Update emscripten [2 XMR]

Open woodser opened this issue 1 year ago • 19 comments

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

The library is currently using version 3.1.10.

woodser avatar Oct 03 '23 17:10 woodser

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.

github-actions[bot] avatar Mar 26 '24 17:03 github-actions[bot]

See the work in progress draft PR as a starting point: https://github.com/woodser/monero-ts/pull/192

woodser avatar Mar 26 '24 17:03 woodser

I will take this. Please assign.

NorrinRadd avatar Mar 29 '24 13:03 NorrinRadd

Great, I've assigned to you @NorrinRadd

woodser avatar Mar 29 '24 14:03 woodser

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.

woodser avatar Mar 29 '24 14:03 woodser

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

NorrinRadd avatar Apr 02 '24 12:04 NorrinRadd

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 avatar Apr 02 '24 12:04 woodser

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.

woodser avatar Apr 02 '24 13:04 woodser

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

NorrinRadd avatar Apr 04 '24 08:04 NorrinRadd

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 avatar Apr 04 '24 08:04 woodser

@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 avatar Apr 04 '24 22:04 NorrinRadd

@woodser PR submitted

NorrinRadd avatar Apr 05 '24 11:04 NorrinRadd

Along with #206,

  • isolated the single-threaded build errors down to the build_boost_emscripten phase.
  • downgraded boost to a version that was out around when emscripten 3.1.26 (last working version) was out >> still fails. Therefore the culprit seems to be emscripten.
  • sent a bug report to emscripten regarding the newer versions: https://github.com/emscripten-core/emscripten/issues/21793 Hopefully can get some transaction soon.

NorrinRadd avatar Apr 21 '24 06:04 NorrinRadd

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

NorrinRadd avatar Apr 22 '24 00:04 NorrinRadd

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

woodser avatar Apr 22 '24 08:04 woodser

@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 https://github.com/woodser/monero-ts/issues/144#issuecomment-2068832549, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHNEHQCCRP3WS667Y5ZET3Y6TEYTAVCNFSM6AAAAAA5RLGSOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRYHAZTENJUHE . You are receiving this because you were mentioned.Message ID: @.***>

NorrinRadd avatar Apr 22 '24 09:04 NorrinRadd

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 avatar Apr 22 '24 09:04 woodser

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

NorrinRadd avatar May 14 '24 16:05 NorrinRadd