[CI] Add wasm support to CI for osx and Linux#198
[CI] Add wasm support to CI for osx and Linux#198vgvassilev merged 1 commit intocompiler-research:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #198 +/- ##
=======================================
Coverage 78.63% 78.63%
=======================================
Files 8 8
Lines 3056 3056
=======================================
Hits 2403 2403
Misses 653 653 |
|
Isn't there some emscripten forge thing where we can pull some dependencies in? |
Quite possibly, but I'm not too familiar with emscripten, so that is the reason I tried to build my own wasm zlib, and find it that way like I did locally. That doesn't seem to be working on the Github runner though. |
|
@vgvassilev I'll try again later using an environment yaml file like in xeus-cpp. Maybe https://repo.mamba.pm/emscripten-forge might have everything we will need, since it has zlib , which is the current stumbling block. |
Can’t we try to get the llvm binaries from there? |
The only reason I didn't is I because of the patches on clang 16 and 17 that CppInterOp uses.. Is this not an issue? I can add this to the environment yml file if not, as this appears to be working now. I just need to understand why it can't find the wasm installed dependencies. I can hardcode in the paths for now until I understand why. |
|
@vgvassilev Now that I have the configure and build stage for the wasm build passing, can you delete the llvm cache for this PR? I want to see if I can get the wasm and non wasm builds using the same llvm cache. There doesn't appear to be any reason they can't use the same one. I will take a look into codebase changes after this, but I may put the codebase changes as a subsequent PR if that is ok. |
|
The cache for this PR is deleted now. |
@vgvassilev My cache was not deleted. I just went to rebuild the cache, and it restored a llvm build from cache. |
Strange. Now I deleted everything. |
mcbarton
force-pushed
the
main
branch
3 times, most recently
from
9540a06 to
2936a72
Compare
mcbarton
changed the title
|
@vgvassilev This PR is ready for review. |
|
Thank you! I am afraid I will need help to review this one. Maybe @anutosh491 can help? |
|
@anutosh491 are you able to review this PR for me please? |
|
Ahh sorry for the delay on this, going through it now. |
|
|
||
| if(EMSCRIPTEN) | ||
| message("Build with emscripten") | ||
| option(CPPINTEROP_ENABLE_TESTING "Enables the testing infrastructure." OFF) |
There was a problem hiding this comment.
Curious to know if there's any concrete reason as to why we're not planning to enable testing here ?
There was a problem hiding this comment.
This was done for no other reason than that was what was done in xeus-cpp (i.e. testing turned off by default when emscripten is on) https://github.com/compiler-research/xeus-cpp/blob/216dbed46e358d82fd8fe3065b4533f3e1c49119/CMakeLists.txt#L77C1-L77C34 .
There was a problem hiding this comment.
Works can skip that for now !
|
@vgvassilev The jobs failing after the suggested change to emscripten 3.1.45 are due to a cache issue as far as I can tell. Can you clear the cache and manually reactivate the workflow? It should get all green checks after this. |
@mcbarton, the cache is cleaned. Maybe you can restart? |
|
@anutosh491 do you have anymore comments on the PR, or do you approve it? |
anutosh491
left a comment
anutosh491
left a comment
There was a problem hiding this comment.
I think this looks good. Maybe the hardcoding of PREFIX path can be looked into in the future.
|
Thanks @anutosh491. Well done @mcbarton. Now we need to figure out how to plug this into the conda recipe here... |
3 participants
The purpose of this PR is to add wasm support to CppInterOp
Fixes #183