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

Enable normal CMake EMSCRIPTEN detection #7

Closed
wants to merge 12 commits into from

Conversation

Projects
None yet
2 participants
@isc30
Copy link
Contributor

commented Jun 9, 2019

The default EMSCRIPTEN detection is based on a global EMSCRIPTEN variable. If we use these toolchains, this variable won't be set making crosscompilation fail

isc30 added some commits Jun 9, 2019

Enable normal CMake EMSCRIPTEN detection
The default EMSCRIPTEN detection is based on a global `EMSCRIPTEN` variable. If we use these toolchains, this variable won't be set making crosscompilation fail

isc30 added a commit to isc30/toolchains that referenced this pull request Jun 9, 2019

isc30 added some commits Jun 9, 2019

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 17, 2019

The default EMSCRIPTEN detection is based on a global EMSCRIPTEN variable.

Who said that? :) The check that never fails is if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten"), or "Android", or ... and so on. Are there projects depending on EMSCRIPTEN being set? (On a related note, I have to #undef ANDROID in C++ code because it breaks things.) For a consistent, reliable and well-documented way of detecting the system in Magnum-based projects, you can use the CORRADE_TARGET_* variables in both CMake and C++.

Show resolved Hide resolved generic/Emscripten-wasm.cmake Outdated
set(CMAKE_SYSTEM_VERSION 1)

set(CMAKE_CROSSCOMPILING TRUE)
set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS FALSE)

This comment has been minimized.

Copy link
@mosra

mosra Jun 17, 2019

Owner

This is not true. You can have shared libs on Emscripten.

This comment has been minimized.

Copy link
@isc30

isc30 Jun 17, 2019

Author Contributor

This was extracted from the official emscripten platform toolchain.
I added a comment below explaining this

Show resolved Hide resolved generic/Emscripten-wasm.cmake Outdated
Show resolved Hide resolved generic/Emscripten-wasm.cmake Outdated
@isc30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

So, these changes were taken from the official emscripten toolchain which is really confusing because it looks outdated.

Even if it is outdated, many people are using it and relying on EMSCRIPTEN global variable (set in here) to detect crosscompilation.

I know this isn't an issue with these toolchains specifically but it would be nice to simply set EMSCRIPTEN here so it remains compatible with other external libraries without any modifications.

Edit: here is the line that sets false to shared libraries. I have no idea why they would do that TBH

@isc30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

For a consistent, reliable and well-documented way of detecting the system in Magnum-based projects, you can use the CORRADE_TARGET_* variables in both CMake and C++

The downside of it is that you cannot use it (or at least I wasn't able) before including corrade CMake.
This is the actual piece of code that hardcodes some configuration depending on the compiler

isc30 added some commits Jun 17, 2019

@isc30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Okay, I just left the important part (the EMSCRIPTEN variable) since the other changes weren't relevant

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2019

Merged as e33a22f (I added a FORCE on top). Thanks a lot!

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2019

The downside of it is that you cannot use it (or at least I wasn't able) before including corrade CMake.

Yeah, that's a downside -- you need to find_package(Corrade REQUIRED) before that. However, I still think that's worth it -- for Emscripten in particular, both if(UNIX) and if(EMSCRIPTEN) will be true and you need to be either extra careful with ordering the branches or you need to remember to do extra things like if(UNIX AND NOT EMSCRIPTEN). Emscripten tries to be Unix, but if you care about executable sizes, you definitely don't want to treat it as Unix -- as you have seen in the latest blogpost anyway :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.