CMake build script updates

[madebr]

Hello! As promised after our chat at cpplang.slack#conan, a pr for better conan support. I've not (yet) looked at Windows support.

What did I change?

• Removed some files + added them to .gitignore
• Added a .editorconfig file to make it easier for supported IDE's to apply formating. (See https://editorconfig.org/ for more info)
• Use FindPythonInterp for finding python + nicer (imho) generated embedded source (too bad btw that
• Add a SEASOCKS_SHARED and SEASOCKS_STATIC option to build shared and static libraries together. I left the possibility for building a static library without PIC support: see seasocks_obj vs seasock_pic_obj target.
• Small change to the internal header configuration: add @ONLY to configure to don't replace cmake type variables. This does not change anything, but is safer for the future.
• reformatted some cmake scripts. The cmake directory is appended to the CMAKE_MODULE_PATH so some module paths can be overridden by external scripts.
• Export the cmake targets in SeasocksTargets.cmake and generate a SeasockConfig.cmake. Also make sure to link to ZLIB::ZLIB and Threads::Threads to avoid paths of the build machine being present in the exported cmake scripts. Note that this requires a find_package(Threads REQUIRED) and find_package(ZLIB REQUIRED) in the generated SeasocksConfig.cmake.
• Remove double quotes from arguments in add_subdirectory. No functional change. But nicer imho.
• Copy the license on installation
• Add a conanfile build script. This should make it easier to build seasocks on Windows. Adding a conanfile.py to your repo allows the master branch in-tree, without doing releases. e.g.:

  mkdir build && cd build
  conan install ..
  conan build ..

  This also works on Windows. Using MSVC, you can then simply open Visual Studio using cmake --open. Also, I made sure to not patch the sources or include conanbuildinfo.cmake in your cmake scripts.

Cheers

[codecov[bot]]

Codecov Report

Merging #140 into master will decrease coverage by 0.01%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   37.53%   37.52%   -0.02%     
==========================================
  Files          52       52              
  Lines        2251     2268      +17     
==========================================
+ Hits          845      851       +6     
- Misses       1406     1417      +11     

Impacted Files	Coverage Δ
src/main/c/seasocks/Connection.h	16.66% <0.00%> (-11.91%)	:arrow_down:
src/main/c/seasocks/Server.h	0.00% <0.00%> (ø)
src/main/c/seasocks/Logger.h	11.11% <0.00%> (+11.11%)	:arrow_up:
src/main/c/seasocks/Request.h	100.00% <0.00%> (+100.00%)	:arrow_up:
src/main/c/seasocks/IgnoringLogger.h	100.00% <0.00%> (+100.00%)	:arrow_up:

---

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 b124f78...01bc83a. Read the comment docs.

[mattgodbolt]

CCing @offa for thoughts too: @offa the context is trying to get a canonical published build of seasocks into the conan ecosystem. A few examples of seasocks in conan exist, but for older versions and not "official" versions.

[mattgodbolt]

Thanks @madebr :) Sorry about the codecov thing. I wish I knew how to stop it being quite so opinionated...

[madebr]

Thanks @madebr :) Sorry about the codecov thing. I wish I knew how to stop it being quite so opinionated...

I used coveralls.io recently. Its settings has a Coverage Decrease Threshold for failure knob.

[mattgodbolt]

Looks like we lost the unittest:

0.01s$ cmake --build . --target unittest
make: *** No rule to make target 'unittest'.  Stop.

which kills all the CIs: if you could make a quick change to the .travis.yml to pick up the change (enough to get it passing, nothing more sophisticated) that would be ace. I think the rest is ready to go in though! Thanks so much!

[mattgodbolt]

Oh spooky! Just as I hit enter in GH I saw your checkin come in: thanks!

[offa]

CCing @offa for thoughts too: @offa the context is trying to get a canonical published build of seasocks into the conan ecosystem. A few examples of seasocks in conan exist, but for older versions and not "official" versions.

