Skip to content

CMake Toolkit: Shared Library as Side Module #16281

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.

Sign up for GitHub

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

Closed
ax3l wants to merge 9 commits into from
Closed

CMake Toolkit: Shared Library as Side Module #16281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3ff2906
CMake Toolkit: Shared Library as Side Module
ax3l Feb 14, 2022
82733b2
Update cmake/Modules/Platform/Emscripten.cmake
ax3l Feb 15, 2022
d3aa7f3
Update Tests
ax3l Feb 15, 2022
4f103cb
`-s SIDE_MODULE=1` -> `-sSIDE_MODULE`
ax3l Feb 16, 2022
23cb03b
other.test_cmake_* / cpp_lib: update defaults (keep static)
ax3l Feb 16, 2022
c442b91
Set CMAKE_STRIP
ax3l Feb 16, 2022
e49a8dc
fix: extra )
ax3l Mar 4, 2022
2296976
CMAKE_STRIP: Use any `llvm-strip`
ax3l Mar 4, 2022
afb0439
CMAKE_STRIP: Use `emstrip`
ax3l Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion cmake/Modules/Platform/Emscripten.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ set(CMAKE_SYSTEM_NAME Emscripten)
set(CMAKE_SYSTEM_VERSION 1)

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

# shared library (side module) support
set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS TRUE)
set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS "-sSIDE_MODULE") # instead of -shared
set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS "-sSIDE_MODULE") # instead of -shared
set(BUILD_SHARED_LIBS OFF) # default to static libs, even if we allow shared ones

@ax3l ax3l Feb 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

not really needed, default is also static.

Suggested change
set(BUILD_SHARED_LIBS OFF) # default to static libs, even if we allow shared ones

@nokotan nokotan Mar 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Our goal should be this table.

cmake user option library type
add_library(foo) static
add_library(foo) -DBUILD_SHARED_LIBS=ON dynamic
add_library(foo SHARED) dynamic
add_library(foo) option(BUILD_SHARED_LIBS "" ON) static
add_library(foo) option(BUILD_SHARED_LIBS "" ON) -DBUILD_SHARED_LIBS=ON dynamic

The most important thing will be that option(BUILD_SHARED_LIBS "" ON) should be overridden to OFF for compatibility unless BUILD_SHARED_LIBS=ON is explicitly specified.

Just adding configure=['cmake', '.', '-DBUILD_SHARED_LIBS=OFF'] will be sufficient for this and set(BUILD_SHARED_LIBS OFF) can be deleted.
The other solution will be that caching BUILD_SHARED_LIBS, this will overwrite option(BUILD_SHARED_LIBS "" ON).

Suggested change
set(BUILD_SHARED_LIBS OFF) # default to static libs, even if we allow shared ones
set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build WebAssembly dynamic libraries. Default value is 'OFF'.") # default to static libs, even if we allow shared ones

@ax3l ax3l Mar 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @nokotan,

what you write in this table is very reasonable, but there is a misunderstanding on your side how CMake works.

Yes, add_library defaults will be based on the BUILD_SHARED_LIBS default. That one is static now.

No, if a user sets either set(BUILD_SHARED_LIBS ON) or option(BUILD_SHARED_LIBS "" ON) this will be overwritten. This is a clear user decision then (in their CMakeLists.txt) and we cannot overwrite this without disabling shared library support altogether (which was done prior to this PR). In other worlds: the 2nd last row in your table is not possible.
All other rows are exactly with this PR as you also wrote.

I hope this clarifies a bit, please let me know if you have more questions.

@nokotan nokotan Mar 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The question remains, option(BUILD_SHARED_LIBS "" ON) seems to do nothing when BUILD_SHARED_LIBS is set by user or is already cached, after CMake 3.13. (CMP0077)

@ax3l ax3l Apr 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That is exactly how all cached variables and options work in CMake.

Yes, user CLI parameters, toolchain files and cache files can be used by users to overwrite the default of an option() - that is fine and how all CMake options should work. If we would mingle with that behavior, then we would break a lot of package management software and stray from community standards.


