Address @madebr post-checkin comments. Doesn't currently build#144
Address @madebr post-checkin comments. Doesn't currently build#144mattgodbolt wants to merge 6 commits intomasterfrom
Conversation
|
@mattgodbolt From 59fd46a557a63233ff5944967be4cb6a14278ae0 Mon Sep 17 00:00:00 2001
From: Anonymous Maarten <anonymous.maarten@gmail.com>
Date: Fri, 7 Aug 2020 15:34:49 +0200
Subject: [PATCH] test_package uses std::thread so needs to link to
Threads::Threads
---
conanfile.py | 7 +++++--
test_package/CMakeLists.txt | 5 +++--
test_package/conanfile.py | 2 +-
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/conanfile.py b/conanfile.py
index 3415e9d..c6e3f8b 100644
--- a/conanfile.py
+++ b/conanfile.py
@@ -28,7 +28,7 @@ class SeasocksConan(ConanFile):
"with_zlib": [True, False],
}
default_options = {
- "shared": True,
+ "shared": False,
"fPIC": True,
"with_zlib": True,
}
@@ -65,6 +65,7 @@ class SeasocksConan(ConanFile):
source_folder=self.source_folder.replace("\\", "/"),
install_folder=self.install_folder.replace("\\", "/")))
cmake = CMake(self)
+ cmake.definitions["SEASOCKS_SHARED"] = self.options.shared
cmake.definitions["DEFLATE_SUPPORT"] = self.options.with_zlib
cmake.configure(source_folder=self.build_folder)
cmake.build()
@@ -78,7 +79,9 @@ class SeasocksConan(ConanFile):
self.cpp_info.names["cmake_find_package"] = "Seasocks"
self.cpp_info.names["cmake_find_package_multi"] = "Seasocks"
self.cpp_info.components["libseasocks"].libs = ["seasocks"]
- self.cpp_info.components["libseasocks"].system_libs = ["pthread"]
+ if not self.options.shared:
+ if self.settings.os == "Linux":
+ self.cpp_info.components["libseasocks"].system_libs = ["pthread"]
# Set the name of the generated seasocks target: `Seasocks::seasocks`
self.cpp_info.components["libseasocks"].names["cmake_find_package"] = "seasocks"
self.cpp_info.components["libseasocks"].names["cmake_find_package_multi"] = "seasocks"
diff --git a/test_package/CMakeLists.txt b/test_package/CMakeLists.txt
index 2dc844a..24cd6e7 100644
--- a/test_package/CMakeLists.txt
+++ b/test_package/CMakeLists.txt
@@ -2,9 +2,10 @@ cmake_minimum_required(VERSION 3.3)
project(PackageTest CXX)
include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
-conan_basic_setup(TARGETS)
+conan_basic_setup(TARGETS)
find_package(Seasocks REQUIRED)
+find_package(Threads REQUIRED)
add_executable(seasocks_test seasocks_test.cpp)
-target_link_libraries(seasocks_test PRIVATE Seasocks::seasocks)
+target_link_libraries(seasocks_test PRIVATE Seasocks::seasocks Threads::Threads)
diff --git a/test_package/conanfile.py b/test_package/conanfile.py
index 74f9376..bdfb323 100644
--- a/test_package/conanfile.py
+++ b/test_package/conanfile.py
@@ -4,7 +4,7 @@ import os
class TestPackageConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
- generators = "cmake"
+ generators = "cmake", "cmake_find_package"
def build(self):
cmake = CMake(self)
--
2.21.3 |
|
The change to the default shared seems wrong; also defining SEASOCKS_SHARED appears counter to the idea of using BUILD_SHARED_LIBS (which is working now!) But! I'll add the threads requried...why is that needed though? Your original comment removed it and added the I don't understand why we'd need to specify it in two places? |
|
The only change I needed to make was to add |
Yeah, it is not needed but I added it to be more explicit.
seasocks uses pthread internally, so needs to link to it. |
|
Also, using |
Codecov Report
@@ Coverage Diff @@
## master #144 +/- ##
=======================================
Coverage 35.92% 35.92%
=======================================
Files 52 52
Lines 2280 2280
=======================================
Hits 819 819
Misses 1461 1461 Continue to review full report at Codecov.
|
Got it: I hackily worked around it for now. I'd probably prefer cutting 1.4.5 and doing things the One True Way™ instead of a mixture of both CMake-y and seasocks-specific. But I'm also not too precious about it :) Thanks again. |
|
Only these changes need to made. From 6f56360f459be4aa69e352b6673e12d9ee012ffc Mon Sep 17 00:00:00 2001
From: Anonymous Maarten <anonymous.maarten@gmail.com>
Date: Fri, 14 Aug 2020 20:37:38 +0200
Subject: [PATCH] small fixes to cmake
---
conanfile.py | 1 +
test_package/CMakeLists.txt | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/conanfile.py b/conanfile.py
index 3415e9d..5617b0b 100644
--- a/conanfile.py
+++ b/conanfile.py
@@ -65,6 +65,7 @@ class SeasocksConan(ConanFile):
source_folder=self.source_folder.replace("\\", "/"),
install_folder=self.install_folder.replace("\\", "/")))
cmake = CMake(self)
+ cmake.definitions["SEASOCKS_SHARED"] = self.options.shared
cmake.definitions["DEFLATE_SUPPORT"] = self.options.with_zlib
cmake.configure(source_folder=self.build_folder)
cmake.build()
diff --git a/test_package/CMakeLists.txt b/test_package/CMakeLists.txt
index 2dc844a..e1cd7f5 100644
--- a/test_package/CMakeLists.txt
+++ b/test_package/CMakeLists.txt
@@ -5,6 +5,8 @@ include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup(TARGETS)
find_package(Seasocks REQUIRED)
+find_package(Threads REQUIRED)
add_executable(seasocks_test seasocks_test.cpp)
-target_link_libraries(seasocks_test PRIVATE Seasocks::seasocks)
+target_link_libraries(seasocks_test PRIVATE Seasocks::seasocks Threads::Threads)
+set_property(TARGET seasocks_test PROPERTY CXX_STANDARD 11)
--
2.21.3 |
|
Oh my, I think we got everything working OK in the end? I realise this is still open..but I /think/ we're good? |
|
@mattgodbolt |
madebr
force-pushed
the
mg/madebr_comments
branch
from
6df395e to
08186f6
Compare
|
I've tested this pr locally and am able to build both static and shared packages. Tree layout of the created packagesBuild logs: Please review. |
|
@madebr from #144 (comment) I take it, that Conan also reports the following in you build log: Other than that |
I'd suggest to stick to static builds, as this seems to be the expected default behavior in conan (see comment above). |
Jasper-Ben
left a comment
Jasper-Ben
left a comment
There was a problem hiding this comment.
other than the "controversal" default behavior of using shared packages, this looks good to me.
|
@Jasper-Ben |
3 participants
I made the changes you suggested in the PR, but
conan create . seasocks/1.4.4@fails during the test stage with: