Address @madebr post-checkin comments. Doesn't currently build

[mattgodbolt]

I made the changes you suggested in the PR, but conan create . seasocks/1.4.4@ fails during the test stage with:

[HOOK - conan-center.py] pre_build(): [FPIC MANAGEMENT (KB-H007)] 'fPIC' option not found
[HOOK - conan-center.py] pre_build(): [FPIC MANAGEMENT (KB-H007)] OK
seasocks/1.4.4 (test package): Calling build()
-- The CXX compiler identification is GNU 9.3.0
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Conan: called by CMake conan helper
-- Conan: Adjusting output directories
-- Conan: Using cmake targets configuration
-- Library seasocks found /home/mgodbolt/.conan/data/seasocks/1.4.4/_/_/package/f55ba25bd602c823ae2a588a3723c6f03570ff59/lib/libseasocks.so
-- Library z found /home/mgodbolt/.conan/data/zlib/1.2.11/_/_/package/6af9cc7cb931c5ad942174fd7838eb655717c709/lib/libz.a
-- Conan: Adjusting default RPATHs Conan policies
-- Conan: Adjusting language standard
-- Conan: Compiler GCC>=5, checking major version 9
-- Conan: Checking correct version: 9
-- Conan: C++ stdlib: libstdc++
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_EXPORT_NO_PACKAGE_REGISTRY

-- Build files have been written to: /home/mgodbolt/dev/personal/seasocks/test_package/build/7e3f3c9f8b0319ca5c01315759aeda2f877da74f
Scanning dependencies of target seasocks_test
[ 50%] Building CXX object CMakeFiles/seasocks_test.dir/seasocks_test.cpp.o
[100%] Linking CXX executable bin/seasocks_test
/usr/bin/ld: CMakeFiles/seasocks_test.dir/seasocks_test.cpp.o: undefined reference to symbol 'pthread_create@@GLIBC_2.2.5'
/usr/bin/ld: /lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/seasocks_test.dir/build.make:85: bin/seasocks_test] Error 1
make[1]: *** [CMakeFiles/Makefile2:76: CMakeFiles/seasocks_test.dir/all] Error 2
make: *** [Makefile:84: all] Error 2
ERROR: seasocks/1.4.4 (test package): Error in build() method, line 12
    cmake.build()
    ConanException: Error 2 while executing cmake --build '/home/mgodbolt/dev/personal/seasocks/test_package/build/7e3f3c9f8b0319ca5c01315759aeda2f877da74f' '--' '-j36'

[madebr]

@mattgodbolt Only the changes below are needed. After these changes: conan create . and conan create . -o seasocks:shared=True work for me :tm: .

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

[mattgodbolt]

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 "pthread" line:

+                self.cpp_info.components["libseasocks"].system_libs = ["pthread"]

I don't understand why we'd need to specify it in two places?

[mattgodbolt]

The only change I needed to make was to add "cmake_find_package"! Yay! thanks

[madebr]

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!)

Yeah, it is not needed but I added it to be more explicit.

But! I'll add the threads requried...why is that needed though? Your original comment removed it and added the "pthread" line:

+                self.cpp_info.components["libseasocks"].system_libs = ["pthread"]

I don't understand why we'd need to specify it in two places?

seasocks uses pthread internally, so needs to link to it. But a shared library already links to pthread. What I did here was make the pthread system library dependency public only when seasocks is built as a static library.

[madebr]

Also, using SEASOCKS_SHARED explicitly, lets me bypass the bug in release 1.4.4

[codecov[bot]]

Codecov Report

Merging #144 (768da76) into master (90239b0) will not change coverage. The diff coverage is n/a.

@@           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.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 90239b0...768da76. Read the comment docs.

[mattgodbolt]

Also, using SEASOCKS_SHARED explicitly, lets me bypass the bug in release 1.4.4

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.

[madebr]

@mattgodbolt

Only these changes need to made. Then the in-tree recipe works equally well as the cci recipe.

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

[mattgodbolt]

Oh my, I think we got everything working OK in the end? I realise this is still open..but I /think/ we're good?

[madebr]

@mattgodbolt This pr + my suggestions from https://github.com/mattgodbolt/seasocks/pull/144#issuecomment-674210391 will make your test_package equal to the cci test_package.

[madebr]

@Jasper-Ben @offa

I've tested this pr locally and am able to build both static and shared packages. I also rebased on top of current master.

