Skip to content
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

Upgrade to emsdk to version 3.1.73 #456

Merged
merged 3 commits into from
Feb 2, 2025

Conversation

mcbarton
Copy link
Collaborator

Description

Please include a summary of changes, motivation and context for this PR.

This is a PR to upgrade the emsdk version we use to version 3.1.73, once emscripten forge is finished with its transition to this version.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

  • I have read the contribution guide recently

@mcbarton mcbarton requested a review from anutosh491 January 16, 2025 21:00
@anutosh491
Copy link
Collaborator

I would prefer we use the patched version of emsdk that both pyodide and emscripten-forge use. They use the same patch and to my knowledge it is better for building shared libraries. This will ensure consistency with emscripten-forge too !

See jupyter-xeus/xeus-r#116 on how to do it

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.17%. Comparing base (490dfcb) to head (4817bab).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #456   +/-   ##
=======================================
  Coverage   71.17%   71.17%           
=======================================
  Files           9        9           
  Lines        3552     3552           
=======================================
  Hits         2528     2528           
  Misses       1024     1024           

@anutosh491
Copy link
Collaborator

Hi,

Could you please finish this PR ?
Cause once done we can try moving to static build for cppinterop !

@mcbarton mcbarton marked this pull request as ready for review January 31, 2025 06:03
@anutosh491
Copy link
Collaborator

anutosh491 commented Jan 31, 2025

Was going through the PR. There are some changes needed

  1. As decided we should prefer linux on all the wasm fronts (so https://github.com/compiler-research/CppInterOp/pull/456/files#diff-0f132b0962009071404990a6789b3466dbd8723d1f7b744f6aca0312396a7900R25 needs to be changed)
  2. Also we would be using the patched emsdk either from pyodide or emscripten-forge hence https://github.com/compiler-research/CppInterOp/pull/456/files#diff-0f132b0962009071404990a6789b3466dbd8723d1f7b744f6aca0312396a7900R76-R81 should be changed (no setup required. Use it right from the env. Check https://github.com/compiler-research/xeus-cpp/blob/main/.github/workflows/deploy-github-page.yml)
  3. These changes are all applicable to the main.yml and the documentation too. This lastest changes through Update xeus-cpp-lite to use emsdk 3.1.73 xeus-cpp#246 should guide you well

@@ -27,7 +27,7 @@ jobs:
clang-runtime: '19'
cling: Off
micromamba_shell_init: bash
emsdk_ver: "3.1.45"
emsdk_ver: "3.1.73"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can completely be removed once the above comment is addressed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly for docs and the main.yml. We wouldn't need to store this in a variable !

@mcbarton mcbarton merged commit a901f57 into compiler-research:main Feb 2, 2025
73 of 74 checks passed
@anutosh491
Copy link
Collaborator

@mcbarton why was this merged ?
I don't see any of my comments being addressed!

@anutosh491
Copy link
Collaborator

?

@compiler-research compiler-research locked and limited conversation to collaborators Feb 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants