-
Notifications
You must be signed in to change notification settings - Fork 15
Stop fetching emsdk from conda-forge #108
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
Conversation
IsabelParedes
left a comment
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.
Good idea!
| cd $HOME | ||
| git clone https://github.com/emscripten-core/emsdk.git | ||
| cd emsdk | ||
| ./emsdk install ${{ matrix.emsdk_ver }} |
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.
How about we also add version 3.1.58 and/or latest to the matrix?
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.
Yes this was very beneficial for me atleast for xeus-cpp-lite. I think it would surely help with xeus-r !
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.
Wait actually no, I think there is a flaw here.
We fetch most things like r-base etc from emscripten-forge through environment-wasm-host.yml
We would want atleast the important packages in the toolchain like r-base, xeus-lite etc to be built against these versions too. (P.S: when I built xeus-cpp-lite against emsdk 3.1.73 ... first I had build errors cause llvm was build against 3.1.45 and once I addressed that, there were build errors from xeus-lite (some symbol mismatch or not exported) and then I built that against 3.1.73 and got xeus-cpp-lite working)
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.
So I think we should stick to 1 emsdk version for all pacakges for now (and I guess once emscripten-forge migrates to a new version like latest i.e. 3.1.73 on a separate branch) we can do this.
|
That being said I think the wasm build env becomes redundant for the CI. Let me try removing it ! |
|
This may not be necessary anymore, switched to using emsdk from emscripten-forge ( #116 ). |
Yeah maybe the patched emsdk from emscripten-forge is a better option to use. (cc @martinRenou we might want to use the same for xeus, xeus-python etc ... I think all of them just clone from upstream) |
|
Agreed. I actually learned this week we had a patched emsdk on emscripten-forge :D The patches only impact the runtime though, so if we're just testing compilation we may not reach any issue. Still, it's better to use the patched one since this is the one being used on emscripten-forge and that's what we want to test. |
No description provided.