compiler-research / CppInterOp

A Clang-based C++ Interoperability Library
Other
38 stars 20 forks source link

[CI] Add wasm support to CI for osx and Linux #198

Closed mcbarton closed 4 months ago

mcbarton commented 4 months ago

The purpose of this PR is to add wasm support to CppInterOp Fixes https://github.com/compiler-research/CppInterOp/issues/183

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.63%. Comparing base (5408538) to head (6e51d18).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/198/graphs/tree.svg?width=650&height=150&src=pr&token=7UWTYSVVT5&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research)](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) ```diff @@ Coverage Diff @@ ## main #198 +/- ## ======================================= Coverage 78.63% 78.63% ======================================= Files 8 8 Lines 3056 3056 ======================================= Hits 2403 2403 Misses 653 653 ```
vgvassilev commented 4 months ago

Isn't there some emscripten forge thing where we can pull some dependencies in?

mcbarton commented 4 months ago

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.

mcbarton commented 4 months ago

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

vgvassilev commented 4 months ago

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

mcbarton commented 4 months ago

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

mcbarton commented 4 months ago

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

vgvassilev commented 4 months ago

The cache for this PR is deleted now.

mcbarton commented 4 months ago

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.

vgvassilev commented 4 months ago

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 commented 4 months ago

@vgvassilev This PR is ready for review.

vgvassilev commented 4 months ago

Thank you! I am afraid I will need help to review this one. Maybe @anutosh491 can help?

mcbarton commented 4 months ago

@anutosh491 are you able to review this PR for me please?

anutosh491 commented 4 months ago

Ahh sorry for the delay on this, going through it now.

mcbarton commented 4 months ago

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

vgvassilev commented 4 months ago

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

mcbarton commented 4 months ago

@anutosh491 do you have anymore comments on the PR, or do you approve it?

vgvassilev commented 4 months ago

Thanks @anutosh491. Well done @mcbarton. Now we need to figure out how to plug this into the conda recipe here...