-
Notifications
You must be signed in to change notification settings - Fork 109
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
Emscripten toolchain should point to cache sys root #133
Comments
Ah, yes, thanks for the reminder. There's still the problem with headers/libraries of 3rd party projects installed into Emscripten sysroot getting stale that I didn't have time to investigate further, which is why I didn't merge this patch yet. But, just yesterday, while working on the cursed issues from mosra/magnum#560 I was browsing Emscripten changelog and found this in 2.0.24:
Yes, we're not using the upstream toolchain, so it doesn't affect us, but what I consider important about this note is that upstream treats installing into Emscripten's own sysroot as the recommended practice. Which means that either they should hopefully know about this potential "stale" issue and having a builtin solution for it (that I'm not aware of yet), or the toolchain handles that somehow (and because we don't use it, we don't have that handled), or there might at least be some issues open about this. I'll try to look back into this when I get time. |
After applying this change, you might run into issues with implicit C headers being included by the C++ compiler. Those headers are not in system/include, but they are in cache/sysroot/include. The fix can be found in emscripten-core/emscripten#17137, the important part: if("${CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES}" STREQUAL "")
set(CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES "${EMSCRIPTEN_SYSROOT}/include")
endif()
if("${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES}" STREQUAL "")
set(CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES "${EMSCRIPTEN_SYSROOT}/include")
endif() This makes sure that explicitly adding the cache sysroot include doesn't have preference over the implicit C++ include directory. |
Closing as resolved, forgot this issue was even here. Just for the record, the first patch was integrated with various version-dependent modification as mosra/toolchains@d5d7430, and the toolchains submodule was updated in 45282f8. The second patch seems to be not needed -- looking into CMake's This is for example with CMake 3.17 and Emscripten 2.0.25 on the CI, the sysroot include dir is last: set(CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES "/emsdk/upstream/emscripten/cache/sysroot/include/SDL;/emsdk/upstream/emscripten/cache/sysroot/include/compat;/emsdk/upstream/emscripten/cache/sysroot/include/c++/v1;/emsdk/upstream/lib/clang/13.0.0/include;/emsdk/upstream/emscripten/cache/sysroot/include") |
Eh, in the end I actually did hit a similar problem, although in my case I had to add the non-cache include dir there: mosra/toolchains@6a9e082 Because FindZstd installed into the non-cache location (because otherwise it wouldn't be able to locate its libraries, because THE DAMN THING copies only contents of include/ but not I hate everything about this platform. Everything. |
I mentioned this on gitter a while back but these things tend to disappear in the mist of time, so here's a trackable issue 🍉
Corrade's Emscripten toolchain file sets
CMAKE_FIND_ROOT_PATH
toupstream/emscripten/system
. In contrast, Emscripten's own toolchain file sets it to the cache sys root (defaults toupstream/emscripten/cache/sysroot
but is configurable). The latter contains generated headers likeversion.h
added in 3.1.4 and included directly byemscripten.h
.This breaks when you link to a library whose include path is the root include directory (like EGL) and then directly include emscripten.h. In that case, the Corrade
CMAKE_FIND_ROOT_PATH
has priority over the compiler's default include path, and whileemscripten.h
is found there, compilation fails because it now can't findversion.h
.Repro: add
#include <emscripten.h>
toTextureGLTest.cpp
on Emscripten 3.1.4 and up.There's a rather lengthy gitter thread with more detail here.
Edit: forgot to add the patch we've been using, been working fine so far:
The text was updated successfully, but these errors were encountered: