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

New package: emscripten #6578

Merged
3 commits merged into from
Apr 24, 2021
Merged

New package: emscripten #6578

3 commits merged into from
Apr 24, 2021

Conversation

truboxl
Copy link
Contributor

@truboxl truboxl commented Mar 29, 2021

No description provided.

@truboxl truboxl marked this pull request as draft March 29, 2021 18:57
@truboxl truboxl force-pushed the truboxl-emscripten branch from e6255b1 to 4f086f8 Compare March 29, 2021 19:10
@truboxl
Copy link
Contributor Author

truboxl commented Mar 29, 2021

Please note that this is a build test. I haven't do proper running on device yet.
Just want to ask does GH Action also work on forked repo? So that I can test over my forked repo first...

@Grimler91
Copy link
Member

Just want to ask does GH Action also work on forked repo? So that I can test over my forked repo first...

It does (except the upload job of course), you just need to enable github actions at the actions tab in your repo

@truboxl truboxl force-pushed the truboxl-emscripten branch from 7485a0c to 7a402d5 Compare April 1, 2021 13:03
@truboxl truboxl changed the title test: New package: emscripten New package: emscripten Apr 3, 2021
@truboxl truboxl force-pushed the truboxl-emscripten branch from f60fe0a to 4d925c2 Compare April 4, 2021 13:57
@Grimler91
Copy link
Member

@truboxl please consider setting yourself as TERMUX_PKG_MAINTAINER for emscripten, partly so that you get all the credit for the port, and partly since you would be the best person to solve possible future issues in the package

@finagolfin
Copy link
Member

Still a draft?

@truboxl
Copy link
Contributor Author

truboxl commented Apr 9, 2021

Sorry for the delay, testing a cross-compiled cross compiler is difficult and time wasting for me. I have been trying to compare the port with emsdk version which directly build emscripten on device from the tree. The summary:

  1. Don't use -O2 and higher, binaryen wasm-opt will segfault, even on Hello world!. This also happens with emsdk version of binaryen. Not sure what is the problem. crash in wasm-opt at -O2 WebAssembly/binaryen#3697 seems to indicate that it has been fixed, and the code is present but still crashes

  2. Some tests provided by emscripten itself: https://emscripten.org/docs/getting_started/test-suite.html, also fail. Can either be attributed to No.1, or third party tests that are not packaged, or x86 Linux only tests, or some other stuff. And the tests result may differ at each run. Also not sure what is the cause. https://gist.github.com/truboxl/4332a198fbc105e99fd7aeff954b83d2

    The constantly rolling emsdk version seems to fair better in tests than this port. https://gist.github.com/truboxl/53bf2d4aa44775be1d5e23d97f286d3c

    Maybe I will package the third party test as subpackage and wrestling portrm for that case in the next release...

  3. emscripten-llvm has some bits from unreleased NDK that prevented it from using as native compiler. eg: missing libunwind support. [FR] switch all architectures to LLVM's libunwind android/ndk#1230 So just keep it for emscripten private use.

  4. As long as you are not using some edge case options that are part of the failed test, the emscripten compiler just works. To be sure, I compiled https://github.com/OneLoneCoder/olcPixelGameEngine emscripten demo: https://www.youtube.com/watch?v=MrI5kkVY6zk It seems to work. Just don't use -O2.

I think @Grimler91 @buttaface and others can review now...

@truboxl truboxl marked this pull request as ready for review April 9, 2021 19:28
@truboxl truboxl force-pushed the truboxl-emscripten branch from f2f204a to 5101815 Compare April 10, 2021 09:22
@truboxl
Copy link
Contributor Author

truboxl commented Apr 13, 2021

For whatever reason, switching binaryen to debug build and wasm-opt segfaults go away, more core tests successfully pass...
I think I have to go back finding the root cause. Setting back to draft...

@truboxl truboxl marked this pull request as draft April 13, 2021 03:49
@truboxl
Copy link
Contributor Author

truboxl commented Apr 14, 2021

Reported a new issue to binaryen upstream at WebAssembly/binaryen#3806
Looks like threading problem in Release build. Not sure how to fix that.

binaryen CMakeLists.txt forces -O2 or -O0. The -Oz is overridden anyway by one of those two. I think I will stick Debug -O0 which do not have threading problems. I don't think I want to patch CMakeLists.txt. What do you guys think?

@truboxl truboxl marked this pull request as ready for review April 14, 2021 17:43
@finagolfin
Copy link
Member

I see no problem with patching the CMake config, do it all the time.

@truboxl truboxl force-pushed the truboxl-emscripten branch from d7ce366 to 00cf736 Compare April 16, 2021 14:46
@truboxl
Copy link
Contributor Author

truboxl commented Apr 16, 2021

Threading issue has been fixed in upstream so using that commit for binaryen, thus one less patch to maintain 🤣

https://gist.github.com/truboxl/91aac174873a05536244fb75ebb0d5de
The core tests passed with 96.8% ((712 - 9 - 12 - 54) / (712 - 54)) consistently so I am fairly confident with this port
The failures are down to not including 3rd party tests for space reasons and LLVM 13 not able to generate native code for now...

@truboxl truboxl force-pushed the truboxl-emscripten branch from 4afc8ae to 39342df Compare April 20, 2021 16:24
@truboxl
Copy link
Contributor Author

truboxl commented Apr 24, 2021

Closes #5043

@ghost
Copy link

ghost commented Apr 24, 2021

@truboxl Ready for merging?

@truboxl
Copy link
Contributor Author

truboxl commented Apr 24, 2021

@xeffyr I believe I tested enough... 😂
For the really edge cases, its up to others to report their issues...

EDIT: Ready for merge if @termux approve

@ghost
Copy link

ghost commented Apr 24, 2021

build.sh look fine. However there is a question about

TERMUX_PKG_RECOMMENDS="emscripten-llvm, emscripten-binaryen"

This will make packages emscripten-llvm and emscripten-binaryen installable among with emscripten by default (i.e. weak dependency). If this is not intended behavior, then TERMUX_PKG_SUGGESTS should be used instead.

@truboxl
Copy link
Contributor Author

truboxl commented Apr 24, 2021

The emscripten package currently depends on a specific snapshot of LLVM and binaryen
so I split them up into smaller subpackages and use TERMUX_PKG_RECOMMENDS
Its pretty big if I combine all of them

I think I can use TERMUX_PKG_SUGGESTS once it no longer requires those specific versions of dependencies

@ghost ghost merged commit 0aba7bd into termux:master Apr 24, 2021
@truboxl truboxl deleted the truboxl-emscripten branch April 25, 2021 02:49
This pull request was closed.
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.

3 participants