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

Better support for CMake: TestBigEndian, CheckTypeSize, CMAKE_CROSSCOMPILING_EMULATOR #3447

Merged
merged 3 commits into from
May 12, 2015

Conversation

thewtex
Copy link
Contributor

@thewtex thewtex commented May 11, 2015

This overrides two CMake modules that are commonly used by projects, especially scientific projects or projects that aim to support a number of platforms.

The TestBigEndian modules is replaced. This addresses:

#1106

Also the CheckTypeSize module is replaced.

#400

The fix for CheckTypeSize involves the use of the CMake try_run command. Also ease the building of CMake project files by adding support for the CMAKE_CROSSCOMPILING_EMULATOR variable, and pointing it to nodejs. This prevents the need to manually populate CMake TryRunResults.cmake files. It currently requires CMake Git master (to be released as CMake 3.3). Everything should still work with older CMake.

New tests are added to the test suite. All tests passed when

tests/runner.py

Make libraries, especially scientific libraries, use the TEST_BIG_ENDIAN CMake
function to determine the endianness of the target system.  Internally, this
uses 'file(STRINGS' on file compiled for the target system that has static
variables.  Usually, the compiled binary will reveal its endianess, but
emscripten does produces javascript or html instead of binaries.

Override TEST_BIG_ENDIAN and always return 0.
@@ -258,3 +258,14 @@ if ("${CMAKE_GENERATOR}" MATCHES "^Visual Studio.*")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${EMSCRIPTEN_VS_LINKER_FLAGS}" CACHE STRING "")
set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${EMSCRIPTEN_VS_LINKER_FLAGS}" CACHE STRING "")
endif()

if (NOT DEFINED CMAKE_CROSSCOMPILING_EMULATOR)
find_program(node_js_executable NAMES node nodejs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be better to be NAMES nodejs node to search for the more specific name first? See e.g. #3296

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. I'll switch it.

@juj juj added the CMake label May 12, 2015
@juj
Copy link
Collaborator

juj commented May 12, 2015

Thanks, this looks very good! One micro-comment above, but can merge either way. Let me know what you think.

@weitjong
Copy link
Contributor

IMHO, there is an issue with this approach of overriding common CMake modules. For a multi-platform project, these overridden modules work only for Emscripten/HTML5 platform. This will force project maintainer to configure CMake module search path differently between Emscripten platform and other platforms. A better way is to use the original CMake modules and then enhance it with if(DEFINED ENV{EMSCRIPTEN}) or something like that to decide which code path it should take. The original version or the "overridden" version. However, this makes the "enhanced" version of the CMake modules to be dependent on the future CMake upstream changes. But the proposed "overridden" CheckTypeSize module also suffer from this anyway. Just my two cents.

thewtex added 2 commits May 12, 2015 09:57
This variable was added in recent CMake Git master.  It should point to an
executable that acts like an emulator for the target system when
cross-compiling.  For emscripten, this is nodejs.

This executable is used by CMake to run executables in try_run commands.  By
adding support for this variable, it prevents the need to manually populate
the tedious TryRunResults.cmake file.

The emscripten configured path to NodeJS is added to the emcmake command.  In
the Emscripten.cmake file, if CMAKE_CROSSCOMPILING_EMULATOR is not defined
(which is would be if using emcmake), and attempt is made to find nodejs and
use it.
The CMake CheckTypeSize module is used by many projects to get sizes of types
for the target platform. Its standard implementation builds a file that
contains the type size with the try_compile command, then uses file(STRINGS on
the built function to extract the size from the contents of the built binary.

Since emscripten produces javascript and html instead of binaries, the string
is not embedded directly in the 'binary' as expected.  Alternatively, we use a
try_run and return the size of the type in the return value.
@thewtex
Copy link
Contributor Author

thewtex commented May 12, 2015

@weitjong Yes, eventually these fixes should be upstreamed into CMake.

@juj
Copy link
Collaborator

juj commented May 12, 2015

@weitjong: The current approach is to call emcmake cmake, which takes care of passing the correct command line parameters (including -DCMAKE_MODULE_PATH) correctly, so a project maintainer does not need to configure the module paths manually. This does have the limitation that a project cannot override CMAKE_MODULE_PATH with its own custom setting (or if it does, it has to be smart about when to do it, I think). If there is a good way around that, it would be good to know.

I don't think it's feasible to take the approach that Emscripten should not do "system" customizations to CMake, but that development should be done in upstream CMake, since especially in Linux world, people typically have very old CMakes in their systems, 2.8 seems to be quite popular, so it would be a very slow way to move forward. Having emcmake wrap cmake and set up CMAKE_MODULE_PATH has been the least painful path for quick iteration so far.

juj added a commit that referenced this pull request May 12, 2015
Better support for CMake: TestBigEndian, CheckTypeSize, CMAKE_CROSSCOMPILING_EMULATOR
@juj juj merged commit 5ec57b8 into emscripten-core:incoming May 12, 2015
juj added a commit that referenced this pull request May 26, 2015
…of not being able to concatenate str and list.
@thewtex thewtex deleted the cmake-endian branch September 3, 2016 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants