-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
} | ||
else if(protocol == SSLv3) | ||
{ | ||
#ifndef OPENSSL_NO_SSL3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use separate commit for this change. I guess it's not related to CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not but it does relate to default version of OpenSSL in Hunter. Seems to be older versions don't need this fix according to this pull request: apache#1102.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not but it does relate to default version of OpenSSL in Hunter
Not sure I follow. It's not about CMake or Hunter. This change is about C++ code. Make a separate commit with this change.
according to this pull request: apache#1102
Please add this link to commit too. Effectively it means we have two commits here.
@@ -0,0 +1,121 @@ | |||
# | |||
# Licensed to the Apache Software Foundation (ASF) under one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it cherry-picked from hunter-0.10.0
branch? Can you add a note about it? Is there any difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake build seems to be new. Essentially checked out all CMake stuff from 0.10.0, then changed source files to match 0.9.2. Where should I add the note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially checked out all CMake stuff from 0.10.0
Commit 1: "CMake stuff from hunter-0.10.0 branch"
then changed source files to match 0.9.2
Commit 2 (I guess it's 1d24cc9 already?)
@@ -0,0 +1,121 @@ | |||
# | |||
# Licensed to the Apache Software Foundation (ASF) under one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially checked out all CMake stuff from 0.10.0
Commit 1: "CMake stuff from hunter-0.10.0 branch"
then changed source files to match 0.9.2
Commit 2 (I guess it's 1d24cc9 already?)
} | ||
else if(protocol == SSLv3) | ||
{ | ||
#ifndef OPENSSL_NO_SSL3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not but it does relate to default version of OpenSSL in Hunter
Not sure I follow. It's not about CMake or Hunter. This change is about C++ code. Make a separate commit with this change.
according to this pull request: apache#1102
Please add this link to commit too. Effectively it means we have two commits here.
OK should be fixed now. |
} | ||
else if(protocol == SSLv3) | ||
{ | ||
#ifndef OPENSSL_NO_SSL3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no such macro in https://github.com/apache/thrift/pull/1102/files change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wrong commit. Hold on...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm expecting two separate commits. One for https://github.com/apache/thrift/pull/1102/files change and one for https://github.com/apache/thrift/pull/944/files
Done |
Second attempt.