Use patched emsdk 3.1.73 out of emscripten-forge #498
Conversation
| @@ -21,13 +21,12 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| include: | |||
| - name: osx15-arm-clang-clang-repl-19-emscripten_wasm | |||
There was a problem hiding this comment.
I'd rather the deployment stuck with the MacOS runner since it is noticeably faster, and works just like the Linux build (this is where I do my testing)
There was a problem hiding this comment.
Already discussed that we would change the runners to linux as preferred for the build.
There was a problem hiding this comment.
Attaching https://pyodide.org/en/stable/development/building-from-sources.html for reference.
There was a problem hiding this comment.
Discord link of discussion for reference
https://discord.com/channels/1235591463472074924/1235591601523396680/1331522896941678603
There was a problem hiding this comment.
I'm referring to the discussion we had on Zoom yesterday. I don't remember agreeing to switch the deployment runner from MacOS. Only that we would move to emscripten forges emsdk subject to determining whether it was needed once xeus-cpp was more complicated. @vgvassilev can confirm whether this was agreed to or not in our Zoom meeting.
There was a problem hiding this comment.
Okay if not discussed then we can discuss it here. The link to the discord above has all the context regarding the switch.
There was a problem hiding this comment.
We should keep these builds even if they are not preferred. Once they start failing we will figure out what to do.
There was a problem hiding this comment.
Yeah yeah not the point of discussion here !
For the deployment, we need to prefer one correct ? For just that we go with all the stable/recommended options. Make sense ?
There was a problem hiding this comment.
I think I see your point. It was not discussed during our last video call. I'd prefer to change this PR as it was discussed and agreed upon. That means keeping these and moving to the patched version of the sdk.
| executing micromamba install llvm -c | ||
| <https://repo.mamba.pm/emscripten-forge> and setting the LLVM_BUILD_DIR | ||
| appropriately) | ||
| https://repo.prefix.dev/emscripten-forge-dev and setting the LLVM_BUILD_DIR |
There was a problem hiding this comment.
Do we need to change this to make sure it install llvm 19, and not just whatever the latest version emsdk has for now?
There was a problem hiding this comment.
New channel used to get llvm built against 3.1.73
There was a problem hiding this comment.
The channel fix is not what I was referring to. That will fetch the llvm being built in that channel, but to be consistent with the clone it should be fixed to get the latest llvm 19 version.
There was a problem hiding this comment.
Yeah this will fetch the latest llvm version. So no issues !
There was a problem hiding this comment.
That is if only no issues if CppInterOp has been updated to work with the latest version before that release in emscripten forge has happened. If not they will not be able to do the subsequent steps. Best to keep it to a fix version that we know works like with the git clones.
There was a problem hiding this comment.
Hmmm not sure what you mean to say here. But the only way to get llvm from emscripten-forge build against emsdk 3.1.73 is through this channel https://repo.prefix.dev/emscripten-forge-dev
There was a problem hiding this comment.
Here is an example of how it could run into trouble in the future. Let's say emscripten forge adds a build of llvm 20 using emsdk 3.1.73 before CppInterOp is ready for the upgrade, but this command has been merged in and people try to use it, then will get llvm 20, but they won't be able to build CppInterOp with it. Hopefully that makes sense.
Also separately I'm not sure how the llvm install command is made aware of what emsdk we are using. The emscripten forge developer channel will be using a different emsdk in a future upgrade (whenever that may be), which may causes issues if the llvm it gets is built with a different emsdk tonthe one we are using (but this second point is an issue for a different day)
There was a problem hiding this comment.
okay I guess you just want me to pin the version isn't it ?
Also separately I'm not sure how the llvm install command is made aware of what emsdk we are using
Shall educate you on this separately.
| @@ -21,13 +21,12 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| include: | |||
| - name: osx15-arm-clang-clang-repl-19-emscripten_wasm | |||
There was a problem hiding this comment.
I'm referring to the discussion we had on Zoom yesterday. I don't remember agreeing to switch the deployment runner from MacOS. Only that we would move to emscripten forges emsdk subject to determining whether it was needed once xeus-cpp was more complicated. @vgvassilev can confirm whether this was agreed to or not in our Zoom meeting.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
=======================================
Coverage 71.17% 71.17%
=======================================
Files 9 9
Lines 3552 3552
=======================================
Hits 2528 2528
Misses 1024 1024 |
This reverts commit dded6fe.
| @@ -21,30 +21,20 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| include: | |||
| - name: ubu24-arm-gcc12-clang-repl-19-emscripten | |||
There was a problem hiding this comment.
This should be reverted, and instead work should be done make the emsdk from emscripten forge available for Linux arm.
There was a problem hiding this comment.
As part of the evidence as to why we shouldn't be dropping Ubuntu arm from the ci, here is an example of xeus-cpp-lite working on a Ubuntu arm vm. Instead we should make emsdk available on the Ubuntu arm platform as commented above.
There was a problem hiding this comment.
Okay let's add this as a TODO for now and proceed ?
As discussed on the issue, it doesn't matter a lot having a lot of native builds. We aren't generating a lot of value here but if that's something we are interested in we can try supporting it.
emscripten-forge/recipes#1689 (comment)
There was a problem hiding this comment.
Yes, we are interested in that.
There was a problem hiding this comment.
I'd rather not have this labelled as a todo item, just to get the PR in now. We currently are able to have people build on Linux arm (and have it work), while as things stands if itgis PR went in people using Linux arm wouldn't be able to do afterwards. We have agreed to use emscripten forges emsdk (while xeus-cpp-lite is developed more and it is checked the patch you mentioned is needed), but it should be done properly first time. Please do the work to get emsdk available throught emscripten forge on Linux arm first and then revert this ci change.
There was a problem hiding this comment.
Okay In that case I don't know how to proceed.
As per what we discussed we decided to go ahead with the patched emsdk
-
And the main point behind using it was not technically to support a lot of platforms rather it was towards introducing a consistent build. I am not sure if it makes sense to use different emsdk's to build llvm and CppInterOp differently and the other platforms differently
-
That being said, I think what we have is a good start. If we read through the issue that I linked above
i) emscripten_emscripten-wasm32 package only available on Ubuntu platform emscripten-forge/recipes#1689 (comment)
ii) emscripten_emscripten-wasm32 package only available on Ubuntu platform emscripten-forge/recipes#1689 (comment)
Answers by Johan and Thortsen state as to why it shouldn't matter even if we have 1 single good build through the CI.
- Now obviously there's this point where what do users do in that case ? But think about it ....
We end up having to make a choice here
Having an inconsistent build on all platforms vs Supporting more platforms !
- Btw here's are some answers by pyodide maintainers on why we need the patch. I had opened a discussion so that I can get some detailed replies on this
i) Discussing patches on emsdk in Pyodide pyodide/pyodide#5420 (reply in thread)
ii) Discussing patches on emsdk in Pyodide pyodide/pyodide#5420 (comment)
So yeah my point here is that ....
Allowing users to access the patched emsdk to build xeus-cpp-lite actually should end up holding more value than having it through the CI !
We know from a CI standpoint that having 1 good build should do the job in a wasm context
So CI shouldn't be our focus but rather how would a user make use of it. And for that I can open up an issue on emscripten-forge asking it to be supported. I probably won't be able to work on it right away as it doesn't fall under my assigned list of tasks. We can ask our users to make use of the emsdk from github till then and add a TODO for the same.
Also a big point is when it comes to the deployment, shouldn't we work towards the best build possible ? Isn't that what our users would use ?
So even if we don't want to use the patched emsdk untill Ubuntu Arm is supported, we should use it for our deployment build as that is our go-to build. We need to have a consistent build there
@vgvassilev how should we move here ?
My suggestion being if we want to be so demanding out of emscripten-forge cause obviously there's work going on there too and not sure if we would have this right away :/
We just have that one perfect build through our deployment where we use the patched emsdk and not have it through the CI untill Ubuntu Arm is supported ?
There was a problem hiding this comment.
What is stopping us from cloning emscripten forges recipe repo, so we can activate their emsdk manually on all platforms until its available as a package though emscripten forges channels? Having this way of building for now will xeus-cpp-lite to maintain the ability to be built from any available platforms, and we also get whatever patches are currently being applied within emscripten forge.
Thanks for the opening the discussions on Pyodide about the patches. I will look into what is said in the dicusssion (probably Monday)
There was a problem hiding this comment.
What is stopping us from cloning emscripten forges recipe repo, so we can activate their emsdk manually on all platforms until its available as a package though emscripten forges channels?
I think that's a brilliant suggestion. Let's do that ?
There was a problem hiding this comment.
What is stopping us from cloning emscripten forges recipe repo, so we can activate their emsdk manually on all platforms until its available as a package though emscripten forges channels?
I think that's a brilliant suggestion. Let's do that ?
Ping me once you've made that suggested change to the ci, and build instructions and I will take another look at the PR (probably Monday as I am busy this weekend)
There was a problem hiding this comment.
Yeah I am making no commits today. Only on Monday or Teusday
Description
Moving as discussed to the patched emsdk from emscripten-forge for consistency among the builds in the CI and the dependencies that already are built on top of the patched version.
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