CMake build script updates #140

madebr · 2020-08-03T05:15:34Z

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

… build

…ig.cmake

codecov · 2020-08-03T05:24:25Z

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%)`	⬇️
src/main/c/seasocks/Server.h	`0.00% <0.00%> (ø)`
src/main/c/seasocks/Logger.h	`11.11% <0.00%> (+11.11%)`	⬆️
src/main/c/seasocks/Request.h	`100.00% <0.00%> (+100.00%)`	⬆️
src/main/c/seasocks/IgnoringLogger.h	`100.00% <0.00%> (+100.00%)`	⬆️

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.

… enabled

mattgodbolt

Thanks so much for doing this. Some comments and questions, mostly about my ignorance of CMake, conan and general editor configuration.

.gitignore

CMakeLists.txt

cmake/Config.h.in

cmake/SeasocksConfig.cmake.in

conanfile.py

mattgodbolt · 2020-08-03T12:29:56Z

conanfile.py

@@ -0,0 +1,82 @@
+from conans import CMake, ConanFile, tools


I'm happy to have this here, but forgive my ignorance: how might this work to get seasocks in conan build center?

Also it looks like we're missing a test_package -- have you been able to test that this seasocks build works?

This won't help at all for getting seasocks in cci, because it doesn't follow cci practices.
Also, in my own experience, using a conanfile makes it easier to test on ci.
If you ever lose your mind and want to add support for Windows, then you can use it to build on appveyor.
A simple conan install.. && conan build .. & conan test .. should work™ .

In theory, now you can add a job on travis that uses this conanfile.
The test() method can be used to run the unit tests.
I haven't added a test_package, because the intention of this recipe is not to package seasocks, but to allow it to build (and test).

I see. I can't see that we'd ever use conan to build seasocks in this case (it has so few upstream dependencies). My projects that do have upstream deps do use conan, but I don't want to rely on it here. However, that doesn't mean this isn't super useful!

We will definitely add a conan test job once this is working, how would you suggest doing so with your changes here?

conan install.. && conan build .. & conan test ..

Aside: I do so hate the multi-stage aspect of conan. In all my experience, if there are manual steps, then people will forget them! In my own projects I always use the conan.cmake file to make conan part of the build itself...I've spent too long debugging things like "oh, you forgot to conan install:? You're probably building against the old libraries...argh!")

This requires adding new jobs, that will run the conan commands.
I would add a CONAN_INSTALL_ARGS environment variable to .travis.yml.

Aside: I do so hate the multi-stage aspect of conan. In all my experience, if there are manual steps, then people will forget them! In my own projects I always use the conan.cmake file to make conan part of the build itself...I've spent too long debugging things like "oh, you forgot to conan install:? You're probably building against the old libraries...argh!")

It's a matter of taste I guess.
I always try to make my project package manager agnostic.
This allows me to build with the libraries of a linux package manager.

(Matt accidentally edited this comment; I hope I've un-edited back appropriate)

Oops! I meant to reply!!! Not edit your comment 😊

I tried to say:

This requires adding new jobs, that will run the conan commands.
I would add a CONAN_INSTALL_ARGS environment variable to .travis.yml.

I get that, just quickly, how might one actually build seasocks using conan? conan install .. && conan build .. && conan test .. like you mentioned above? Would that actually run the tests? Thanks again for teaching me this.

I always try to make my project package manager agnostic.
Got it: that makes sense. In my world having the one way that just works and is reproducible is most important, but I have the luxury of mostly getting to choose this within my team etc

I get that, just quickly, how might one actually build seasocks using conan? conan install .. && conan build .. && conan test .. like you mentioned above? Would that actually run the tests? Thanks again for teaching me this.

I was wrong there. The intended use of conan test is only to test conan packages.
What can be done is use the virtualenv generator. The tests can then be run from any script (bash/python/ctest/...). The virtualenv makes sure that paths of all dependencies are in the search path (LD_LIBRARY_PATH/PATH/DYLD_LIBRARY_PATH/...). This is especially required for shared library dependencies.

I always try to make my project package manager agnostic.
Got it: that makes sense. In my world having the one way that just works and is reproducible is most important, but I have the luxury of mostly getting to choose this within my team etc

Check the conanfile.py of thie recipe. It writes a wrapper CMakeLists.txt in the build directory that includes the source directory. It is more or less the inverse approach as your conan.cmake. (conan.cmake's driver is cmake. In this pr, the driver is conan)

I'll try and work through that once we're merged in. I'm not quite following but I imagine trying to do it will elucidate more than reading dry documentation.

conanfile.py

src/app/c/CMakeLists.txt

mattgodbolt · 2020-08-03T12:32:51Z

src/CMakeLists.txt

+if(SEASOCKS_SHARED)
+    add_subdirectory(main/web)
+
+    if (SEASOCKS_EXAMPLE_APP)


Why is the example app only built against the shared library? It should work against the lib too? (unless I'm missing something)

The cmake script currently builds both shared and static libraries, which can be disabled.
Because the examples+tests link to the shared library, this dependency might be missing
I'll take another look at this and add a new option: SEASOCKS_APPS_USE_SHARED.

No need to make things more complex: I was just trying to understand :)

Still, this is a consequence of not using cmake's BUILD_SHARED_LIBS.
Because right now, the static library is built, but not used/tested anywhere.
This is a potential bug.

Thanks @madebr

What option do you prefer?

Add a SEASOCKS_APPS_USE_SHARED that lets the user choose to what type of library the examples/tests link. This has the potential for people forgetting to test linking to the static library.

Add the examples/tests twice.
The examples/tests can be built once, but linked twice.

I'm not worried either way: I doubt there are many categories of errors which mean that static libs work when dynamic don't. That said; if it's not much work then I'd prefer the second option, but the first works too if the CI system matrix is expanded to build with it on and off.

I've applied the second approach done this in the latest commit.
Most of the additional complexity comes from us building both shared and static libraries (as opposed to using cmake's BUILD_SHARED_LIBS).

When configuring cmake as cmake .. -DCMAKE_POSITION_INDEPENDENT_CODE=ON, the number of source compilatations should be equal, but the number of linkings should be doubled.

mattgodbolt · 2020-08-03T13:23:00Z

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.

This reverts commit 083bb0c.

mattgodbolt · 2020-08-03T22:16:37Z

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

…ed to shared

offa · 2020-08-04T14:56:39Z

CMakeLists.txt

 project(Seasocks VERSION 1.4.3)

+if(WIN32)


Maybe Mac OS should be included too? Test for not-linux?

Indeed, test should include APPLE

offa · 2020-08-04T14:58:16Z

CMakeLists.txt

 endif ()

 find_package(Threads)
+find_package(PythonInterp)


Nice, didn't know that package! 👍

It's superseded by FindPython3, but that's not available on older cmake releases.

offa · 2020-08-04T15:05:07Z

src/main/c/CMakeLists.txt

-    target_link_libraries(seasocks_so PRIVATE ${CMAKE_THREAD_LIBS_INIT} "${ZLIB_LIBRARIES}")
-endif()
-set_target_properties(seasocks_so PROPERTIES OUTPUT_NAME seasocks VERSION ${PROJECT_VERSION})
+if (SEASOCKS_SHARED)


Since the main difference between the static / shared part is target name, they can be extracted into a macro I guess. This would reduce the code duplication.

Agreed.
I didn't because I wasn't sure you would agree with the complexity I was adding.

offa · 2020-08-04T15:08:45Z

Doohh … few minutes to slow 🤦

mattgodbolt · 2020-08-04T15:08:53Z

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 · 2020-08-04T15:09:20Z

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

mattgodbolt · 2020-08-04T15:09:47Z

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

mattgodbolt · 2020-08-04T15:10:46Z

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 · 2020-08-04T15:12:56Z

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 · 2020-08-04T15:14:31Z

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

mattgodbolt · 2020-08-04T15:14:57Z

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 · 2020-08-04T15:15:19Z

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

👍

Sounds good to me! Thanks all!

madebr · 2020-08-04T15:18:05Z

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 #140 (comment) for our discussion
Using BUILD_SHARED_LIBS would be simpler, but might break use cases.

offa · 2020-08-04T15:18:52Z

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 … 😄.

mattgodbolt · 2020-08-04T15:20:07Z

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 · 2020-08-04T15:21:27Z

@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 · 2020-08-04T15:22:12Z

If BUILD_SHARED_LIBS is problematic, we could use SEASOCKS_SHARED too.

madebr · 2020-08-04T15:24:41Z

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 · 2020-08-04T15:30:33Z

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 😁

madebr · 2020-08-04T15:35:51Z

@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 · 2020-08-04T15:52:14Z

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

mattgodbolt · 2020-08-04T15:52:59Z

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 · 2020-08-04T15:58:28Z

"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 · 2020-08-04T15:59:04Z

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

madebr · 2020-08-04T16:06:19Z

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 · 2020-08-04T16:15:19Z

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 · 2020-08-04T16:15:47Z

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

madebr · 2020-08-04T16:18:40Z

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 · 2020-08-04T16:21:47Z

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

mattgodbolt · 2020-08-04T16:54:37Z

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 · 2020-08-04T16:59:25Z

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

mattgodbolt · 2020-08-04T17:02:01Z

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

madebr added 11 commits August 3, 2020 06:50

Remove IDE project files + add to .gitignore

083bb0c

Add .editorconfig file to automatically apply styles

486c1a7

Add build directories to .gitignore

82b3854

Use FindPythonInterp to look for python + create nicer embedded source

8d994a1

Add SEASOCKS_SHARED and SEASOCKS_STATIC option + allow non-PIC static…

682db24

… build

Generate internal/Config.h file with @only

aae4195

Reformat cmake scripts

a3704c5

Export cmake targets in SeasocksTargets.cmake + generate SeasocksConf…

008d195

…ig.cmake

Remove double quotes from add_subdirectory arguments

abcf1da

Install license

57302ab

Add conanfile build script

8511eb4

madebr added 5 commits August 3, 2020 07:26

Fix conanfile.py for use on Windows

0ba88d0

Fix compile options for MSVC

f49f674

Object library also needs to include directories

40524c1

Fix embedded source generator for Python3

9b1397f

Only add ZLIB's include directories to object library when deflate is…

ae8c75a

… enabled

This was referenced Aug 3, 2020

Unable to build on Visual Studio 2017 #141

Closed

Unable to build using mingw on Windows #142

Closed

fPIC does not exist on Windows

0f5aeb3

mattgodbolt reviewed Aug 3, 2020

View reviewed changes

mattgodbolt requested a review from offa August 3, 2020 13:21

madebr added 4 commits August 3, 2020 22:53

Fix license of conanfile

7e6a8b3

Fail with a fatal error when building on Windows

797f4a8

Revert "Remove IDE project files + add to .gitignore"

29a0119

This reverts commit 083bb0c.

Fix bad grammar in error message

5336cfa

madebr added 2 commits August 4, 2020 00:45

Tests and exemples were linked to static libraries by default

8706615

Create all executables in 2 forms: linked to static seasocks and link…

42db87d

…ed to shared

offa reviewed Aug 4, 2020

View reviewed changes

CMake build script updates #140

CMake build script updates #140

Conversation

madebr commented Aug 3, 2020 • edited Loading

codecov bot commented Aug 3, 2020 • edited Loading

Codecov Report

mattgodbolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madebr Aug 3, 2020 • edited by mattgodbolt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattgodbolt commented Aug 3, 2020

mattgodbolt commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

offa commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

offa commented Aug 4, 2020

madebr commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020 • edited Loading

offa commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020

offa commented Aug 4, 2020 • edited Loading

madebr commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020 • edited Loading

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020 • edited Loading

mattgodbolt commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 4, 2020

mattgodbolt commented Aug 4, 2020

madebr commented Aug 3, 2020 •

edited

Loading

codecov bot commented Aug 3, 2020 •

edited

Loading

madebr Aug 3, 2020 •

edited by mattgodbolt

Loading

madebr commented Aug 4, 2020 •

edited

Loading

offa commented Aug 4, 2020 •

edited

Loading

madebr commented Aug 4, 2020 •

edited

Loading

madebr commented Aug 4, 2020 •

edited

Loading