Tree layout of the created packages

``` /home/maarten/.conan/data/seasocks/1.4.4/_/_/package/ ├── 0ce8c5e7be2494c16e753be40082218e1bfda37c │ ├── conaninfo.txt │ ├── conanmanifest.txt │ ├── include │ │ └── seasocks │ │ ├── Connection.h │ │ ├── Credentials.h │ │ ├── IgnoringLogger.h │ │ ├── internal │ │ │ └── Config.h │ │ ├── Logger.h │ │ ├── PageHandler.h │ │ ├── PrintfLogger.h │ │ ├── Request.h │ │ ├── ResponseBuilder.h │ │ ├── ResponseCodeDefs.h │ │ ├── ResponseCode.h │ │ ├── Response.h │ │ ├── ResponseWriter.h │ │ ├── Server.h │ │ ├── ServerImpl.h │ │ ├── SimpleResponse.h │ │ ├── StreamingResponse.h │ │ ├── StringUtil.h │ │ ├── SynchronousResponse.h │ │ ├── ToString.h │ │ ├── TransferEncoding.h │ │ ├── util │ │ │ ├── CrackedUri.h │ │ │ ├── CrackedUriPageHandler.h │ │ │ ├── Html.h │ │ │ ├── Json.h │ │ │ ├── PathHandler.h │ │ │ ├── RootPageHandler.h │ │ │ └── StaticResponseHandler.h │ │ ├── WebSocket.h │ │ └── ZlibContext.h │ ├── lib │ │ ├── cmake │ │ │ └── Seasocks │ │ │ ├── SeasocksConfig.cmake │ │ │ ├── SeasocksConfigVersion.cmake │ │ │ ├── SeasocksTargets.cmake │ │ │ └── SeasocksTargets-release.cmake │ │ └── libseasocks.a │ └── share │ └── licenses │ └── Seasocks │ └── LICENSE └── 2811898c1d9d712f569faeb54fb22c8ca99804f5 ├── conaninfo.txt ├── conanmanifest.txt ├── include │ └── seasocks │ ├── Connection.h │ ├── Credentials.h │ ├── IgnoringLogger.h │ ├── internal │ │ └── Config.h │ ├── Logger.h │ ├── PageHandler.h │ ├── PrintfLogger.h │ ├── Request.h │ ├── ResponseBuilder.h │ ├── ResponseCodeDefs.h │ ├── ResponseCode.h │ ├── Response.h │ ├── ResponseWriter.h │ ├── Server.h │ ├── ServerImpl.h │ ├── SimpleResponse.h │ ├── StreamingResponse.h │ ├── StringUtil.h │ ├── SynchronousResponse.h │ ├── ToString.h │ ├── TransferEncoding.h │ ├── util │ │ ├── CrackedUri.h │ │ ├── CrackedUriPageHandler.h │ │ ├── Html.h │ │ ├── Json.h │ │ ├── PathHandler.h │ │ ├── RootPageHandler.h │ │ └── StaticResponseHandler.h │ ├── WebSocket.h │ └── ZlibContext.h ├── lib │ ├── cmake │ │ └── Seasocks │ │ ├── SeasocksConfig.cmake │ │ ├── SeasocksConfigVersion.cmake │ │ ├── SeasocksTargets.cmake │ │ └── SeasocksTargets-release.cmake │ └── libseasocks.so └── share └── licenses └── Seasocks └── LICENSE 22 directories, 76 files ```

Build logs: log_static.txt and log_shared.txt

Please review.

[Jasper-Ben]

@madebr from https://github.com/mattgodbolt/seasocks/pull/144#issuecomment-670523298 I take it, that conan create . should default to a static package? This would also be the default behavior in the previous release.

Conan also reports the following in you build log: [HOOK - conan-center.py] post_export(): ERROR: [DEFAULT SHARED OPTION VALUE (KB-H050)] The option 'shared' must be 'False' by default. Update 'default_options'. (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H050)

Other than that conan create . and conan create . -o seasocks:shared=False work as intended on my machine.

[Jasper-Ben]

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 "pthread" line:

+                self.cpp_info.components["libseasocks"].system_libs = ["pthread"]

I don't understand why we'd need to specify it in two places?

I'd suggest to stick to static builds, as this seems to be the expected default behavior in conan (see comment above).

[madebr]

@Jasper-Ben I changed the default to static libraries. So there's (almost) no difference in behavior between cci and here.