-
Notifications
You must be signed in to change notification settings - Fork 30
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
Improve emscripten llvm build in ci #384
Improve emscripten llvm build in ci #384
Conversation
44e77bc
to
bf6a1dd
Compare
Hey @mcbarton I shall review this by tomorrow! Thanks |
@anutosh491 I'll cc you to review once its ready (hopefully i'll have it done by tomorrow). At the moment I am fixing issues with this PR. |
e4390eb
to
7e407fa
Compare
d4d43cf
to
436827e
Compare
.github/workflows/ci.yml
Outdated
@@ -1385,4 +1628,4 @@ jobs: | |||
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \ | |||
-DCppInterOp_DIR="${{ env.CPPINTEROP_BUILD_DIR }}/lib/cmake/CppInterOp" \ | |||
.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see micromamba activate CppInterOp-wasm
on line 1616
Probably that's not required. We would like to stay in the CppInterOp-wasm-build for this !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CppInterOp-wasm build is activate by default when you start an action because of here https://github.com/mcbarton/CppInterOp/blob/027cbb1fc65a39f4b2bb2d2b38fa21c8019b2b57/.github/workflows/ci.yml#L1528 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah what I meant is that activating cppinterop-wasm is not required here. We would like to stay in cppinterop-wasm-build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shall make the change in my PR as this is more relevant to the shared build.
Also you could get rid of the zlib changes now ! |
ce0ad5e
to
f025f32
Compare
2bf383f
to
8cb1e13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now !
Description
Please include a summary of changes, motivation and context for this PR.
This PR should make the emscripten build of llvm we make in the ci match that in Emscripten forge.
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist