#5496 closed defect (fixed)

boost 1.66 issues

Reported by: fdupont Owned by: fdupont
Priority: very high Milestone: Kea1.4
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: 2
Total Hours: 0 Internal?: no

Description (last modified by fdupont)

Last boost version 1.66 changed some asio definitions, e.g. it makes io_service a key word using a typedef. Gihub PR 60.

Subtickets

Change History (20)

comment:1 Changed 18 months ago by fdupont

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

comment:2 Changed 18 months ago by fdupont

  • Description modified (diff)

comment:3 Changed 18 months ago by fdupont

  • Description modified (diff)

comment:4 Changed 18 months ago by fdupont

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

Works fine for me (macOS after a brew upgrade).

comment:5 Changed 18 months ago by fdupont

  • Priority changed from medium to very high

Blocking on last macOS and any system which will be updated to last boost...

comment:6 Changed 18 months ago by fdupont

BTW you can consider I successfully reviewed it: I just want a second eye and a decision between merging the PR or the branch (they are identical so it is not a technical question).

comment:7 Changed 18 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:8 follow-up: Changed 18 months ago by tmark

  • Add Hours to Ticket changed from 0 to 2
  • Owner changed from tmark to fdupont

I found no changes in trac5496, did you not push them?

I did pull the patch for PR60 and applied it to a pull of master under Ubuntu 16.04 and building against Boost 1.66 I get this error:

In file included from /opt/boost/boost_1_66_0/boost/asio/impl/write.hpp:25:0,

from /opt/boost/boost_1_66_0/boost/asio/write.hpp:927,
from /opt/boost/boost_1_66_0/boost/asio/buffered_write_stream.hpp:29,
from /opt/boost/boost_1_66_0/boost/asio/buffered_stream.hpp:22,
from /opt/boost/boost_1_66_0/boost/asio.hpp:41,
from ../../../../src/lib/asiolink/asio_wrapper.h:75,
from ../../../../src/bin/dhcp4/ctrl_dhcp4_srv.h:10,
from get_config_unittest.cc:19:

/opt/boost/boost_1_66_0/boost/asio/detail/consuming_buffers.hpp: In member function ‘boost::asio::detail::consuming_buffers<Buffer, Buffers, Buffer_Iterator>::prepared_buffers_type boost::asio::detail::consuming_buffers<Buffer, Buffers, Buffer_Iterator>::prepare(std::size_t)’:
/opt/boost/boost_1_66_0/boost/asio/detail/consuming_buffers.hpp:105:50: error: parse error in template argument list

while (next != end && max_size > 0 && result.count < result.max_buffers)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 18 months ago by fdupont

Replying to tmark:

I found no changes in trac5496, did you not push them?

=> I forgot but as trca5496 is the same than the PR this should not have an impact.

I did pull the patch for PR60 and applied it to a pull of master under Ubuntu 16.04 and building against Boost 1.66 I get this error:

In file included from /opt/boost/boost_1_66_0/boost/asio/impl/write.hpp:25:0,

from /opt/boost/boost_1_66_0/boost/asio/write.hpp:927,
from /opt/boost/boost_1_66_0/boost/asio/buffered_write_stream.hpp:29,
from /opt/boost/boost_1_66_0/boost/asio/buffered_stream.hpp:22,
from /opt/boost/boost_1_66_0/boost/asio.hpp:41,
from ../../../../src/lib/asiolink/asio_wrapper.h:75,
from ../../../../src/bin/dhcp4/ctrl_dhcp4_srv.h:10,
from get_config_unittest.cc:19:

/opt/boost/boost_1_66_0/boost/asio/detail/consuming_buffers.hpp: In member function ‘boost::asio::detail::consuming_buffers<Buffer, Buffers, Buffer_Iterator>::prepared_buffers_type boost::asio::detail::consuming_buffers<Buffer, Buffers, Buffer_Iterator>::prepare(std::size_t)’:
/opt/boost/boost_1_66_0/boost/asio/detail/consuming_buffers.hpp:105:50: error: parse error in template argument list

