Skip to content

Conversation

@mcbarton
Copy link
Collaborator

Description

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

This reverts commit 5860e95. Although with this commit xeus-cpps Emscripten ci passes, it made CppInterOps Emscripten ci fail. The Emscripten ci in CppInterOp has failed ever since this commit went in. I initially discounted it as the source of the problem, despite it being the obvious difference between when the ci passed and when it failed, but that is because I made the silly mistake of forgetting there is a big difference between git reset --soft HEAD~ and git reset --hard HEAD~ when undoing the last commit of a repo. I opened up a dummy repo in CppInterOp here https://github.com/compiler-research/CppInterOp/pull/749/files which hard resets xeus-cpps latest commit to show the Emscripten ci passes again without it. See here for the workflow run of that PR https://github.com/compiler-research/CppInterOp/actions/runs/19310693489 .

@mcbarton
Copy link
Collaborator Author

mcbarton commented Nov 12, 2025

Since this PR is a revert of a commit to get CppInterOps Emscripten ci passing again I am inclined to ignore the git clang format and git clang review workflows.

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.94%. Comparing base (5860e95) to head (78bfc75).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
- Coverage   82.40%   81.94%   -0.46%     
==========================================
  Files          21       21              
  Lines         858      853       -5     
  Branches       89       87       -2     
==========================================
- Hits          707      699       -8     
- Misses        151      154       +3     
Files with missing lines Coverage Δ
src/xinterpreter.cpp 90.56% <100.00%> (-0.29%) ⬇️

... and 1 file with indirect coverage changes

Files with missing lines Coverage Δ
src/xinterpreter.cpp 90.56% <100.00%> (-0.29%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@mcbarton
Copy link
Collaborator Author

Since this PR is a revert of a commit to get CppInterOps Emscripten ci passing again I am inclined to ignore the git clang format and git clang review workflows.

I have clicked resolved on all git clang review comments due to the reason mentioned here.

Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

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

Have we found the fundamental reason as to how this results into a failure ?

I don't think I'm comfortable with a revert without knowing what exactly happens.

This was not an emscripten build specific PR right ?
It was solving a bug in both the native and the wasm kernel. And cppinterop's native CI passes for the same correct or does it not ?

Cause if it does, then there is no difference in this aspect between the latest build of cppinterop from a package manager and the latest main that can cause this right ? Which means the bug fix might not "technically" be the reason for the failure.

On top of it, the PR didn't mess with any build flags/configs, so this raises more questions.

@anutosh491
Copy link
Collaborator

Can you isolate the error locally ? Cause I haven't been able to

I built latest cppinterop using emsdk 3.1.73 (not the patched one) and made use of it to build test_xeus_cpp.js and I don't see an error using it locally. Everything passes as expected.

Not sure why it fails for you if it does, but let's experiement around I guess
Comment out all tests except the one you think is buggy and re-run test_xeus_cpp.js against node 22. And tell me what do you see ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants