Use patched emsdk 3.1.73 out of emscripten-forge #498
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -21,30 +21,20 @@ jobs: | ||
| fail-fast: false | ||
| matrix: | ||
| include: | ||
|
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. 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
Answers by Johan and Thortsen state as to why it shouldn't matter even if we have 1 single good build through the CI.
Having an inconsistent build on all platforms vs Supporting more platforms !
So yeah my point here is that .... 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 :/ 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.
I think that's a brilliant suggestion. Let's do that ? There was a problem hiding this comment.
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 |
||
| - name: osx15-arm-clang-clang-repl-19-emscripten | ||
| os: macos-15 | ||
| compiler: clang | ||
| clang-runtime: '19' | ||
| cling: Off | ||
| llvm_enable_projects: "clang;lld" | ||
| llvm_targets_to_build: "WebAssembly" | ||
| - name: ubu24-x86-gcc12-clang-repl-19-emscripten | ||
| os: ubuntu-24.04 | ||
| compiler: gcc-12 | ||
| clang-runtime: '19' | ||
| cling: Off | ||
| llvm_enable_projects: "clang;lld" | ||
| llvm_targets_to_build: "WebAssembly" | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| @@ -115,13 +105,6 @@ jobs: | ||
| key: ${{ env.CLING_HASH }}-${{ runner.os }}-${{ matrix.os }}-${{ matrix.compiler }}-clang-${{ matrix.clang-runtime }}.x-emscripten | ||
| lookup-only: true | ||
|
|
||
| - name: Setup default Build Type on *nux | ||
| if: ${{ runner.os != 'windows' && steps.cache.outputs.cache-hit != 'true' }} | ||
| run: | | ||
| @@ -262,11 +245,17 @@ jobs: | ||
| sudo apt-get autoremove | ||
| sudo apt-get clean | ||
|
|
||
| - name: install mamba | ||
| if: ${{ steps.cache.outputs.cache-hit != 'true' }} | ||
| uses: mamba-org/setup-micromamba@main | ||
| with: | ||
| environment-file: environment-wasm-build.yml | ||
| init-shell: bash | ||
| environment-name: CppInterOp-wasm-build | ||
|
|
||
| - name: Build LLVM/Cling on Unix systems if the cache is invalid (emscripten) | ||
| if: ${{ runner.os != 'windows' && steps.cache.outputs.cache-hit != 'true' }} | ||
| run: | | ||
| cling_on=$(echo "${{ matrix.cling }}" | tr '[:lower:]' '[:upper:]') | ||
| if [[ "${cling_on}" == "ON" ]]; then | ||
| git clone https://github.com/root-project/cling.git | ||
| @@ -465,21 +454,12 @@ jobs: | ||
| clang-runtime: '19' | ||
| cling: Off | ||
| micromamba_shell_init: bash | ||
| - name: osx15-arm-clang-clang-repl-19-emscripten_wasm | ||
| os: macos-15 | ||
| compiler: clang | ||
| clang-runtime: '19' | ||
| cling: Off | ||
| micromamba_shell_init: bash | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| @@ -524,19 +504,6 @@ jobs: | ||
| echo "ncpus=$(nproc --all)" >> $GITHUB_ENV | ||
| fi | ||
|
|
||
| - name: Restore Cache LLVM/Clang runtime build directory | ||
| uses: actions/cache/restore@v4 | ||
| id: cache | ||
| @@ -545,16 +512,22 @@ jobs: | ||
| llvm-project | ||
| ${{ matrix.cling=='On' && 'cling' || '' }} | ||
| key: ${{ env.CLING_HASH }}-${{ runner.os }}-${{ matrix.os }}-${{ matrix.compiler }}-clang-${{ matrix.clang-runtime }}.x-emscripten | ||
|
|
||
| - name: install mamba | ||
| uses: mamba-org/setup-micromamba@main | ||
| with: | ||
| environment-file: environment-wasm-build.yml | ||
| init-shell: bash | ||
| environment-name: CppInterOp-wasm-build | ||
|
|
||
| - name: Emscripten build of CppInterOp on Unix systems | ||
| if: ${{ runner.os != 'windows' }} | ||
| shell: bash -l {0} | ||
| run: | | ||
| micromamba create -f environment-wasm-host.yml --platform=emscripten-wasm32 | ||
|
|
||
| export PREFIX=$MAMBA_ROOT_PREFIX/envs/CppInterOp-wasm-host | ||
| export BUILD_PREFIX=$MAMBA_ROOT_PREFIX/envs/CppInterOp-wasm-build | ||
| export CMAKE_PREFIX_PATH=$PREFIX | ||
| export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX | ||
|
|
||
| @@ -597,7 +570,6 @@ jobs: | ||
| -DBUILD_SHARED_LIBS=ON \ | ||
| -DCODE_COVERAGE=${{ env.CODE_COVERAGE }} \ | ||
| -DCMAKE_INSTALL_PREFIX=$PREFIX \ | ||
| -DLLVM_ENABLE_WERROR=On \ | ||
| ../ | ||
| fi | ||
| @@ -612,26 +584,22 @@ jobs: | ||
| echo "LLVM_BUILD_DIR=$LLVM_BUILD_DIR" >> $GITHUB_ENV | ||
| echo "CPLUS_INCLUDE_PATH=$CPLUS_INCLUDE_PATH" >> $GITHUB_ENV | ||
| echo "PREFIX=$PREFIX" >> $GITHUB_ENV | ||
| echo "BUILD_PREFIX=$BUILD_PREFIX" >> $GITHUB_ENV | ||
|
|
||
| - name: Build xeus-cpp | ||
| shell: bash -l {0} | ||
| run: | | ||
| export SYSROOT_PATH=$BUILD_PREFIX/opt/emsdk/upstream/emscripten/cache/sysroot | ||
| git clone --depth=1 https://github.com/compiler-research/xeus-cpp.git | ||
| cd ./xeus-cpp | ||
| mkdir build | ||
| pushd build | ||
| emcmake cmake \ | ||
| -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \ | ||
| -DCMAKE_PREFIX_PATH=${{ env.PREFIX }} \ | ||
| -DCMAKE_INSTALL_PREFIX=${{ env.PREFIX }} \ | ||
| -DXEUS_CPP_EMSCRIPTEN_WASM_BUILD=ON \ | ||
| -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \ | ||
| -DCppInterOp_DIR="${{ env.CPPINTEROP_BUILD_DIR }}/lib/cmake/CppInterOp" \ | ||
| -DSYSROOT_PATH=$SYSROOT_PATH \ | ||
| .. | ||
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already discussed that we would change the runners to linux as preferred for the 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.
Attaching https://pyodide.org/en/stable/development/building-from-sources.html for reference.
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.
Discord link of discussion for reference
https://discord.com/channels/1235591463472074924/1235591601523396680/1331522896941678603
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.