-
Notifications
You must be signed in to change notification settings - Fork 34
CMake: Add compile check to test for correct ZeroMQ C++ headers #530
Conversation
9f3ca3a
to
ebfd0b2
Compare
We are relying on the presence of convertability from zmq::socket_t to void *. Add a cmake check so that we know that we have the correct version.
ebfd0b2
to
8873b16
Compare
@t-b , well I do not run travis locally, I run the build process and tests (all in one run, or just a single one) from CLion. Taking into account that I am on Debian 8 build process finishes fine. Which is not the case for this particular PR for non-Debian 8 configurations. As I see cmake complains about cppzmq:
On Debian 9&7 where we build our own version of cppzmq (on Debian 8 we use The error message is added in this PR so the answer is easy - what we build is not compatible or maybe the reason is that cmake compiles against wrong version of cppzmq :) Fetching cppzmq code is in .travis.yml and the build is in install_cppzmq.sh Hope this helps |
${CMAKE_BINARY_DIR}/configure | ||
SOURCES ${CMAKE_SOURCE_DIR}/configure/test_zmq_hpp.cpp | ||
CMAKE_FLAGS "-I ${CPPZMQ_BASE}/include -I ${ZMQ_PKG_INCLUDE_DIRS}/include" | ||
LINK_LIBRARIES ${ZMQ_PKG_LIBRARIES}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to avoid ${ZMQ_PKG_XXX}
here and use ${CPPZMQ_BASE}
everywhere. As it is now - cmake compiles against cppzmq from pkg-config (from apt-get
) which is indeed incompatible on Debian 9&7
Hi @t-b, The scripts to run the tests are actually described in https://github.com/tango-controls/cppTango/blob/tango-9-lts/INSTALL.md file and on the wiki for running the tests from CLion: https://github.com/tango-controls/cppTango/wiki/Contribution-Guide |
Why can't we simply use explicit conversions to void* to compile Tango with libzmq3-dev from Ubuntu 16 repos? diff --git a/cppapi/client/zmqeventconsumer.cpp b/cppapi/client/zmqeventconsumer.cpp
index 25128a25..2b0f676a 100644
--- a/cppapi/client/zmqeventconsumer.cpp
+++ b/cppapi/client/zmqeventconsumer.cpp
@@ -181,9 +181,9 @@ void *ZmqEventConsumer::run_undetached(TANGO_UNUSED(void *arg))
zmq::pollitem_t *items = new zmq::pollitem_t [MAX_SOCKET_SUB];
int nb_poll_item = 3;
- items[0].socket = *control_sock;
- items[1].socket = *heartbeat_sub_sock;
- items[2].socket = *event_sub_sock;
+ items[0].socket = static_cast<void*>(*control_sock);
+ items[1].socket = static_cast<void*>(*heartbeat_sub_sock);
+ items[2].socket = static_cast<void*>(*event_sub_sock);
for (int loop = 0;loop < nb_poll_item;loop++)
{
@@ -1063,7 +1063,7 @@ bool ZmqEventConsumer::process_ctrl(zmq::message_t &received_ctrl,zmq::pollitem_
// Update poll item list
//
- poll_list[old_poll_nb].socket = *tmp_sock;
+ poll_list[old_poll_nb].socket = static_cast<void*>(*tmp_sock);
poll_list[old_poll_nb].fd = 0;
poll_list[old_poll_nb].events = ZMQ_POLLIN;
poll_list[old_poll_nb].revents = 0;
diff --git a/cppapi/server/zmqeventsupplier.cpp b/cppapi/server/zmqeventsupplier.cpp
index b1752101..f5bfb11e 100644
--- a/cppapi/server/zmqeventsupplier.cpp
+++ b/cppapi/server/zmqeventsupplier.cpp
@@ -686,7 +686,7 @@ void ZmqEventSupplier::create_mcast_socket(string &mcast_data,int rate,McastSock
// Bind the publisher socket to the specified port
//
- if (zmq_bind(*(ms.pub_socket),ms.endpoint.c_str()) != 0)
+ if (zmq_bind(static_cast<void*>(*ms.pub_socket),ms.endpoint.c_str()) != 0)
{
TangoSys_OMemStream o;
o << "Can't bind ZMQ socket with endpoint ";
In some older implementations of zmq.hpp, void* conversion is explicit in C++11 (and above):
|
@mliszcz Good point. I think we should do something along these lines. Looking at https://github.com/zeromq/cppzmq/blob/d25c58a05dc981a63a92f8b1f7ce848e7f21d008/zmq.hpp#L1272 also shows that How about something like (from the top of my head): template<typename T>
void* ConvertToZMQPlainSocket(const T &s)
{
#if CPPZMQ_VERSION >= ZMQ_MAKE_VERSION(A, B, C)
return s.handle();
#elif CPPZMQ_VERSION >= ZMQ_MAKE_VERSION(D, E, F)
return static_cast<void*>(s);
#else
return s;
#endif
} With that wrapper we have a chance of getting rid of the code again at some point in the future. |
Looks good to me. Two comments:
|
@bourtemb @mliszcz So I nearly finished the code to detect all the different cppzmq variants. But looking closer into the latest version of cppzmq showed that socket_t is still available and it also has a implicit |
We are relying on the presence of convertability from zmq::socket_t to void *.
Add a cmake check so that we know that we have the correct version.
Close #273.
@Ingvord.