You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/08/18 14:36:10 UTC
[GitHub] [thrift] lorteddie opened a new pull request, #2644: Use openssl and zlib targets
lorteddie opened a new pull request, #2644:
URL: https://github.com/apache/thrift/pull/2644
<!-- Explain the changes in the pull request below: -->
If available the targets of OpenSSL and ZLIB should be used instead of library paths
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1241687985
Yep, this is a very reasonable change, and seems fully backwards compatible.
@lorteddie , the build errors are mostly unrelated. But could you kindly rebase on latest master and force push once more, to see if they maybe go away?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1241846366
The AppVeyor test is successful except for an unrelated issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1242656131
From a brief look, I think an inconsistency may have been there even before your change. After your change, the line `target_link_libraries(thrift_c_glib_zlib ${SYSLIBS})` is inconsistent with the other statements of `target_link_libraries`, because some use the `PUBLIC` keyword and others do not. Is that possibly the problem?
I think most of cmake uses the `PUBLIC` keyword nowadays in thrift, so maybe add that where it's missing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1243359361
The build errors seem all unrelated, but I'll pull this first into my local CI and test there, to be sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1243742068
Ok, build errors are unrelated and the changes work in my local CI too, merging. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau merged pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau merged PR #2644:
URL: https://github.com/apache/thrift/pull/2644
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] lorteddie commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
lorteddie commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1243000796
thanks for the feedback, I added the PUBLIC keyword to make the calls more consistent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2644: Use openssl and zlib targets
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2644:
URL: https://github.com/apache/thrift/pull/2644#issuecomment-1242649156
Thanks @lorteddie ! The Travis build shows a problem in the current implementation, see https://app.travis-ci.com/github/apache/thrift/jobs/582329079:
```
CMake Error at lib/c_glib/CMakeLists.txt:105 (target_link_libraries):
The plain signature for target_link_libraries has already been used with
the target "thrift_c_glib_zlib". All uses of target_link_libraries with a
target must be either all-keyword or all-plain.
The uses of the plain signature are here:
* lib/c_glib/CMakeLists.txt:101 (target_link_libraries)
* lib/c_glib/CMakeLists.txt:102 (target_link_libraries)
```
I think this error did not exist before, is that possible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org