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

[cmake] Add emscan-deps tool to enable C++20 #21987

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 23, 2024

This is wrapper around clang-scan-deps. Currently this is just enough to make C++20 work under cmake. We don't currently have any actaully tests of C++20 modules.

See: #21042
Fixes: #21866 #22305

@sbc100 sbc100 requested review from dschuff and kripken May 23, 2024 16:28
@sbc100 sbc100 force-pushed the scan_deps branch 3 times, most recently from b85c465 to 5849027 Compare May 23, 2024 18:13
emscan-deps.py Outdated Show resolved Hide resolved
test/cmake/cxx20/main.cpp Outdated Show resolved Hide resolved
emscan-deps.py Outdated Show resolved Hide resolved
@dschuff
Copy link
Member

dschuff commented May 23, 2024

Is it actually necessary to have the em-foo wrapper? Is there some reason we can't just tell CMake the location of the clang-scan-deps tool directly?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 23, 2024

Is it actually necessary to have the em-foo wrapper? Is there some reason we can't just tell CMake the location of the clang-scan-deps tool directly?

The problem with that is that I can't find a way to make clang-scan-deps know about the emscripten include paths in that case. Perhaps there is some way to make it work, but I don't really know how clang-scan-deps works.

@dschuff
Copy link
Member

dschuff commented May 23, 2024

Is it actually necessary to have the em-foo wrapper? Is there some reason we can't just tell CMake the location of the clang-scan-deps tool directly?

The problem with that is that I can't find a way to make clang-scan-deps know about the emscripten include paths in that case. Perhaps there is some way to make it work, but I don't really know how clang-scan-deps works.

I've been wondering for a while if it made sense to just teach clang directly about the emscripten include paths. We already have an emscripten triple. I would imagine clang-scan-deps just uses the same logic, but I also don't really know how clang-scan-deps works.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 23, 2024

Is it actually necessary to have the em-foo wrapper? Is there some reason we can't just tell CMake the location of the clang-scan-deps tool directly?

The problem with that is that I can't find a way to make clang-scan-deps know about the emscripten include paths in that case. Perhaps there is some way to make it work, but I don't really know how clang-scan-deps works.

I've been wondering for a while if it made sense to just teach clang directly about the emscripten include paths. We already have an emscripten triple. I would imagine clang-scan-deps just uses the same logic, but I also don't really know how clang-scan-deps works.

But how would clang know where the emscripten sysroot is without being told like this in the wrapper?

@dschuff
Copy link
Member

dschuff commented May 23, 2024

But how would clang know where the emscripten sysroot is without being told like this in the wrapper?

I think it might make sense for CMake to pass the sysroot and triple flags. This solution of course wouldn't generalize to other build systems, and we might want to keep passing those in emcc, but I think CMake still passes those flags in some other contexts so it wouldn't be too weird. That might be enough to make scan-deps work.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 23, 2024

But how would clang know where the emscripten sysroot is without being told like this in the wrapper?

I think it might make sense for CMake to pass the sysroot and triple flags. This solution of course wouldn't generalize to other build systems, and we might want to keep passing those in emcc, but I think CMake still passes those flags in some other contexts so it wouldn't be too weird. That might be enough to make scan-deps work.

I don't think cmake has enough information to figure that stuff out. It doesn't know where the sysroot it is. It could probably run emcc to figure that out? But also I'm not sure it is able to pass cflags only to scan deps do it would end up passing them everywhere.. which would be redundant. emcc doesn't need a sysroot flag so why both to calculate one and pass it in?

Ultimately I think we will need to figure out a better / deeper way to integrate with scan-deps, but this approach seems like a reasonable step one.

@dschuff
Copy link
Member

dschuff commented May 23, 2024

Actually I meant having CMake have the information about the sysroot in our toolchain file (since that already has alll the special knowledge about emscripten/emsdk) do it for both clang and scan-deps. Then if we wanted to, we could remove that from emcc and/or be one step closer to not needing emcc at all at compile time. But yeah in the near term it would be redundant. I'm kind of ok with this as a short-term solution, mostly I'm just noting that it goes the opposite direction we want to go in long term.

@connorjclark
Copy link
Contributor

FYI the "Fixes" github reference is wrong, it should probably be #21866

Any chance this could land as a temporary fix for newer cmake? :)

@jpvanoosten
Copy link

jpvanoosten commented Jul 25, 2024

I'm also hoping to see this short-term fix get merged soon ;)

Thanks for the effort so far @sbc100 and @dschuff and @kripken !

@jpvanoosten
Copy link

Any news about this?

@leandro-benedet-garcia
Copy link

Hello, just wanted to know if there is anything preventing this from getting merged.

@dsamo
Copy link

dsamo commented Oct 25, 2024

Hello, will there be any progress on this?

@eliemichel
Copy link

While waiting for this to land, here is a template repo that imports the bits of this PR locally so that one can build C++20 without the need to upgrade emscripten: https://github.com/eliemichel/cpp20-cmake-emscripten-template

barne856 added a commit to barne856/squint that referenced this pull request Oct 27, 2024
@sbc100 sbc100 closed this Jan 3, 2025
@sbc100 sbc100 deleted the scan_deps branch January 3, 2025 22:45
@dschuff
Copy link
Member

dschuff commented Jan 4, 2025

@sbc100 I think we still want this. IIRC it appeared to be working but there was a failure in the CMake tests that we never debugged. This has been an issue for a long time, I think we should fix it soon.

@sbc100 sbc100 restored the scan_deps branch January 4, 2025 00:03
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 4, 2025

Sorry, I did an accidental mass-closing. Re-opening them all now.

@sbc100 sbc100 reopened this Jan 4, 2025
@sbc100 sbc100 force-pushed the scan_deps branch 2 times, most recently from 39b6249 to 70281b2 Compare January 29, 2025 23:11
@sbc100 sbc100 changed the title Add emscan-deps tool Add emscan-deps tool to enable C++20 under cmake Jan 29, 2025
@sbc100 sbc100 changed the title Add emscan-deps tool to enable C++20 under cmake [cmake] Add emscan-deps tool to enable C++20 Jan 29, 2025
@sbc100 sbc100 force-pushed the scan_deps branch 5 times, most recently from 55ed1dc to a1b7e1e Compare January 30, 2025 17:15
This is wrapper around clang-scan-deps.  Currently this is just enough
to make C++20 work under cmake.  We don't currently have any actaully
tests of C++20 modules.

See: emscripten-core#21042
Fixes: emscripten-core#22305
@sbc100 sbc100 enabled auto-merge (squash) January 30, 2025 17:23
@sbc100 sbc100 disabled auto-merge January 30, 2025 17:59
@sbc100 sbc100 enabled auto-merge (squash) January 30, 2025 17:59
@sbc100 sbc100 merged commit 209b704 into emscripten-core:main Jan 30, 2025
29 checks passed
@sbc100 sbc100 deleted the scan_deps branch January 30, 2025 18:35
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.

import C++20 module but clang-scan-deps doen't known the "-s USE_WEBGPU=1" options.
8 participants