while (next != end && max_size > 0 && result.count < result.max_buffers)

=> I have some Ubuntu VMs so I'll check but I have a silly question: is the problem from the patch or from boost 1.66? In the second case it is neither blocking or something we have to fix ourselves.
IMHO 16.04 is a bit old but it is a LTS so a version customers could use...

comment:10 in reply to: ↑ 9 Changed 18 months ago by tmark

Replying to fdupont:

Replying to tmark:

I found no changes in trac5496, did you not push them?

=> I forgot but as trca5496 is the same than the PR this should not have an impact.

I did pull the patch for PR60 and applied it to a pull of master under Ubuntu 16.04 and building against Boost 1.66 I get this error:

In file included from /opt/boost/boost_1_66_0/boost/asio/impl/write.hpp:25:0,

from /opt/boost/boost_1_66_0/boost/asio/write.hpp:927,
from /opt/boost/boost_1_66_0/boost/asio/buffered_write_stream.hpp:29,
from /opt/boost/boost_1_66_0/boost/asio/buffered_stream.hpp:22,
from /opt/boost/boost_1_66_0/boost/asio.hpp:41,
from ../../../../src/lib/asiolink/asio_wrapper.h:75,
from ../../../../src/bin/dhcp4/ctrl_dhcp4_srv.h:10,
from get_config_unittest.cc:19:

/opt/boost/boost_1_66_0/boost/asio/detail/consuming_buffers.hpp: In member function ‘boost::asio::detail::consuming_buffers<Buffer, Buffers, Buffer_Iterator>::prepared_buffers_type boost::asio::detail::consuming_buffers<Buffer, Buffers, Buffer_Iterator>::prepare(std::size_t)’:
/opt/boost/boost_1_66_0/boost/asio/detail/consuming_buffers.hpp:105:50: error: parse error in template argument list

while (next != end && max_size > 0 && result.count < result.max_buffers)

=> I have some Ubuntu VMs so I'll check but I have a silly question: is the problem from the patch or from boost 1.66? In the second case it is neither blocking or something we have to fix ourselves.
IMHO 16.04 is a bit old but it is a LTS so a version customers could use...

I believe the issue is from 1.66,not the patch. However, I don't really see how we can consider a ticket called Boost 1.66 issues done, if building with 1.66 doesn't work.

16.04 is only a 1.5 years old, so it's hardly ancient.

comment:11 Changed 18 months ago by fdupont

  • Owner changed from fdupont to tmark

Tried on macOS 10.3.2, Fedora 27, Ubuntu 17.10 (where Botan crashes at initialization?) and Ubuntu 16.0.3 LTS without problems. (*)

My 16.04.3 config is an up-to-date 64 system with master branch + 5496 patch, gtest-1.8.0 and boost 1.66.0 from boost.org.
Configure arguments are --enable-boost-headers-only --with-boost-include=/home/dupont/boost_1_66_0 --with-gtest-source=/homedupont/gtest-1.8.0/googletest

(*) first make check crashes in CtrlAgentCommandMgrTest.forwardListCommands, second try worked well.

comment:12 follow-up: Changed 18 months ago by tmark

--enable-boost-headers-only is not the recommended/supported way to build anymore.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 18 months ago by fdupont

Replying to tmark:

--enable-boost-headers-only is not the recommended/supported way to build anymore.

1- it should be safe with recent versions of boost
2- it is far easier to check than to build libraries too, now if you know a pre-build package...
3- it has more constraint than libraries so if it works any option should work too

Now if I can reproduce your setup I am ready...

comment:14 in reply to: ↑ 13 ; follow-up: Changed 18 months ago by tmark

  • Owner changed from tmark to fdupont

Replying to fdupont:

Replying to tmark:

--enable-boost-headers-only is not the recommended/supported way to build anymore.

1- it should be safe with recent versions of boost
2- it is far easier to check than to build libraries too, now if you know a pre-build package...
3- it has more constraint than libraries so if it works any option should work too

This is entirely different issue, so how about we tackle one thing at a time. I pointed it out just so you'd realize that we currently document that it isn't recommended. If the issue has been addressed in newer versions of boost then that maybe it deserves its own ticket to explore.

Now if I can reproduce your setup I am ready...

I install boost from source tar ball, so I can run install more than one version on a given machine and control where it goes. It isn't hard to do and if you use --with-system so it only builds the system library it builds quickly.

comment:15 in reply to: ↑ 14 Changed 18 months ago by fdupont

Replying to tmark:

Now if I can reproduce your setup I am ready...

I install boost from source tar ball, so I can run install more than one version on a given machine and control where it goes. It isn't hard to do and if you use --with-system so it only builds the system library it builds quickly.

=> I am doing this. Anyway the meltdown patch should be available now so I have to update my Ubuntu VMs...

comment:16 Changed 18 months ago by fdupont

  • Owner changed from fdupont to tmark

Reproduced and fixed the issue (by changing include order). Fixed directly in master with 83cf70aec7..7d4cd801ef commit.
Now boost 1.66.0 with libs works well on Ubuntu 16.04 LTS so we can resume review.

comment:17 follow-up: Changed 17 months ago by tmark

  • Owner changed from tmark to fdupont

Why did you commit the include order changes directly to master and not in the branch for this ticket? You committed an un-reviewed change directly to master, which we are not supposed to do. Even when the build farm is broken the change is at least looked at by someone else via jabber.

Despite git log for trac5496 showing the include change being merged into the branch, the changes are not actually there. I had to apply them as a patch to test them. I did not commit this.

After I applied the includes change from master, it does indeed build and the unit tests pass. However, please explain why the include order makes a difference. Is it a name collision with something in Boost?

Is shuffling the includes around the proper fix for this? Apparently, there is now an order dependency of includes that is not obvious to the casual observer.

Last edited 17 months ago by tmark (previous) (diff)

comment:18 in reply to: ↑ 17 Changed 17 months ago by fdupont

  • Owner changed from fdupont to tmark

Replying to tmark:

Why did you commit the include order changes directly to master and not in the branch for this ticket?

=> because it was not related to this ticket (other than this ticket found the problem).

Actually, it is related to 1.66 (or seems to be) as the error does not occur with Boost 1.63.

You committed an un-reviewed change directly to master, which we are not supposed to do. Even when the build farm is broken the change is at least looked at by someone else via jabber.

=> nobody was available on Jabber.

By the logic, I can merge in un-reviewed tickets on a weekend. If the build farm were totally broken, that might be different.

=> BTW do you agree I fix the hooks/tests/Makefile.am?

I assume you are talking adding the BOOST_LIBS? Yes.

Despite git log for trac5496 showing the include change being merged into the branch, the changes are not actually there.

=> it was only in master so I don't understand why git log trac5496 showed it

Sorry, I misread the log.

After I applied the includes change from master, it does indeed build and the unit tests pass. However, please explain why the include order makes a difference. Is it a name collision with something in Boost?

=> I suspect a typedef name collision with one dhcp/option header on Buffer but it did not happen on macOS and namespaces are supposed to avoid this so it should be something more complex... Note I am not surprise the asio include hack leads to some problems.

Is shuffling the includes around the proper fix for this?

=> includes were not in the recommended (cf style guidelines) order anyway.

Apparently, there is now an order dependency of includes that is not obvious to the casual observer.

=> I am afraid we do not have the time to explain everything in the universe. The fix was easy, I tried to get the more logical order so nobody should change it for a silly reason and reactivate the issue.

So you didn't root cause it, your changes work around it. It may be this bites us again, let's hope not.

=> BTW as we are now 2 persons to have checked the include order patch can you reapply it to master? And if you are happy with the PR60 or this branch can you merge it with a ChangeLog like "Added support of boost 1.66.0 ASIO."?

You took the ticket on, you should complete it.

Last edited 17 months ago by tmark (previous) (diff)

comment:19 Changed 17 months ago by tmark

  • Owner changed from tmark to fdupont

comment:20 Changed 17 months ago by fdupont

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

Done. Closing...

Note: See TracTickets for help on using tickets.