Use cmake standard BUILD_SHARED_LIBS setting#8764
Conversation
sbc100
requested review from
a team,
kripken and
stevenfontanella
and removed request for
a team
| if(BUILD_STATIC_LIB) | ||
| message(STATUS "Building libbinaryen as statically linked library.") | ||
| add_library(binaryen STATIC) | ||
| add_definitions(-DBUILD_STATIC_LIBRARY) |
There was a problem hiding this comment.
Will this code still work without this def?
Line 61 in 505dae4
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Hmm, that could be. Should we remove it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just to be clear the __declspec was never activated before this change also
There was a problem hiding this comment.
Oh, sorry - if that was there before then I just missed it somehow. lgtm!
See https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html I'm not sure why we made up our own setting here rather than using the standard one.
| if(BUILD_STATIC_LIB) | ||
| message(STATUS "Building libbinaryen as statically linked library.") | ||
| add_library(binaryen STATIC) | ||
| add_definitions(-DBUILD_STATIC_LIBRARY) |
There was a problem hiding this comment.
Oh, sorry - if that was there before then I just missed it somehow. lgtm!
2 participants
See https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html
I'm not sure why we made up our own setting here rather than using the standard one.