Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4049 closed defect (fixed)

Kea no longer compiles with Xcode 7

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

Description

At least 2 issues:

Even OS X is not in the official list of supported systems (mainly because this list doesn't exist yet?) I put the maximum priority/severity.

Subtickets

Change History (19)

comment:1 Changed 4 years ago by fdupont

The typeid bug:

libdhcp++_unittest.cc:210:28: error: expression with side effects will be evaluated despite being used as an operand to 'typeid' [-Werror,-Wpotentially-evaluated-expression]
        EXPECT_TRUE(typeid(*option) == expected_type)
                           ^
/usr/local/Cellar/gtest/1.6.0/include/gtest/gtest.h:1784:23: note: expanded from macro 'EXPECT_TRUE'
  GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \
                      ^
/usr/local/Cellar/gtest/1.6.0/include/gtest/internal/gtest-internal.h:1179:34: note: expanded from macro 'GTEST_TEST_BOOLEAN_'
      ::testing::AssertionResult(expression)) \
                                 ^

comment:2 Changed 4 years ago by fdupont

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

comment:3 Changed 4 years ago by fdupont

Same typeid errors in option_definition_unittest.cc. BTW it seems it is a side effect of smart pointers: *option can really execute more than expected.
Suggested fix:

EXPECT_TRUE(typeid(*option) == expected_type);

replaced by:

const Option* optptr = option.get();
EXPECT_TRUE(typeid(optpr) == expected_type);
Last edited 4 years ago by fdupont (previous) (diff)

comment:4 Changed 4 years ago by fdupont

Easier fix: typeid(*foo) -> typeid(foo.get()).

comment:5 Changed 4 years ago by fdupont

No, it should be:

const Option* optptr = option.get();
EXPECT_TRUE(typeid(*optpr) == expected_type);

comment:6 Changed 4 years ago by fdupont

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

Got it. branch ready for review.

comment:7 follow-up: Changed 4 years ago by fdupont

BTW it seems to be a clang bug as typeid works well on a referenced smart pointer until the pointed class is polymorphic (i.e., has a virtual method). Of course this "until" matches exactly the case where typeid has some interest...
If we have a platform with a recent clang it will be interesting to know if the bug is Apple clang or plain clang, and to report it.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by marcin

Replying to fdupont:

BTW it seems to be a clang bug as typeid works well on a referenced smart pointer until the pointed class is polymorphic (i.e., has a virtual method). Of course this "until" matches exactly the case where typeid has some interest...
If we have a platform with a recent clang it will be interesting to know if the bug is Apple clang or plain clang, and to report it.

I don't observe any problem with typeid after installing Xcode 7.0. I only had to add --with-werror=no in the configuration to overcome the boost issue.

comment:9 in reply to: ↑ 8 Changed 4 years ago by fdupont

Replying to marcin:

=> BTW the boost bug is already fixed in the boost development version (cf the 11664 boost ticket which is now closed).

Replying to fdupont:
I don't observe any problem with typeid after installing Xcode 7.0. I only had to add --with-werror=no in the configuration to overcome the boost issue.

=> IMHO you got some warnings which was ignored by the --with-werror=no...

comment:10 Changed 4 years ago by marcin

  • Milestone changed from Kea-proposed to Kea1.0
  • Owner changed from UnAssigned to marcin

Since this is the regression which causes build failures on most of the developement systems that Kea team is using, I am moving it to 1.0 on my own discretion, assuming that it has to be fixed with high priority.

comment:11 Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

If this is clang and boost issue how do I reliably test it, without having a new clang and boost version?

comment:12 Changed 4 years ago by fdupont

Clang only.
The boost issue can be only be solved in the boost code (i.e., waiting for the next version of boost to publish and used by brew, or if we are lucky the patch added in brew, or (reliable) just patch locally the boost source). BTW this is why I put the boost ticket number.

comment:13 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

BTW as it solves an issue on a not supported platform (in this case more exactly a platform not present in Jenkins) IMHO this can be split into two parts:

  • check it has no bad effect on other platforms
  • trust someone (else) who has OS X with the last Xcode

comment:14 follow-up: Changed 4 years ago by marcin

I do have Xcode 7, but I don't see any urgency in resolving this ticket until patch for boost is available in brew (which doesn't seem to be the case yet).

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by fdupont

Replying to marcin:

I do have Xcode 7, but I don't see any urgency in resolving this ticket until patch for boost is available in brew (which doesn't seem to be the case yet).

=> this means you have no problem with adding --without-werror to all builds... BTW I signalled the problem to someone from the brew team but there was no answer nor action. If someone has a contact?

Another point: it is possible both issues appear on other platforms using clang (FreeBSD?) as they come from recent changes in clang.
So IMHO there is no good reason to postpone the resolution of this ticket (other than to have a large amount of tickets needed work, and it is not the case for the 1.0 review queue).

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by marcin

Replying to fdupont:

Replying to marcin:

I do have Xcode 7, but I don't see any urgency in resolving this ticket until patch for boost is available in brew (which doesn't seem to be the case yet).

=> this means you have no problem with adding --without-werror to all builds... BTW I signalled the problem to someone from the brew team but there was no answer nor action. If someone has a contact?

The point is that because of the boost error I need to add --without-werror to the build anyway. No?

Another point: it is possible both issues appear on other platforms using clang (FreeBSD?) as they come from recent changes in clang.
So IMHO there is no good reason to postpone the resolution of this ticket (other than to have a large amount of tickets needed work, and it is not the case for the 1.0 review queue).

There are multiple tickets in the review queue. But, even if they are not a concern we have over 120 tickets to work on in 1.0, which makes it a large amount of work. So, I don't understand your point here.

comment:17 in reply to: ↑ 16 Changed 4 years ago by fdupont

Replying to marcin:

The point is that because of the boost error I need to add --without-werror to the build anyway. No?

=> yes it is.

There are multiple tickets in the review queue.

=> only a few (which is a good thing) and the first choice should be to empty the review queue (I confess I don't follow this rule enough)

But, even if they are not a concern we have over 120 tickets to work on in 1.0, which makes it a large amount of work. So, I don't understand your point here.

=> my point is there is more win to review and merge #4049 than mostly anything else (and in particular when one looks at the review queue for 1.0). Note this applies even more to #3962...

comment:18 Changed 4 years ago by fdupont

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

It seems it was merged in master (IMHO as unwanted side effect of the merge of an unrelated branch). As it was very painful to work on an OS X machine without it I shan't complain and I shall simply close it!

comment:19 Changed 4 years ago by tomek

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

Milestone renamed

Note: See TracTickets for help on using tickets.