Great idea, it would be nice to get official seasock releases through conan center. I have opened an issue in their repository months ago (https://github.com/conan-io/conan-center-index/issues/726), but I'm not very experienced in packaging. #80 is related.

[madebr]

Thanks for merging. You can use the just-merged conanfile.py as a base for submission to cci. Some changes are required:

• use cci format to set the version
• only package shared or static, not both
• don't generate the cmake wrapper in build as I do in build but just create a cmake script with similar content (and add it to exports_sources)
• add a package_info method. You will probably need to add pthread to system_libs when seasocks is built as a static library

Of course, you will first need to do a release :smile:

[offa]

Doohh … few minutes to slow :facepalm:

[mattgodbolt]

Thanks both! I'm just discussing in Slack how best to get an official release done. Like @madebr says it will be the basis...

[mattgodbolt]

haha nps @offa we can fix these little minor issues in the main branch!

[mattgodbolt]

https://github.com/conan-io/conan/issues/4734 was just linked in the Slack and https://github.com/conan-io/conan-center-index/issues/22 too

[mattgodbolt]

So; let's get any late-breaking changes in and then plan to cut a release later? Then I'll try and baby sit the process of getting it into cci

[offa]

I'm not through the complete discussion, so sorry for the stupid question: But why is it necessary to include literally every target twice (static / shared)? Especially the tests don't make sense to me right now (… again, sorry this not-really-prepared question).

[madebr]

conan-io/conan#4734 was just linked in the Slack and conan-io/conan-center-index#22 too

https://github.com/conan-io/conan-center-index/issues/22#issuecomment-623970334 sums it up, I think

• in-source recipe for development
• CCI recipe for releases

[mattgodbolt]

The idea is to ensure CI builds et all build and link with both the static and shared library if configured. We can definitely consider changing this: it's possible we can adopt a more "build one or the other" approach too. I was scared to break backwards compatibility with people needing both, but maybe it's costing too much to support both simultaneously. @madebr mentioned a CMAKE setting to pick one or the other.

[mattgodbolt]

conan-io/conan#4734 was just linked in the Slack and conan-io/conan-center-index#22 too

conan-io/conan-center-index#22 (comment) sums it up, I think

• in-source recipe for development
• CCI recipe for releases

:+1:

Sounds good to me! Thanks all!

[madebr]

I'm not through the complete discussion, so sorry for the stupid question: But why is it necessary to include literally every target twice (static / shared)? Especially the tests don't make sense to me right now (… again, sorry this not-really-prepared question).

See https://github.com/mattgodbolt/seasocks/pull/140#discussion_r464383276 for our discussion Using BUILD_SHARED_LIBS would be simpler, but might break use cases.

[offa]

The idea is to ensure CI builds et all build and link with both the static and shared library if configured. We can definitely consider changing this: it's possible we can adopt a more "build one or the other" approach too. I was scared to break backwards compatibility with people needing both, but maybe it's costing too much to support both simultaneously. @madebr mentioned a CMAKE setting to pick one or the other.

Yeah, this would make sense. The current situation is a little … untypical … :smile:.

[mattgodbolt]

Ok cool! Unless someone has a burning urge I'll try and work out how to move to BUILD_SHARED_LIBS instead (sorry @madebr for making you go through the tricky process of supporting the other way :( ), and then I'll add a unsupported for macos too.

[madebr]

@mattgodbolt No problem. The changes are straightforward. You won't need to explicitly add a option(BUILD_SHARED_LIBS "Build seasocks as shared library") option. It's defined implicitly by cmake.

[offa]

If BUILD_SHARED_LIBS is problematic, we could use SEASOCKS_SHARED too.

[madebr]

If BUILD_SHARED_LIBS is problematic, we could use SEASOCKS_SHARED too.

It's not problematic at all. I wouldn't add a new option and just keep the default one to fit in with the rest.

[mattgodbolt]

If BUILD_SHARED_LIBS is problematic, we could use SEASOCKS_SHARED too.

It's not problematic at all. I wouldn't add a new option and just keep the default one to fit in with the rest.

Maybe I'm missing something here? Wouldn't having SEASOCKS_SHARED and BUILD_SHARED_LIBS be confusing? I suspect I now don't understand how build_shared_libs works :grin:

[madebr]

@mattgodbolt What BUILD_SHARED_LIBS does, is change the behavior of add_library. If BUILD_SHARED_LIBS is true, then a shared library is built. If it false, then a static library is built.

So only one library will be built, but the type depends on the option. Linking to that library will then allow linking to a shared and static library, but not at the same time (or in the same build directory).

Adding both options, would make it possible to build the shared library twice.

[mattgodbolt]

Understood. So why would we have SEASOCKS_SHARED too? That seems like it would potentially conflict with BUILD_SHARED_LIBS ?

[mattgodbolt]

Perhaps I misunderstand:

It's not problematic at all. I wouldn't add a new option and just keep the default one to fit in with the rest.

"the default one" => BUILD_SHARED_LIBS "a new option" => our existing SEASOCKS_SHARED stuff?

[madebr]

"the default one" => BUILD_SHARED_LIBS "a new option" => our existing SEASOCKS_SHARED stuff?

You could do that, but then you would duplicate cmake's behavior. I think this is only relevant when seasocks is included as a subproject, and you wnat seasocks to be different then the default.

option(SEASOCKS_SHARED "Build seasocks as a shared library" "${BUILD_SHARED_LIBS}")
if(SEASOCKS_SHARED)
    set(SEASOCKS_LIBTYPE "SHARED")
else()
    set(SEASOCKS_LIBTYPE "STATIC")
endif()

and use it as:

add_library(seasocks ${SEASOCK_LIBTYPE} a.cpp b.cpp c.cpp)

[mattgodbolt]

Perfect! I get it now! Thanks yet again :)

[madebr]

I think you can remove the POSITION_INDEPENDENT_CODE things littered around the code, or at least only set this property when SEASOCKS_SHARED is true.

[mattgodbolt]

I think you can remove the POSITION_INDEPENDENT_CODE things littered around the code, or at least only set this property when SEASOCKS_SHARED is true.

I disagree here: the original use case of seasocks was embedding it (statically) in a shared library... fPIC being independent of "build a shared object" is really important :)

[mattgodbolt]

(plus folks building executables with -fPIE need it too)

[madebr]

You can use CMAKE_POSITION_INDEPENDENT_CODE for that. That is the default value for the POSITION_INDEPENDENT_CODE property.

Alternatively, you can also provide a SEASOCKS_PIC option.

[mattgodbolt]

Oh lovely! There's a CMakeful solution for everything: thanks!

[mattgodbolt]

I'm tying myself in knots rather here. Will put a PR together which will hopefully be 90% there and then I'd love your help getting it over the finish line.

[madebr]

No problem, the changes shouldn't be that big.

[mattgodbolt]

So far one weird cmake thing and one conan thing. Might have to PR with them in and ask for your help. Will spend another 10m on it first :)