Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#4009 closed defect (fixed)

Use boost::asio in build

Reported by: stephen Owned by: fdupont
Priority: high Milestone: Kea1.0-beta
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

When BIND 10 was written, the decision was made to use the header-only form of boost::asio, mainly to avoid the need to build that library when installing the BIND 10 pre-requisites. For various reasons, that library is included in the git repository.

Boost has come a long way since the time that decision was made. In particular, Boost is now available as a package on most systems. It therefore makes sense to get rid of the local (header-only) copy of the ASIO library and link against the system-supplied one.

Issues to be considered when converting from the header-only version to the library version of ASIO can be found here.

Subtickets

Change History (39)

comment:1 Changed 2 years ago by fdupont

I strongly support this idea the current asio code is known both to have bugs for Windows and no support for visibility stuff.

comment:2 Changed 2 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

Trying:

  • remove ext/asio
  • include <asio/xxx> -> include <boost/asio/xxx>
  • include <asio.hpp> -> include <asio.hpp>
  • asio::xxx -> boost::asio::xxx

I'll summarise findings.

comment:3 Changed 2 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from medium to high

comment:4 Changed 2 years ago by fdupont

There are some asio in using and namespace.
asio::error_code and system_error are now in (boost::)::system::
boost_system is required if no specific flags to avoid this are set (BTW these flags are not trivial).

Managed to compile on OS X with boost::asio headers! And unit tests passed.

comment:5 Changed 2 years ago by fdupont

Ready for review! 3 questions:

  • should we add a specific test in configure.ac to avoid too old boost (i.e., boost without asio)?
  • is there a platform where the header only asio flag is required?
  • (as usual) ChangeLog entry

comment:6 Changed 2 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:7 Changed 2 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:8 Changed 2 years ago by fdupont

For the first I propose to add asio.hpp and asio/ip/address.h to ax_boost_for_kea.m4.
For the second I can't see a good reason (else it has no visible effect) to not define BOOST_ASIO_HEADER_ONLY.

comment:9 Changed 2 years ago by fdupont

asio/ip/address.hpp of course.

comment:10 follow-up: Changed 2 years ago by stephen

Reviewed commit d7c8e1aa32fe717a8e31def0daa9c46142279d27

All OK, the code built and the unit tests ran. I have a few minor issues:

  1. There were a number of places where the include of "asio.hpp" in a .cc file was replaced by an include of "boost/asio.hpp", but there were no other changes to the file. This suggests that we could simply remove the include in those files. The files I noticed were:
  • src/bin/dhcp4/dhcp4_srv.cc
  • src/bin/dhcp6/dhcp6_srv.cc
  1. Documentation of asiolink.h needs to be updated, as it refers to the non-Boost version of ASIO. Also, now that we are using the boost version of ASIO, is the restriction on the inclusion of asio.hpp (as documented in {tcp,udp}_endpoint.h & {tcp,udp}_socket.h) still valid? (And if so, why?)
  1. Not for this ticket, but as io_fetch is only used by dhcp-ddns, why not move it to the src/bin/d2 library. Then we can get rid of asides completely. At the same time, we can remove a number of unused asiodns messages as well as the README.

should we add a specific test in configure.ac to avoid too old boost (i.e., boost without asio)?

We could do that, or we could simply check on the version of Boost (in the file boost/version.hpp). Either way, we do need to check that the first version of Boost that included ASIO can be used to build Kea successfully.

is there a platform where the header only asio flag is required?

I don't know of one. What are the pros and cons of compiling with and without the header flag?

ChangeLog entry

I suggest something like:

Modified code to use the version of ASIO that comes with the Boost
library.

I'd organised one final check before I give the go-ahead to merge this ticket. As this is a reasonably significant change, I've asked QA to run this branch through one of their Forge tests. That should be a reasonable check that changing the version of ASIO has had no ill-effects. As soon as I get the results, I'll add a comment to the ticket and pass it back to you.

comment:11 in reply to: ↑ 10 Changed 2 years ago by fdupont

Replying to stephen:

Reviewed commit d7c8e1aa32fe717a8e31def0daa9c46142279d27

All OK, the code built and the unit tests ran. I have a few minor issues:

  1. There were a number of places where the include of "asio.hpp" in a .cc file was replaced by an include of "boost/asio.hpp", but there were no other changes to the file. This suggests that we could simply remove the include in those files. The files I noticed were:
  • src/bin/dhcp4/dhcp4_srv.cc
  • src/bin/dhcp6/dhcp6_srv.cc

=> I don't believe it is correct: boost/asio.hpp is the equivalent of asio.hpp so if asio.hpp was required boost/asio.hpp is likely still required.

  1. Documentation of asiolink.h needs to be updated, as it refers to the non-Boost version of ASIO.

=> done.

Also, now that we are using the boost version of ASIO, is the restriction on the inclusion of asio.hpp (as documented in {tcp,udp}_endpoint.h & {tcp,udp}_socket.h) still valid? (And if so, why?)

=> same answer than for 1.

  1. Not for this ticket, but as io_fetch is only used by dhcp-ddns, why not move it to the src/bin/d2 library. Then we can get rid of asides completely. At the same time, we can remove a number of unused asiodns messages as well as the README.

=> or merge asiolink and asides libraries?

should we add a specific test in configure.ac to avoid too old boost (i.e., boost without asio)?

We could do that, or we could simply check on the version of Boost (in the file boost/version.hpp). Either way, we do need to check that the first version of Boost that included ASIO can be used to build Kea successfully.

=> I am adding the 2 Casio headers in the boost m4 check.

is there a platform where the header only asio flag is required?

I don't know of one. What are the pros and cons of compiling with and without the header flag?

=> IMHO it is neutral so I am adding it.

ChangeLog entry

I suggest something like:

Modified code to use the version of ASIO that comes with the Boost
library.

I'd organised one final check before I give the go-ahead to merge this ticket. As this is a reasonably significant change, I've asked QA to run this branch through one of their Forge tests. That should be a reasonable check that changing the version of ASIO has had no ill-effects. As soon as I get the results, I'll add a comment to the ticket and pass it back to you.

=> I check the last changes still compile and I push them on the trac4009 branch.

comment:12 Changed 2 years ago by fdupont

Changes pushed and checked.

comment:13 follow-ups: Changed 2 years ago by marcin

Compilation failed for me on Centos 6 with the following error:

  CXX    run_unittests-data_file_unittests.o
In file included from /usr/include/boost/asio/error.hpp:23,
                 from /usr/include/boost/asio/ip/address.hpp:26,
                 from ../../../../src/lib/asiolink/io_address.h:23,
                 from ../../../../src/lib/dhcpsrv/cfg_hosts.h:18,
                 from ../../../../src/lib/dhcpsrv/srv_config.h:18,
                 from ../../../../src/lib/dhcpsrv/daemon.h:19,
                 from data_file_unittests.cc:17:
