Skip to content

Use cmake standard BUILD_SHARED_LIBS setting #8764

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

Open
sbc100 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use cmake standard BUILD_SHARED_LIBS setting #8764

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
16 changes: 8 additions & 8 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ if(EMSCRIPTEN)
set(BUILD_LLVM_DWARF OFF)
endif()

option(BUILD_STATIC_LIB "Build as a static library" OFF)
option(BUILD_SHARED_LIBS "Build using shared libraries" ON)
if(MSVC OR EMSCRIPTEN)
# We don't have dllexport declarations set up for Windows yet.
# With emscripten we require a static library to create binaryen_js correctly.
set(BUILD_STATIC_LIB ON)
set(BUILD_SHARED_LIBS OFF)
endif()

# Advised to turn on when statically linking against musl libc (e.g., in the
Expand Down Expand Up @@ -451,18 +451,18 @@ else() # MSVC
endif()

# Declare libbinaryen
# This will be either be STATIC or SHARED depending on BUILD_SHARED_LIBS
add_library(binaryen)

if(BUILD_STATIC_LIB)
message(STATUS "Building libbinaryen as statically linked library.")
add_library(binaryen STATIC)
add_definitions(-DBUILD_STATIC_LIBRARY)
Copy link
Copy Markdown
Member

@kripken kripken May 22, 2026

Choose a reason for hiding this comment

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

Will this code still work without this def?

binaryen/src/binaryen-c.h

Line 61 in 505dae4

#elif defined(_MSC_VER) && !defined(BUILD_STATIC_LIBRARY)

Copy link
Copy Markdown
Member Author

@sbc100 sbc100 May 22, 2026

Choose a reason for hiding this comment

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

We don't currently support building shared library under MSVC (we force BUILD_SHARED_LIBS to false) so I guess this code is dead.. but somebody was trying to get it working at some point I guess?

Copy link
Copy Markdown
Member

@kripken kripken May 22, 2026

Choose a reason for hiding this comment

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

Hmm, that could be. Should we remove it?

Copy link
Copy Markdown
Member Author

@sbc100 sbc100 May 22, 2026

Choose a reason for hiding this comment

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

It looks like it could be useful as the first step in making a windows DLL work. It seems reasonable to leave in in place for when somebody needs it.

Copy link
Copy Markdown
Member

@kripken kripken May 22, 2026

Choose a reason for hiding this comment

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

Ok, maybe add a comment there that we do not currently define that value? (which is the case as of this PR). That is, add a TODO there to define and use it.

Copy link
Copy Markdown
Member

@kripken kripken May 22, 2026

Choose a reason for hiding this comment

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

Yes, I meant in src/binaryen-c.h.

Otherwise it is looking at a C define that is not defined anywhere, which seems confusing? It looks like a bug rather than a TODO.

Copy link
Copy Markdown
Member Author

@sbc100 sbc100 May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I did updated binaryen-c.h in this PR to refer to the new define. Did you see that part?

I didn't change that fact that we currently disable the shared library build on windows, I just flipped the logic around.

Copy link
Copy Markdown
Member Author

@sbc100 sbc100 May 22, 2026

Choose a reason for hiding this comment

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

OK I added a TODO.

Copy link
Copy Markdown
Member Author

@sbc100 sbc100 May 22, 2026

Choose a reason for hiding this comment

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

Just to be clear the __declspec was never activated before this change also

Copy link
Copy Markdown
Member

@kripken kripken May 22, 2026

Choose a reason for hiding this comment

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

Oh, sorry - if that was there before then I just missed it somehow. lgtm!

else()
if(BUILD_SHARED_LIBS)
add_definitions(-DBUILD_SHARED_LIBS)
message(STATUS "Building libbinaryen as shared library.")
add_library(binaryen SHARED)
if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
# Disable interposition and resolve Binaryen symbols locally.
add_link_flag("-Bsymbolic")
endif()
else()
message(STATUS "Building libbinaryen as statically linked library.")
endif()
target_link_libraries(binaryen PUBLIC Threads::Threads)
binaryen_setup_rpath(binaryen)
Expand Down
4 changes: 3 additions & 1 deletion src/binaryen-c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@
#if defined(__EMSCRIPTEN__)
#include <emscripten.h>
#define BINARYEN_API EMSCRIPTEN_KEEPALIVE
#elif defined(_MSC_VER) && !defined(BUILD_STATIC_LIBRARY)
#elif defined(_MSC_VER) && defined(BUILD_SHARED_LIBS)
// TODO: This is not yet used since we disabled BUILD_SHARED_LIBS under
// _MSC_VER in CMakeLists.txt
#define BINARYEN_API __declspec(dllexport)
#else
#define BINARYEN_API
Expand Down
Loading