You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "dragon512 (via GitHub)" <gi...@apache.org> on 2023/11/14 22:33:46 UTC
[PR] Add logic to allow for external libswoc to be used [trafficserver]
dragon512 opened a new pull request, #10780:
URL: https://github.com/apache/trafficserver/pull/10780
Uses pkg_config todo find logic
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780#discussion_r1394639739
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
Maybe you can provide some context on why it was done this way. If we decide to keep this, we should definitely comment it, or this will be a "whoa there" moment for everyone who lays eyes on this in the future.
If we want to automatically pick up either the static or dynamic libswoc, depending on which one was found, then we do want one target that could represent either. But this does not look to me like the correct way to go about it. How come this even works? Some projects have both a static and dynamic target and install both of them - what does CMake do if both targets exist? Does `libswoc::libswoc` end up aliasing the static one, or the dynamic one? Or does CMake error out?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780#discussion_r1394639739
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
Maybe you can provide some context on why it was done this way. If we decide to keep this, we should definitely comment it, or this will be a "whoa there" moment for everyone who lays eyes on this in the future.
If we want to automatically pick up either the static or dynamic libswoc, depending on which one was found, then we do want one target that could represent either. How come this even works? Some projects have both a static and dynamic target and install both of them - what does CMake do if both targets exist? Does `libswoc::libswoc` end up aliasing the static one, or the dynamic one? Or does CMake error out?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780#discussion_r1394627576
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
Whoa there. This looks problematic. 😄
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
Whoa there. This looks problematic. 😄
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "dragon512 (via GitHub)" <gi...@apache.org>.
dragon512 commented on code in PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780#discussion_r1395960653
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
@cmcfarlen I believe was saying that we want to say something different to tell CMake what to use for the library
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "dragon512 (via GitHub)" <gi...@apache.org>.
dragon512 merged PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "dragon512 (via GitHub)" <gi...@apache.org>.
dragon512 commented on code in PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780#discussion_r1394630993
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
That was the easy fix... I am happy to have a better fix for the cmake build file for libswoc
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add logic to allow for external libswoc to be used [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10780:
URL: https://github.com/apache/trafficserver/pull/10780#discussion_r1394955873
##########
lib/swoc/CMakeLists.txt:
##########
@@ -118,3 +118,6 @@ set_target_properties(
)
install(TARGETS libswoc PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/swoc)
+
+add_library(libswoc::libswoc ALIAS libswoc-static)
+add_library(libswoc::libswoc ALIAS libswoc)
Review Comment:
I think he should use `libswoc::libswoc-static` for the static variant.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org