sccn / liblsl

C++ lsl library for multi-modal time-synched data transmission over the local network
Other
108 stars 63 forks source link

Allow to use system Boost #114

Closed chausner closed 2 years ago

chausner commented 3 years ago

I experimented with updating the code to work with the latest version of Boost (1.75) and also allowing to use the system Boost installation instead of the bundled one. The latter is the recommended approach for package managers like vcpkg and Conan which have their own Boost packages. My intent is to first make the LSL-side more flexible before attempting to create a Conan recipe and improving the existing vcpkg package.

In Boost 1.70 there have been breaking changes in the Asio library which lead to compilation issues in cancellable_streambuf.h. I was able to make it compile with some hacks but I haven't tested correctness yet. In any case, the way I solved it is most likely not right and I would love to get some hints from someone who knows the API better.

I saw that the bundled Boost distribution uses a patch on Windows related to timer delays. With an external Boost distribution, this patch would obviously not be included. I am wondering whether it is critical or could maybe even be proposed for inclusion in Boost itself?

Let me know what you think.

tstenner commented 3 years ago

I experimented with updating the code to work with the latest version of Boost (1.75) and also allowing to use the system Boost installation instead of the bundled one. The latter is the recommended approach for package managers like vcpkg and Conan which have their own Boost packages. My intent is to first make the LSL-side more flexible before attempting to create a Conan recipe and improving the existing vcpkg package.

Much appreciated :+1:

One of the reasons we're bundling boost (or rather, lslboost) is that some programs, most notably Matlab, also link to various versions of Boost and that leads to all sorts of hard to debug issues. Newer versions of Boost.System are header only and I have replaced most other usages of Boost with the C++11 stdlib. The only remaining part is #103, but that's still experimental.

I saw that the bundled Boost distribution uses a patch on Windows related to timer delays. With an external Boost distribution, this patch would obviously not be included. I am wondering whether it is critical or could maybe even be proposed for inclusion in Boost itself?

In the past, Boost has also needed some patches, e.g. so sleep(.001) isn't automatically translated to sleep(.03), but as part of the move to C++11 that isn't needed any more.

In Boost 1.70 there have been breaking changes in the Asio library which lead to compilation issues in cancellable_streambuf.h. I was able to make it compile with some hacks but I haven't tested correctness yet. In any case, the way I solved it is most likely not right and I would love to get some hints from someone who knows the API better.

I'm not a chef on TV, but I've always wanted to say that I've prepared something: #99 has some more fixes than your PR, but it needs some more testing in the real world.

chausner commented 3 years ago

Great to hear you are already working on some of the things I tried. I guess I should have checked the existing issues more thoroughly first. :) In this case, I'll wait until the work you've started is merged and will then have a look again at the remaining issues.

tstenner commented 2 years ago

I have merged the Asio changes, so if you could rebase your PR I can take a look at it. With my recent changes, the number of accidentally exported Boost symbols is down from several hundreds to a handful from Boost.Archive and Boost.Thread, so in the near future it should be possible to just use a (recent) system install of boost.

chausner commented 2 years ago

I have merged the Asio changes, so if you could rebase your PR I can take a look at it.

I have rebased onto master.

tstenner commented 2 years ago

After the change to plain Asio was merged, there was only Boost.Serialization left. With the new unit tests, the replacement code for the subset of Boost.Serialization we're actively using is already better tested on x64 than the old code, so I'm confident that it's an alternative to the real Boost.Serialization (especially since your PR builds against new versions by default). After enabling this by default with system boost, your PR is a bit shorter (https://github.com/tstenner/liblsl/commit/9081e72fa0ab86498bfa56d38534190f030efb97):

@@ -27,2 +27,3 @@ option(LSL_OPTIMIZATIONS "Enable some more compiler optimizations" ON)
 option(LSL_UNITTESTS "Build LSL library unit tests" OFF)
+option(LSL_BUNDLED_BOOST "Use the bundled Boost by default" ON)
 option(LSL_BUNDLED_PUGIXML "Use the bundled pugixml by default" ON)
@@ -167,22 +168,30 @@ find_package(Threads REQUIRED)
 # create the lslboost target
-add_library(lslboost OBJECT
-       lslboost/serialization_objects.cpp
-)
-target_link_libraries(lslboost PUBLIC Threads::Threads)
+add_library(lslboost INTERFACE)
+if(LSL_BUNDLED_BOOST)
+       message(STATUS "Using bundled Boost")
+       target_include_directories(lslboost SYSTEM INTERFACE
+               $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/lslboost>)
+else()
+       message(STATUS "Using system Boost")
+       find_package(Boost REQUIRED)
+       target_compile_definitions(lslboost
+               INTERFACE
+                       lslboost=boost # allows the LSL code base to work with the original Boost namespace/headers
+       )
+       target_link_libraries(lslboost INTERFACE Boost::boost Boost::disable_autolinking)
+endif()

-if(LSL_SLIMARCHIVE)
+if(LSL_SLIMARCHIVE OR NOT LSL_BUNDLED_BOOST)
        message(STATUS "Using shim instead of full Boost.Archive")
-       target_compile_definitions(lslboost PUBLIC SLIMARCHIVE)
+       target_compile_definitions(lslboost INTERFACE SLIMARCHIVE)
+elseif(LSL_BUNDLED_BOOST)
+       # compile Boost.Serialization objects as part of lslobj
+       target_sources(lslobj PRIVATE lslboost/serialization_objects.cpp)
 endif()
-target_compile_features(lslboost PUBLIC cxx_std_11 cxx_lambda_init_captures)

-target_compile_definitions(lslboost
-       PUBLIC
-               BOOST_ALL_NO_LIB
-)
-target_include_directories(lslboost SYSTEM PUBLIC
-       $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/lslboost>)
+target_compile_definitions(lslboost INTERFACE BOOST_ALL_NO_LIB)

 # target configuration for the internal lslobj target
-target_link_libraries(lslobj PRIVATE lslboost)
+target_link_libraries(lslobj PRIVATE lslboost Threads::Threads)
+target_compile_features(lslobj PUBLIC cxx_std_11 cxx_lambda_init_captures)
 target_include_directories(lslobj
chausner commented 2 years ago

After enabling this by default with system boost, your PR is a bit shorter (tstenner@9081e72):

Thanks! I updated the PR to match your suggested changes.

tstenner commented 2 years ago

Thanks for the PR and your patience :+1:

The CI is happy, but I had to rebase and push the PR by hand (a9a06c78d4218b76e1c906c04452eec697f02f8e).

chausner commented 2 years ago

@tstenner

I have an updated vcpkg port ready and also a Conan recipe: https://github.com/microsoft/vcpkg/pull/21284 https://github.com/conan-io/conan-center-index/pull/8435

Neither of them has been reviewed yet but I expect them to pass review quickly. As soon as we have a new stable release of liblsl, I can update/publish the ports.