# Advertise Emscripten as a 32-bit platform (as opposed to
# CMAKE_SYSTEM_PROCESSOR=x86_64 for 64-bit platform), since some projects (e.g.
Expand Down Expand Up @@ -102,6 +107,10 @@ if ("${CMAKE_RANLIB}" STREQUAL "")
set(CMAKE_RANLIB "${EMSCRIPTEN_ROOT_PATH}/emranlib${EMCC_SUFFIX}" CACHE FILEPATH "Emscripten ranlib")
endif()

if ("${CMAKE_STRIP}" STREQUAL "")
set(CMAKE_STRIP "${EMSCRIPTEN_ROOT_PATH}/emstrip${EMCC_SUFFIX}" CACHE FILEPATH "Emscripten strip")
endif()

if ("${CMAKE_C_COMPILER_AR}" STREQUAL "")
set(CMAKE_C_COMPILER_AR "${CMAKE_AR}" CACHE FILEPATH "Emscripten ar")
endif()
Expand Down
2 changes: 1 addition & 1 deletion tests/cmake/cpp_lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.0)

project(cpp_library)

set(CPP_LIBRARY_TYPE "SHARED" CACHE STRING "Library type to build")
set(CPP_LIBRARY_TYPE "STATIC" CACHE STRING "Library type to build")
set_property(CACHE CPP_LIBRARY_TYPE PROPERTY STRINGS STATIC SHARED)

add_library(cpp_lib ${CPP_LIBRARY_TYPE} lib.cpp)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6644,7 +6644,7 @@ def line_splitter(data):
Path('codec/CMakeFiles/j2k_to_image.dir/convert.c.o'),
Path('codec/CMakeFiles/j2k_to_image.dir/__/common/color.c.o'),
Path('bin/libopenjpeg.a')],
configure=['cmake', '.'],
configure=['cmake', '.', '-DBUILD_SHARED_LIBS=OFF'],

@sbc100 sbc100 Feb 16, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this needed if the default is set of OFF above?

@ax3l ax3l Feb 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I wonder as well. Without it the test_openjpeg does not seem to build

libopenjpeg/CMakeFiles/openjpeg.dir/build.make:388: bin/libopenjpeg.so.1.4.0

Is the toolchain file even used here?

@ax3l ax3l Feb 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah, it's because ./tests/third_party/openjpeg/CMakeLists.txt lists:

# OpenJPEG build configuration options.
OPTION(BUILD_SHARED_LIBS "Build OpenJPEG shared library and link executables against it." ON)

Before this change, this was force-changed to static (in add_library) by the global property TARGET_SUPPORTS_SHARED_LIBS FALSE.

@sbc100 sbc100 Feb 16, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see.. I'm ok with the explicit extra -DBUILD_SHARED_LIBS=OFF here then . And yes the emcmake toolchain is used for all our cmake builds.

# configure_args=['--enable-tiff=no', '--enable-jp3d=no', '--enable-png=no'],
make_args=[]) # no -j 2, since parallel builds can fail

Expand Down
2 changes: 1 addition & 1 deletion tests/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ def test_emsize(self):
'js': ('target_js', 'test_cmake.js', ['-DCMAKE_BUILD_TYPE=Debug']),
'html': ('target_html', 'hello_world_gles.html', ['-DCMAKE_BUILD_TYPE=Release']),
'library': ('target_library', 'libtest_cmake.a', ['-DCMAKE_BUILD_TYPE=MinSizeRel']),
'static_cpp': ('target_library', 'libtest_cmake.a', ['-DCMAKE_BUILD_TYPE=RelWithDebInfo', '-DCPP_LIBRARY_TYPE=STATIC']),
'shared_cpp': ('target_library', 'libtest_cmake.so', ['-DCMAKE_BUILD_TYPE=RelWithDebInfo', '-DBUILD_SHARED_LIBS=ON', '-DCPP_LIBRARY_TYPE=SHARED']),
'stdproperty': ('stdproperty', 'helloworld.js', []),
'post_build': ('post_build', 'hello.js', []),
})
Expand Down