Skip to content

Exclude the mimalloc files from the Binaryen install #7386

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

Merged
merged 1 commit into from
Mar 20, 2025
Merged

Exclude the mimalloc files from the Binaryen install #7386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ if(BUILD_LLVM_DWARF)
endif()

if(MIMALLOC_STATIC)
if(NOT(LINUX AND BUILD_STATIC_LIB) OR EMSCRIPTEN)
if(NOT(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND BUILD_STATIC_LIB) OR EMSCRIPTEN)
Copy link
Member Author

@dschuff dschuff Mar 20, 2025

Choose a reason for hiding this comment

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

When using the emscripten-releases build script I got a very strange behavior; namely that the LINUX variable was empty, but CMAKE_SYSTEM_NAME was set to Linux as expected. I couldn't figure out what was causing it. This change works around that.

Copy link
Member Author

@dschuff dschuff Mar 20, 2025

Choose a reason for hiding this comment

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

OK, I figure it out; the LINUX variable was introduced in CMake 3.25 but we support 3.16 and use 3.21 on our bots. This is the right way to do it on those versions, so I'll just update the commit message.

message(FATAL_ERROR "Statically linking mimalloc is only supported when building as a native, statically linked library on Linux.")
endif()
message(STATUS "Building with statically linked mimalloc allocator.")
Expand Down
2 changes: 1 addition & 1 deletion third_party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,5 @@ if(MIMALLOC_STATIC)
# (They can still be enabled via MIMALLOC_VERBOSE=1 wasm-opt ...)
add_compile_definitions(MI_DEBUG=0)

add_subdirectory(mimalloc)
add_subdirectory(mimalloc EXCLUDE_FROM_ALL)
Copy link
Member

@tlively tlively Mar 20, 2025

Choose a reason for hiding this comment

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

From briefly looking up EXCLUDE_FROM_ALL, it seems that it means that this directory won't be built by default? Is that what we want, or is there a more specific way to exclude it from being installed?

Copy link
Member Author

@dschuff dschuff Mar 20, 2025

Choose a reason for hiding this comment

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

Right, I think what's happening here is that the target_link_libraries(binaryen mimalloc-static) causes a dependence from libbinaryen on libmimalloc-static which causes it to be built when libbinaryen does. And then the tools link it in, but it doesn't get installed, which is exactly what we want.

Copy link
Member Author

@dschuff dschuff Mar 20, 2025

Choose a reason for hiding this comment

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

I couldn't really find another way to prevent the installation (see also https://discourse.cmake.org/t/hot-to-make-add-subdirectory-prevent-install-but-still-build-as-part-of-all/8698)

endif()
Loading