/usr/include/boost/system/error_code.hpp:508:54: error: boost/../libs/system/src/error_code.cpp: No such file or directory
make[5]: *** [run_unittests-data_file_unittests.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[5]: Leaving directory `/home/marcin/devel/kea/src/lib/cc/tests'
make[4]: *** [all-recursive] Error 1
make[4]: Leaving directory `/home/marcin/devel/kea/src/lib/cc'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/home/marcin/devel/kea/src/lib'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/marcin/devel/kea/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/marcin/devel/kea'

comment:14 in reply to: ↑ 13 Changed 2 years ago by fdupont

Replying to marcin:

Compilation failed for me on Centos 6

=> can you post the Boost version and the BOOST defines at config time (i.e., grep BOOST config.status)?
BTW is there an available CentOS 6 VM? (I have only a CentOS 7 VM). If not should a CentOS 6.7 VM enough to reproduce and fix the problem?

comment:15 in reply to: ↑ 13 Changed 2 years ago by fdupont

Replying to marcin:

Compilation failed for me on Centos 6 with the following error:

/usr/include/boost/system/error_code.hpp:508:54: error: boost/../libs/system/src/error_code.cpp: No such file or directory

At the very end of the error_code.hpp of Boost 1.41 (which should be the CentOS 6 version) I can find:

# ifdef BOOST_ERROR_CODE_HEADER_ONLY
#   include <boost/../libs/system/src/error_code.cpp>
# endif

BTW more recent versions use a .ipp file in the header tree:

# ifdef BOOST_ERROR_CODE_HEADER_ONLY
#   include <boost/system/detail/error_code.ipp>
# endif

with a very similar content...

so perhaps the libs directory is not installed or not at the expected place?
You can try removing the BOOST_ERROR_CODE_HEADER_ONLY flags and adding in cc and asiolink the -lboost_system dependency (note if you have no boost libraries it won't really help).

Last edited 2 years ago by fdupont (previous) (diff)

comment:16 Changed 2 years ago by fdupont

BTW the .cpp / .ipp change was done between 1.55 and 1.56 (and asio is included in boost since 1.35).

comment:17 follow-up: Changed 2 years ago by fdupont

I am afraid we have the choice between:

  • for recent systems with boost >= 1.56: header only boost.asio and boost.system
  • same for locally installed boost (which BTW is likely >= 1.56 too)
  • could be the same with C++ 11 where error_code is builtin?
  • for old systems with boost < 1.,56: header only boost.asio and boot_system library

IMHO it is an acceptable trade-off. If you agree I'll implement the test in configure.ac.

comment:18 in reply to: ↑ 17 Changed 2 years ago by marcin

Replying to fdupont:

I am afraid we have the choice between:

  • for recent systems with boost >= 1.56: header only boost.asio and boost.system
  • same for locally installed boost (which BTW is likely >= 1.56 too)
  • could be the same with C++ 11 where error_code is builtin?
  • for old systems with boost < 1.,56: header only boost.asio and boot_system library

IMHO it is an acceptable trade-off. If you agree I'll implement the test in configure.ac.

I think this is acceptable, as version 1.56 and later is going to be the one mostly present on all systems.

comment:19 Changed 2 years ago by fdupont

  • Owner changed from stephen to fdupont

Getting the ticket to implement the header vs library test.

comment:20 Changed 2 years ago by fdupont

The configure test works. It compiles with old Boost.
On Ubuntu the asiolink unit tests failed:

[ RUN      ] TCPSocket.sequenceTest
tcp_socket_unittest.cc:470: Failure
Value of: client_cb.called()
  Actual: 4
Expected: TCPCallback::READ
Which is: 2
[  FAILED  ] TCPSocket.sequenceTest (0 ms)

comment:21 Changed 2 years ago by marcin

I ran the build on CentOS 6 and found the following issues:


duid.cc: In static member function ‘static isc::dhcp::ClientIdPtr isc::dhcp::ClientId::fromText(const std::string&)’:
duid.cc:183: error: return type ‘struct isc::dhcp::ClientIdPtr’ is incomplete
duid.cc:185: error: invalid use of incomplete type ‘struct isc::dhcp::ClientIdPtr’
/usr/include/boost/exception/exception.hpp:135: error: declaration of ‘struct isc::dhcp::ClientIdPtr’

which required including the boost/shared_ptr.hpp in the duid.cc


iface_mgr_unittest.cc: In member function ‘virtual void<unnamed>::IfaceMgrTest_multipleSockets_Test::TestBody()’:
iface_mgr_unittest.cc:853: error: ‘F_GETFL’ was not declared in this scope
iface_mgr_unittest.cc:853: error: ‘fcntl’ was not declared in this scope
iface_mgr_unittest.cc:855: error: ‘F_SETFL’ was not declared in this scope
iface_mgr_unittest.cc:855: error: ‘O_NONBLOCK’ was not declared in this scope

which requires fcntl.h include


The packet_filter_unittest.cc also requires fcntl.h


duid_unittest.cc requires include of boost/shared_ptr.hpp


One I successfully compiled it, the unit test failed:

[ RUN      ] MemfileLeaseMgrTest.lfcTimer
terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::system::system_error> >'
  what():  mutex: Invalid argument

comment:22 Changed 2 years ago by marcin

So I included missing header files which allowed me to compile and committed this change.

As for the test... it turned out to be a problem in the Memfile unittest, which allowed for destruction of objects in the wrong order. There are two objects that belong to the test fixture class: IO service and interval timer. Interval timer is constructed using the IO Service. The order of declaration was such, that the IO Service was destructe before the interval timer. Hence, when the interval timer was destructed it held the reference to the non-existing IO service and caused a crash. Now, the crash wouldn't happen if the interval timer was explicitly cancelled, because the crash only occurs when the timer is running and the io service is destroyed. But, the interval timer is cancelled when it is destroyed so it seemed unnecessary to make explicit cancel. Another solution was to swap declaration in the class so as the destructor takes them in the correct order. But, since it would be difficult to highlight that the order matters I decided to put explicit reset() on both objects in the destructor, and added an extensive comment. I think no one should now overlook it.

So both changes are now pushed to the branch.

comment:23 follow-up: Changed 2 years ago by marcin

There is one more test that seems to fail consistently:

2015-09-04 17:12:50.414 INFO  [kea.dhcpddns/19262] DHCP_DDNS_SHUTDOWN DHCP-DDNS has shut down
d_controller_unittests.cc:247: Failure
Expected: runWithConfig("{}", 2000, elapsed_time) throws an exception of type ProcessRunError.
  Actual: it throws nothing.
[  FAILED  ] DStubControllerTest.launchRuntimeError (2002 ms)

I didn't investigate it yet.

comment:24 in reply to: ↑ 23 Changed 2 years ago by marcin

Replying to marcin:

There is one more test that seems to fail consistently:

2015-09-04 17:12:50.414 INFO  [kea.dhcpddns/19262] DHCP_DDNS_SHUTDOWN DHCP-DDNS has shut down
d_controller_unittests.cc:247: Failure
Expected: runWithConfig("{}", 2000, elapsed_time) throws an exception of type ProcessRunError.
  Actual: it throws nothing.
[  FAILED  ] DStubControllerTest.launchRuntimeError (2002 ms)

I didn't investigate it yet.

Ok, so this appeared to be a timing issue in one of the tests. The test simulates fatal error that occurs 2 seconds after d2 is started. But the D2 is started for 2 seconds so it seems that the D2 terminates before the fatal error can be generated and thus doesn't emit an exception. I extended the run of the test controller to 5 seconds to make sure that the fatal error is generated before it terminates. This fixes the problem.

I am also pleased to say that there are no other failures on CentOS. So, well done!

comment:25 Changed 2 years ago by fdupont

Fixed other timeouts to pass unit tests over CentOS 7 (cf #4034 to reduce the side effect).
Got strange errors on Fedora 22 (errors were no longer raised with some debug printf()s): added a --with-boost-libs both for handling cases where a special library path is required and for forcing Boost libraries, e.g., on Fedora 22.
Fixed make distcheck with specific boost setting.

comment:26 Changed 2 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

Ready for re-review. The ChangeLog entry should add some words about the new --with-boost-libs.

comment:27 Changed 2 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:28 follow-up: Changed 2 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit c77c514db657e94611701455a3c4a62b6f406c72

ext/LICENSE_1_0.txt
As this now only applies to the file in ext/coroutine, I suggest that it be moved there. Note that "Makefile.am" will need to be updated to reflect the new location.

As an aside, I note that the order of libraries given in the _LDADD variables is somewhat random. We should have a ticket to add them in dependency order (as recommended in your post to kea-dev about Makefile.am coding guidelines).

A suitable ChangeLog entry could be:

Removed the included header-only ASIO code.  Kea is now built
against the installed copy of Boost.  The build by default attempts
to use the header-only ASIO code, but may also use the version
in the boost system library.  The location of this library can be
specified.

Documentation
The doc/guide/install.xml file should be updated with a description of the --with-boost-libs switch.

Other
One issue I have is that the name of the --with-boost-libs switch is not really intuitive. The flag specifies the switches needs on the link command line to include the libraries, not - as the name of the switch suggests - the list of Boost libraries. --with-boost-lib-flags might be a better name for it.

comment:29 in reply to: ↑ 28 Changed 2 years ago by fdupont

ext/LICENSE_1_0.txt
As this now only applies to the file in ext/coroutine, I suggest that it be moved there. Note that "Makefile.am" will need to be updated to reflect the new location.

=> I agree.

As an aside, I note that the order of libraries given in the _LDADD variables is somewhat random. We should have a ticket to add them in dependency order (as recommended in your post to kea-dev about Makefile.am coding guidelines).

=> #3911

A suitable ChangeLog entry could be:

Removed the included header-only ASIO code.  Kea is now built
against the installed copy of Boost.  The build by default attempts
to use the header-only ASIO code, but may also use the version
in the boost system library.  The location of this library can be
specified.

Documentation
The doc/guide/install.xml file should be updated with a description of the --with-boost-libs switch.

=> yes.

Other
One issue I have is that the name of the --with-boost-libs switch is not really intuitive. The flag specifies the switches needs on the link command line to include the libraries, not - as the name of the switch suggests - the list of Boost libraries. --with-boost-lib-flags might be a better name for it.

=> lib-flags suggests LDFLAGS when libs is more for LDADD... And today the only not empty value is -lboost_system so clearly a LIBS. Perhaps we need both?

comment:30 Changed 2 years ago by fdupont

  • Owner changed from fdupont to stephen

Moved license file and added a --with-boost-libs description in install.xml. Next step?

comment:31 Changed 2 years ago by fdupont

  • Owner changed from stephen to UnAssigned

comment:32 Changed 2 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:33 Changed 2 years ago by marcin

  • Owner changed from marcin to fdupont

Reviewed commit 634a4806315166bf3283587551c10d90f9394857

We discussed via jabber that there are issues with detecting the boost system libraries on some operating systems. We figured that on those systems it may be required to put this into configuration:

--with-boost-libs="-L/usr/pkg/lib/ -lboost_system"

I think requiring the user to provide a proper combination of options is going to cause a lot of problems and debugging. So, I'd very much prefer to provide separate parameter for the location of the libraries and another one for the spec:

--with-boost-lib-dir=/usr/pkg/lib
--with-boost-libs=-lboost_system

Both should be well documented, with examples of use when running ./configure.

Also the install.xml should contain some examples.

comment:34 Changed 2 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

I thought about to poll the dev list about 1 vs 2 configuration flags but it seems I may directly adopt the 2 flag idea...
Both -L and -l go to *_LDADD automake variables to IMHO Marcin's proposal seems to best.

Done, branch trac4009 ready for another review round (please note some Jenkins VMs requires Boost system library).

comment:35 Changed 2 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:36 Changed 2 years ago by marcin

  • Owner changed from marcin to fdupont

I reviewed commit f96149661dbd3f5861bfdf154d18afdd866117e4

I tested this change on OS-X, FreeBSD10.1, NetBSD 6.0.2 and CentOS5. On each of those systems I was finally able to build the code.

I suggest that the description of the --with-boost-libs in the ./configure includes example of the SPEC, so:

--with-boost-libs=SPEC  specify Boost libraries to link with, e.g. "-lboost_system"

install.xml

I'd recommend that instead of this:

Specify the path to Boost libraries to link with (same comment than for --with-boost-libs).

you're more explicit

Specify the path to Boost libraries to link with (usually there should be no reason to specify this option).

I also suggest slighthly updating this text:

If you have some problems with the header-only Boost error code or you'd like to use the Boost system library in /usr/pkg/lib:

with

If you have some problems with building Kea using the header-only Boost error code or you'd like to use the Boost system library (e.g. located in /usr/pkg/lib)

If you agree with these suggestions please apply them and you're ready to merge. Otherwise, please state why not.

comment:37 Changed 2 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed

Just merged and push. Updating the Fedora 22 wiki and Jenkins VM. Closing...

comment:38 Changed 2 years ago by fdupont

Put here again comment 16:

BTW the .cpp / .ipp change was done between 1.55 and 1.56 (and asio is included in boost since 1.35).
(so header-only after >= 1.56 and Boost < 1.35 are too old).

comment:39 Changed 2 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.