Skip to content

Build libbinaryen as a monolithic statically/shared library#2463

Merged
kripken merged 2 commits intoWebAssembly:masterfrom
ImmanuelHaffner:libbinaryen
Nov 22, 2019
Merged

Build libbinaryen as a monolithic statically/shared library#2463
kripken merged 2 commits intoWebAssembly:masterfrom
ImmanuelHaffner:libbinaryen

Conversation

@ImmanuelHaffner
Copy link
Contributor

@ImmanuelHaffner ImmanuelHaffner commented Nov 22, 2019

This PR addressess issue #2450

So far, the CMake configuration did not produce a standalone statically linked or shared library of libbinaryen. The command

TARGET_LINK_LIBRARIES(binaryen passes wasm asmjs emscripten-optimizer ir cfg support)

that was intended to provide this library actually just told CMake that every target, to which the library binaryen is linked, the other libraries (passes, wasm, ...) must also be linked. This works fine if you include this repository with CMake add_subdirectory but fails to provide a standalone library that can be used by third-parties.

This PR solves this problem by constructing the libraries of the subdirectories (passes, wasm, ...) as OBJECT libraries. From the CMake documentation: "An object library compiles source files but does not archive or link their object files into a library. Instead other targets created by add_library() or add_executable() may reference the objects [...]".
This modification prevents the archiving/linking of the object files of subdirectories and enables linking binaryen as a standalone library. (One important observation here, that was totally not obvious for me, is that CMake simply does not link multiple static libraries into a single static library, for portability reasons IIRC. See this informative answer on the CMake mailing list: https://cmake.org/pipermail/cmake/2018-September/068263.html )

Further, declare the libraries as PRIVATEly linked to binaryen, because we do not want their symbols to leak through the statically linked or shared library. CMake documentation says: "Libraries and targets following PRIVATE are linked to, but are not made part of the link interface."

According to CMake documentation: "Libraries and targets following
PRIVATE are linked to, but are not made part of the link interface."
This is exactly what we want, as we only want the C API to be part of
the interface.
@kripken
Copy link
Member

kripken commented Nov 22, 2019

Thanks @ImmanuelHaffner !

This is beyond my understanding of CMake, so hopefully someone more knowledgeable can review. However, given this works for you, and passes CI here, it looks good to me.

Except for one issue - making those PRIVATE means that only the binaryen C API is exported, if I understand correctly. That means libbinaryen is no longer useful for projects that want to use the internal C++ API surface. I'm not sure if we have such users or not.

@tlively
Copy link
Member

tlively commented Nov 22, 2019

I'm no CMake expert, but this looks just fine to me as well. If we do have users of our C++ interface, they are almost certainly C++ projects themselves, in which case they can just add Binaryen to their build. So making the C++ API private sounds good to me. It doesn't have any stability guarantees anyway.

@kripken
Copy link
Member

kripken commented Nov 22, 2019

Sounds good, merging!

@kripken kripken merged commit bf8f36c into WebAssembly:master Nov 22, 2019
@kripken
Copy link
Member

kripken commented Nov 23, 2019

This seems to hit a problem on the mac build bot, which runs after merging,

https://travis-ci.org/WebAssembly/binaryen/jobs/615772482?utm_medium=notification&utm_source=github_status

CMake Error at src/support/CMakeLists.txt:12 (TARGET_LINK_LIBRARIES):
  Object library target "support" may not link to anything.

That does look like a cmake-related error, so it looks like a regression from this?

@ImmanuelHaffner We can revert this, and investigate, if there isn't an obvious quick fix here.

@ImmanuelHaffner
Copy link
Contributor Author

There may be a quick fix. The official Cmake Syntax for linking object libraries is described as: "Instead other targets created by add_library() or add_executable() may reference the objects using an expression of the form $<TARGET_OBJECTS:objlib> as a source, where objlib is the object library name."
Since I did not need that Syntax to make it work on my Linux machine as well as on the Mac OS Catalina of my colleague, I chose to be lazy here and stick with the shorter Syntax.
Maybe using the official cmake Syntax fixes this issue.
I can look into that on Monday when I am back in office. Currently I am stuck at a place with very poor internet access :/

@ImmanuelHaffner
Copy link
Contributor Author

@kripken I opened a new issue #2471 for the problems on the mac build bot and already addressed it in a PR #2472

@kripken
Copy link
Member

kripken commented Nov 26, 2019

There is progress in #2472, but as we don't have a fix yet, reverting for now while we continue the investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants