Skip to content

Conversation

@mcbarton
Copy link

This Pull request:

Changes or fixes:

This change enables cling to be built with Emscripten. The Emscripten build of cling doesn't currently work, but these changes allow it to build. If this goes in I will begin debugging what changes are needed to enable it to function in a web browser and node through CppInterOp, which has an Emscripten ci (including tests).

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@ferdymercury
Copy link
Collaborator

Maybe related: #20225

@github-actions
Copy link

Test Results

    22 files      22 suites   3d 14h 27m 50s ⏱️
 3 705 tests  3 705 ✅ 0 💤 0 ❌
79 559 runs  79 559 ✅ 0 💤 0 ❌

Results for commit 62f6ac7.

Copy link
Contributor

@devajithvs devajithvs left a comment

Choose a reason for hiding this comment

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

I think the changes in #20225 should fix Cling build with Emscripten. Let's wait for that before making this change.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

In my opinion, #20225 is the more correct approach because it's not about building with Emscripten, but really about the missing NVPTX backend.

@vgvassilev
Copy link
Member

In my opinion, #20225 is the more correct approach because it's not about building with Emscripten, but really about the missing NVPTX backend.

What has to do the emscripten work with some other PR we discussed. Is there a reason to block the current PR?

@hahnjo
Copy link
Member

hahnjo commented Nov 5, 2025

In my opinion, #20225 is the more correct approach because it's not about building with Emscripten, but really about the missing NVPTX backend.

What has to do the emscripten work with some other PR we discussed. Is there a reason to block the current PR?

I don't understand the first sentence of the comment. And yes, I object to this PR because #ifndef __EMSCRIPTEN__ is wrong and #if LLVM_HAS_NVPTX_TARGET from #20225 is correct.

@vgvassilev
Copy link
Member

vgvassilev commented Nov 5, 2025

and #if LLVM_HAS_NVPTX_TARGET from #20225 is correct.

In your opinion.

Now I see what you meant. Yes, we should not wrap the nvptx initialization around emscripten clauses.

@hahnjo
Copy link
Member

hahnjo commented Nov 5, 2025

and #if LLVM_HAS_NVPTX_TARGET from #20225 is correct.

In your opinion.

Yes, obviously. You asked for the reason to block this PR, there it is. I'm open to discussion, but that's the technical arguments I see so far.

@vgvassilev
Copy link
Member

and #if LLVM_HAS_NVPTX_TARGET from #20225 is correct.

In your opinion.

Yes, obviously. You asked for the reason to block this PR, there it is. I'm open to discussion, but that's the technical arguments I see so far.

Apologies -- I overlooked. Updated my comment...

@mcbarton
Copy link
Author

mcbarton commented Nov 5, 2025

Closing as I checked locally and #20225 achieves the same result of allowing cling to be compiled with Emscripten.

@mcbarton mcbarton closed this Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants