IFL-CAMP / simple

S.I.M.P.L.E. - Smart Intuitive Messaging Platform with Less Effort. A Cross-Platform C++ Library to Exchange Data Across Network.
Mozilla Public License 2.0
22 stars 5 forks source link

Fixing windows shared library issues #32

Closed TheHugeManatee closed 6 years ago

TheHugeManatee commented 6 years ago

This fixes unresolved linker errors for the two static symbols now imported from the shared lib. simple::simple is now also forced shared lib. WINDOWS_EXPORT_ALL_SYMBOLS unfortunately only works for functions but not for static data members. An alternative approach would be to hide the static member a local static variable of the instance() function.

Also adds a method to context_manager to destroy it, fixing #26

This could need a test case but I'm not sure how it would look like. @SalvoVirga can you suggest something?

codecov-io commented 6 years ago

Codecov Report

Merging #32 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #32   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files          56       56           
  Lines        3882     3882           
=======================================
  Hits         3871     3871           
  Misses         11       11
Impacted Files Coverage Δ
include/simple/context_manager.hpp 100% <ø> (ø) :arrow_up:
msgs/include/simple_msgs/float.hpp 100% <100%> (ø) :arrow_up:
msgs/include/simple_msgs/int.hpp 100% <100%> (ø) :arrow_up:
msgs/include/simple_msgs/double.hpp 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3de5d71...637cc69. Read the comment docs.

SalvoVirga commented 6 years ago

@TheHugeManatee did you have the chance to check if the changes actually fix #32 completely?

For a test case, something even smaller than what you wrote on the issue would be ok, but if I understood right that would be only for Windows and with dynamic linking. We could easily give a tag on CMake to the test so that is run only on Windows, but dynamic linking is so annoying for the CI, we would have to copy the dll just for that.

Btw, I see the need for this, but having that public function there is a bit "risky". If simple::ContextManager::destroy() is called at any other point in time then the use case in #32, the application would just hang there, since it will try to destroy the context and that will wait for all the socket currently alive etc... But I guess we just have to document the function carefully.

TheHugeManatee commented 6 years ago

I am using this branch successfully to use simple in a shared context (i.e. using simple AS a shared library INSIDE multiple shared libraries, which would otherwise result in duplicate symbols if linked statically), so this fixes the issues as far as I can tell.

I understand the concern of introducing a kind-of-unsafe destroy() function, but I don't see another way tbh. It is just necessary to control the context lifetime explicitly for dynamic linking, and it's the same problem with the zmq lib itself. A (slightly) safer option would be to wrap the context destruction into an RAII object which lives until the end of main() (but not with static lifetime) to be more exception safe. Maybe something could be done to address this at library level using std::atexit but I am not sure tbh.

What is the problem with dynamic linking with CI? With the changes to the CMakeLists.txt all binaries are output into a common binary directory, thus the tests should be able to find the dlls as well?

SalvoVirga commented 6 years ago

What is the problem with dynamic linking with CI? With the changes to the CMakeLists.txt all binaries are output into a common binary directory, thus the tests should be able to find the dlls as well?

Good point! I didn't pay attention to that. I will then write a small test for this.

I prefer to have a static context anyhow, so for the moment wit stick with this and hope users are a bit careful (a wrong assumption most of the times).