Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

Be compatible with all in-use cppzmq versions. #561

Merged
merged 2 commits into from
Aug 17, 2019
Merged

Be compatible with all in-use cppzmq versions. #561

merged 2 commits into from
Aug 17, 2019

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Jun 5, 2019 •
edited
Loading

Follow up from #530.

Close #499.
Close #273.
Close #535.

@t-b t-b requested review from mliszcz, Ingvord and bourtemb June 5, 2019 16:36
@t-b t-b mentioned this pull request Jun 5, 2019
Closed
@mliszcz
Copy link
Collaborator

mliszcz commented Jun 6, 2019

Looks like there is a problem with cppzmq from debian 7 repos:

/home/tango/src/cppapi/client/zmqeventconsumer.cpp:3553:16: error: 'class zmq::socket_t' has no member named 'disconnect'

@t-b
Copy link
Collaborator Author

t-b commented Jun 6, 2019

@mliszcz Okay so I'll add a test for disconnect and resurrect the manual cppzmq installation. Nobody should use debian7 (wheezy) anymore as it is EOL, but let's not get distracted here.

@mliszcz
Copy link
Collaborator

mliszcz commented Jun 6, 2019

Why do you need a test for disconnect? Maybe just revert install_cppzmq.sh and use if only for debian 7.

@t-b
Copy link
Collaborator Author

t-b commented Jun 6, 2019

@mliszcz The test is required so that people how are trying to compile it on debian 7 don't get a compile error but an error message from cmake.

@t-b t-b force-pushed the bugfix/more-compatible-zmqcpp branch from 5a82103 to 5923aab Compare June 11, 2019 10:39
@t-b t-b force-pushed the bugfix/more-compatible-zmqcpp branch 2 times, most recently from 29f0eb6 to 4675d56 Compare July 24, 2019 17:18
@tango-controls tango-controls deleted a comment Jul 24, 2019
mliszcz
mliszcz reviewed Jul 25, 2019
View reviewed changes
Copy link
Collaborator

@mliszcz mliszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@bourtemb
Copy link
Member

I think the issue with zmq disconnect is actually due to a recent refactoring change when ZmqEventConsumer::disconnect_socket() method was created.
I think we should simply enclose the definition of ZmqEventConsumer::disconnect_socket() inside a
#ifdef ZMQ_HAS_DISCONNECT section and it should compile without having the need to do the check at cmake level.

@t-b
Copy link
Collaborator Author

t-b commented Jul 25, 2019

@bourtemb The disconnect method was introduced in 2013 and only cppzmq of debian wheezy does not have that. I'll make the #ifdef changes you suggested.

@t-b t-b force-pushed the bugfix/more-compatible-zmqcpp branch from 4675d56 to cf1612b Compare July 25, 2019 19:40
@t-b
Copy link
Collaborator Author

t-b commented Jul 25, 2019

@bourtemb Turns out we already had the ZMQ_HAS_DISCONNECT define. I've done as you suggested and also removed my try compile as we can how handle both cases properly.

@tango-controls tango-controls deleted a comment Jul 25, 2019
@t-b t-b force-pushed the bugfix/more-compatible-zmqcpp branch from cf1612b to 7213bc7 Compare August 1, 2019 11:17
t-b added 2 commits August 2, 2019 18:21
@t-b
zmqeventconsumer/zmqeventsupplier: Make it compatible with all stock …
4b5128f 
…cppzmq versions

Depending on the cppzmq version from https://github.com/zeromq/cppzmq the
operator void* is either implicit or explicit.

We could now add a feature check in cmake to check for that and use code
paths for each variant.

But we could also be really pragmatic and just use a unconditional
static_cast<void*>.

Idea and initial patch by Michal Liszcz.
@t-b
ZmqEventConsumer: Add missing ZMQ_HAS_DISCONNECT check
a637035 
Although we only call the function disconnect_socket if the define is
true the compiler still sees the code.
@t-b t-b force-pushed the bugfix/more-compatible-zmqcpp branch from 7213bc7 to a637035 Compare August 2, 2019 16:22
@mliszcz mliszcz mentioned this pull request Aug 16, 2019
Merged
@t-b t-b merged commit cea2296 into tango-controls:tango-9-lts Aug 17, 2019
@t-b t-b deleted the bugfix/more-compatible-zmqcpp branch August 17, 2019 11:51
bourtemb added a commit that referenced this pull request Oct 18, 2019
@bourtemb
Update CHANGELOG.md with #561
0793ba1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bourtemb bourtemb Awaiting requested review from bourtemb bourtemb is a code owner

2 more reviewers

@mliszcz mliszcz mliszcz approved these changes

@Ingvord Ingvord Ingvord approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Ready For Merge
Projects
None yet
Milestone
No milestone
4 participants
@t-b @mliszcz @bourtemb @Ingvord