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

Conversation

@t-b
Copy link
Collaborator

@t-b t-b commented Oct 22, 2019

In 86174eea (Travis Tests: Avoid code duplication, 2019-06-05) we added
support for using the environment variable NON_STOCK_CPPZMQ to say if we
use the stock version or the non-stock version.

That commit got the logic wrong. Flip the naming to have now one less
negation.

STOCK_CPPZMQ=ON means that the system cppzmq is used, =OFF that we use
the latest cppzmq from the repo.

(Adapted from fedback in #598 (comment))

@t-b t-b requested a review from bourtemb October 22, 2019 14:44
@t-b t-b self-assigned this Oct 22, 2019
@bourtemb
Copy link
Member

In 4b5128f (zmqeventconsumer/zmqeventsupplier: Make it compatible with
all stock cppzmq versions, 2019-06-05) we added support for using the
environment variable NON_STOCK_CPPZMQ to say if we use the stock version
or the non-stock version.

That commit got the logic wrong. Flip the naming to have now one less
negation

To be fair, NON_STOCK_CPPZMQ was added in 86174ee and the logic simply reproduced what we had before where we were installing a custom cppzmq version on all Debian versions except Debian 8 which was already coming with a compatible zmq.hpp

In the mean time, the compilation issue when using these different zmq.hpp files versions got fixed, so I think it is a good idea to do the tests with the zmq.hpp file coming from the Debian distribution.
For Debian 8, we know it used to work with the default zmq.hpp file coming with Debian 8, so why not trying with a more recent cppzmq version...?
This is what this PR is proposing.

@t-b t-b force-pushed the fix-cppzqm-non-stock-flag branch from 7a339cc to 4954113 Compare October 22, 2019 15:21
@t-b
Copy link
Collaborator Author

t-b commented Oct 22, 2019

@bourtemb Thx. I've corrected the referenced commit.

@t-b
Copy link
Collaborator Author

t-b commented Oct 22, 2019

The wheezy build fails with the stock cppzmq.

The version check for the zmq socket having the disconnect method is wrong. It currently reads

#ifdef ZMQ_VERSION
	#if ZMQ_VERSION > 30201
		#define ZMQ_HAS_DISCONNECT
	#endif
#endif

but this check the libzmq version and not the cppzmq version. A proper solution would be to instead of checking the version checking the existance of the method in cmake and set the define from that. Or just use a non-stock cppzmq for wheezy.

@bourtemb
Copy link
Member

but this check the libzmq version and not the cppzmq version. A proper solution would be to instead of checking the version checking the existance of the method in cmake and set the define from that.

I agree this would be the cleanest. Could you please do that?

@t-b t-b force-pushed the fix-cppzqm-non-stock-flag branch from 4954113 to b4f7b5b Compare October 23, 2019 14:59
@t-b
Copy link
Collaborator Author

t-b commented Oct 23, 2019

@bourtemb I've added a feature check for zmq::socket::disconnect. I'm only doing that on non-windows platforms, as windows should be unproblematic. Let's see what CI says.

@t-b
Copy link
Collaborator Author

t-b commented Oct 23, 2019

@bourtemb Wheezy now builds with stock cppzmq.

@t-b t-b removed their assignment Oct 23, 2019
@bourtemb bourtemb self-requested a review October 24, 2019 15:26
@bourtemb
Copy link
Member

bourtemb commented Oct 24, 2019

Thanks for the fix.
This will fix the problem of someone compiling cppTango with CMake and using an old version of zmq.hpp.

For the Tango source distribution, we are still using the autotools. We are in a strange situation where CMake is actually used to generate some files (ant build package command to build the source distribution package) which are provided with the source distribution tar file. tango_const.h is one of these files.
So if the source distribution package is built from a computer where zmq.hpp does support zmq::socket::disconnect and then someone uses this generated package on a computer where zmq.hpp does not support zmq::socket::disconnect, there will still be some compilation problems for this person.

I think we may have to do something on the autoconf side to make it work for this use case, or we should provide a different package version, or instructions for the persons using an old version of zmq.hpp file, or a configure flag, ...?

There is a plan in the future to use CMake instead of configure to compile cpptango in the source distribution provided to the users.

@t-b
Copy link
Collaborator Author

t-b commented Oct 24, 2019

think we may have to do something on the autoconf side to make it work for this use case, or we should provide a different package version, or instructions for the persons using an old version of zmq.hpp file, or a configure flag, ...?

I'll find a autoconf version as well.

@t-b
Copy link
Collaborator Author

t-b commented Oct 24, 2019

I did some research and I think we don't need to do anything.

In the tango source distribution configure.ac file https://github.com/tango-controls/TangoSourceDistribution/blob/8a63f208b27c6cec36b76a24cdc02b673804da25/assets/configure.ac#L194

we require libzmq >=4.05. This is the version which was shipped with debian jessie (oldoldstable). And I just looked into that package and there zmq::socket_t::disconnect exists. According to https://docs.travis-ci.com/user/reference/linux#default travis uses ubuntu xenial by default so cmake (running on travis) and the user running ./configure will very very likely get the same results.

@bourtemb
Copy link
Member

I did some research and I think we don't need to do anything.

In the tango source distribution configure.ac file https://github.com/tango-controls/TangoSourceDistribution/blob/8a63f208b27c6cec36b76a24cdc02b673804da25/assets/configure.ac#L194

we require libzmq >=4.05. This is the version which was shipped with debian jessie (oldoldstable). And I just looked into that package and there zmq::socket_t::disconnect exists.

Debian Jessie (Debian 8) is fine... And this exactly why we didn't need to install a recent version of cppzmq on Debian 8 travis tests.
We'll get some problems with Debian 7 (wheezy), which provided zmq 3.2.3 (libzmq3 package).
But if we require libzmq 4.0.5 in the configure as you noticed, then I think it's a good point because users who want to build latest source distribution on Debian 7 or older (there will probably be some), will already have to install a more recent version of libzmq.
The problem is that zmq.hpp file is not provided with libzmq when you install it manually from source.
Users will have to install as well cppzmq in order to get a correct zmq.hpp file, but they could do that, since they already had to install libzmq manually.
Maybe we can simply provide some support for these specific use cases and guide them?

According to https://docs.travis-ci.com/user/reference/linux#default travis uses ubuntu xenial by default so cmake (running on travis) and the user running ./configure will very very likely get the same results.

Not if the user is on an old debian 7 (or older) without the correct cppzmq... We can also consider it is his responsibility to get it right (or to use a more recent OS), but then we need to document it well.

I think there is something we can improve too in this PR. It is to avoid to clone cppzmq repository when STOCK_CPPZMQ=ON

t-b added 3 commits October 25, 2019 13:16
In 86174ee (Travis Tests: Avoid code duplication, 2019-06-05) we added
support for using the environment variable NON_STOCK_CPPZMQ to say if we
use the stock version or the non-stock version.

That commit got the logic wrong. Flip the naming to have now one less
negation.

STOCK_CPPZMQ=ON means that the system cppzmq is used, =OFF that we use
the latest cppzmq from the repo.
Since 9c02de3 (- Commit after merge with NextRelease branch at release 22135, 2013-02-22)
we check the libzmq version to decide if cppzmq's socket class has a
disconnect method or not.

This currently fails on debian wheezy.

As that is also the wrong approach in general, we now use a compilation
test in the cmake setup phase to determine if we have the disconnect method
or not. This is only done on non-Windows platforms, on Windows we assume
that we have a disconnect method (we currently always compile against a
cppzmq version which has it).

In addition the define got also renamed from ZMQ_HAS_DISCONNECT to
TANGO_ZMQ_HAS_DISCONNECT to make it clear that this is our define.
@t-b t-b force-pushed the fix-cppzqm-non-stock-flag branch from ec6650b to 907aae9 Compare October 25, 2019 11:29
@t-b
Copy link
Collaborator Author

t-b commented Oct 25, 2019

I think there is something we can improve too in this PR. It is to avoid to clone cppzmq repository when STOCK_CPPZMQ=ON

Done.

Not if the user is on an old debian 7 (or older) without the correct cppzmq

I'll add an autotools check as well, see tango-controls/TangoSourceDistribution#54.

We can also consider it is his responsibility to get it right (or to use a more recent OS), but then we > need to document it well.

I've opened #606 to enhance the installation instructions.

Copy link
Member

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@t-b t-b merged commit ddeb263 into tango-9-lts Oct 27, 2019
@t-b t-b deleted the fix-cppzqm-non-stock-flag branch October 29, 2019 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants