Added cmake file and partial testing support. #3

FrozenSource · 2021-11-22T21:21:27Z

Hi,

I love the idea of your library and since I would love adding it as a vendor I would like to suggest adding a CMake file.
I also attempted to add doctest, but was unable to due to some private libraries which I couldn't find in your public repos.

Sincerely,
FrozenSource

FrozenSource · 2021-11-22T21:23:53Z

As I saw from #2 this CMake file will also work on MacOS/linux as long as you use at least GCC-11.11 from brew.

s9w · 2021-11-22T21:38:32Z

Hey! I was expecting the c-word to come up :D

And thanks for providing the code! I'll look into this tomorrow ok? Getting a bit late here.

For the tests there should only the doctest header be missing in the repo? The demos use the yet unreleased s9w libs, but maybe they're not so interesting to include into the cmake stuff?

FrozenSource · 2021-11-22T22:11:44Z

Probably the same time here :D

I have only worked with gtest before so I am not familiar with how doctest works. I used a little snippet to automatically clone it into the vendor and make it build/link in the intermediates. This would depend on how state-full you want the commits to be. In practicality I made it so that Vendor would have the current clone and the sources of those vendors would be pushed too if not in the ignore. This way it isn't a sub module (as long as the lib isn't that big) and you can compile it later too.

For the demos it would depend on how far you want to take this... if using a build server, pipelines, or whatever you might want to test builds for all oses.

I take it you use either Visual Studio 2019/2022 which have a CMake extension so you could go CMake all the way but that's up to you. I am happy as long as I can link it in my things :P

The way I use CMake is also as a library manager. If possible I don't want any find_package things that people would need to install beforehand. If I were you I would just push that s9w-helper-library in the repo for now and should you separate it push that to a different repo and add it in the CMake as a vendor. Same goes for the doctest which I think should just be a clone/submodule however you implement it for compatibility reasons.

With this CMake file setup anybody can clone and doesn't need anything but CMake and git installed.

friendlyanon

Lots of mistakes, errors and missing things in the CML file. Please take a look at this pending PR for how to write a minimally viable CML for a header-only library that also supports vendoring hanickadot/compile-time-regular-expressions#230

friendlyanon · 2021-11-24T22:23:06Z

CMakeLists.txt

@@ -0,0 +1,54 @@
+cmake_minimum_required(VERSION 3.16)


This requirement is unnecessarily high. There is no feature used in the CMake scripts that require this version. A header-only project can very comfortably use 3.14 at the lowest.

You're right I took this from another one.... thought FetchContent was quite high but apparently its only 11.

MakeAvailable is available since 3.14, but it's really only a convenience function wrapping Populate and add_subdirectory. I would say you should stick to 3.14 regardless, because it has better support for header-only libraries (DESTINATION is not mandatory in target install rules, ARCH_INDEPENDENT).

friendlyanon · 2021-11-24T22:25:10Z

CMakeLists.txt

+set(CMAKE_C_STANDARD 99)
+set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_POSITION_INDEPENDENT_CODE ON)
+option(SANITIZE_ADDRESS "Address Sanitizer." OFF)
+
+if (SANITIZE_ADDRESS)
+set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fno-omit-frame-pointer -fsanitize=address")
+set (CMAKE_LINKER_FLAGS_DEBUG "${CMAKE_LINKER_FLAGS_DEBUG} -fno-omit-frame-pointer -fsanitize=address")
+endif()


These don't really belong in a project CML. These should be externalized to a preset, like this and this.

I'll admit it was a mistake to make them global in this case.
Wouldn't this do the job? Since CXX 20 is required for this header. Haven't worked with presets will take a look at that option too.

set_property(TARGET ${This} PROPERTY C_STANDARD 99)
set_property(TARGET ${This} PROPERTY CXX_STANDARD 20)
if (SANITIZE_ADDRESS)
set_property (TARGET ${This} PROPERTY CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fno-omit-frame-pointer -fsanitize=address")
set_property (TARGET ${This} PROPERTY CMAKE_LINKER_FLAGS_DEBUG "${CMAKE_LINKER_FLAGS_DEBUG} -fno-omit-frame-pointer -fsanitize=address")
endif()

CMake has compile features to indicate standard requirement. Everything else is an external concern, a social requirement if you will. Project CMLs should only be concerned with build and usage requirements.

friendlyanon · 2021-11-24T22:27:46Z

CMakeLists.txt

+include(FetchContent)
+
+if(NOT InstallVendor)
+function(InstallVendor project_name git_repo branch)
+        IF (NOT ${project_name})
+                FetchContent_Declare(
+                        ${project_name}
+                        GIT_REPOSITORY ${git_repo}
+                        GIT_TAG ${branch}
+                        SOURCE_DIR "${PROJECT_SOURCE_DIR}/vendor/${project_name}"
+                        BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/${project_name}"
+                        SUBBUILD_DIR "${CMAKE_CURRENT_BINARY_DIR}/${project_name}-sub"
+                )
+                FetchContent_MakeAvailable(${project_name})
+                set(${project_name} CACHE INTERNAL "")
+                message(STATUS "Found ${project_name} @ ${branch}")
+        endif()
+endfunction()
+endif()


This function is completely unnecessary. FetchContent already has built-in mechanisms that allows an unguarded Declare followed by a MakeAvailable to Just Work™. Fetching test dependencies, building tests and running tests however is not a top level concern and should be moved to an opt-in codepath. If oyu consider the earlier comment w.r.t. presets, this could be made default for the developer workflow.

Doing it as a function allows for nesting from multiple dependencies that's why it is like this and has worked for many years. When declaring a second time even though we already have it the CMake blocks with an obvious error last time I checked.

Nesting dependencies? Makes no sense. This function just duplicates logic from FC. The if(NOT InstallVendor) is also wrong, since it checks for a variable and not a command.

friendlyanon · 2021-11-24T22:28:22Z

CMakeLists.txt

+endfunction()
+endif()
+
+set(This oof)


Come on, this is completely unnecessary and it's just more characters than spelling oof out directly.

I will not agree with you on that as it is seen in pretty much every CMake file and makes things easy and consistent. Should the CMake file be in here in makes things easier.
I'll leave the end decision to @s9w unless you would agree.

seen in pretty much every CMake file

Pretty much every CMake script is composed of oodles of spaghetti for all the wrong reasons. Let's not copy that if we can.

friendlyanon · 2021-11-24T22:29:28Z

CMakeLists.txt

+endif()
+
+set(This oof)
+project(${This} LANGUAGES C CXX)


A header-only project doesn't need any languages enabled at top-level, because it doesn't build anything, so it should be set to NONE.

At the moment it has no header file but as you have seen we might need some impl C files which make it easier for the project setup in the end. Since we are required to set a Language I am not going to set it to something that it isn't.

You can enable languages in subdirectories just fine. Check this project.

friendlyanon · 2021-11-24T22:35:35Z

CMakeLists.txt

+
+file(GLOB_RECURSE TEST_SOURCES ${PROJECT_SOURCE_DIR}/tests/**.cpp)
+
+add_library(${This} INTERFACE)


To allow vendoring this project, the target name should have a project specific prefix which is stripped in the install interface. You can find such an example here.

I thought this was quite a common way of doing it? But you might be right in that there is no reason for namespaces in that case. Will do.

If you check Craig Scott's Deep CMake talk slides, he suggests the same at the end. He didn't have time to mention it in the talk though.

friendlyanon · 2021-11-24T22:40:46Z

CMakeLists.txt

+
+add_library(${This} INTERFACE)
+add_library(${This}::${This} ALIAS ${This})
+target_compile_definitions(${This} INTERFACE OOF_IMPL)


This isn't correct. The definition will affect every file this target is linked to, which will result in ODR violations. This definition is ought to be set by the consuming project using code such as:

set_property(SOURCE source/oof_impl.cpp PROPERTY COMPILE_DEFINITIONS OOF_IMPL)

In other words I'l need to change the sources for this one way or another. Since we don't have a cpp file but want to keep it oof as an isolated lib. I don't want external users to have to do this externally.

friendlyanon · 2021-11-24T22:42:59Z

CMakeLists.txt

+add_library(${This} INTERFACE)
+add_library(${This}::${This} ALIAS ${This})
+target_compile_definitions(${This} INTERFACE OOF_IMPL)
+target_include_directories(${This} INTERFACE ${PROJECT_SOURCE_DIR})


This should be wrapped in a $<BUILD_INTERFACE:...> genex with a conditional SYSTEM modifier if making this project vendoring friendly is a goal. See review comment for example.

At the moment I didn't write any INSTALL-code and as such this isn't a problem right now. Guess I'll add that.

friendlyanon · 2021-11-24T22:44:03Z

CMakeLists.txt

+#add_executable(${This}-demo ${DEMO_HEADERS} ${DEMO_SOURCES})
+#add_executable(${This}-test ${TEST_SOURCES})
+#target_link_libraries(${This}-test PRIVATE doctest::doctest)


Demos and tests should be opt-in and be in a subdirectory CML.

Never heard of this CML directory; please elaborate.
You're right about the opt-in but the test/demo code failed either way so didn't do that much work for it.
Purpose of this pull-request was to improve in more things.

CML is just shorthand for CMakeLists.txt. The reason why they should be in a subdirectory is because they are not a top-level concern. Someone who just wants to install the project should not be bothered with tests and demos. cmake -B build && cmake --install build should just work.

friendlyanon · 2021-11-24T22:48:47Z

tests/include_doctest.h

+#pragma once
+#define DOCTEST_CONFIG_IMPLEMENT
+#if __has_include(<doctest.h>)
+#include <doctest.h>
+#else
+#include "doctest.h"
+#endif


All of this is not really necessary. You can just #include <doctest.h> and add the define(s) you want for doctest to one of the test source files:

set_property(SOURCE tests/tests.cpp PROPERTY COMPILE_DEFINITIONS DOCTEST_CONFIG_IMPLEMENT)

Nothing to add to that.

s9w · 2021-11-25T07:34:11Z

Hey! A couple of thoughts.

First of all, this repo is really three parts: The library header, the tests and the demos. The tests aren't really all that comprehensive. And the demos lack the s9w library to even compile. While that could be fixed, I'm not sure if they even should be part of this cmake thing.

So how about we limit the cmake integration to the actual library? It can't possibly be that hard to write a cmake file for a single file?

Also, some people in the reddit thread brought up their dislike of this "single header with implementation macro" style of code. One way to make everyone happy could be to add an oof.cpp with these contents:

#define OOF_IMPL
#include <oof.h>

That would be optional then.

Thoughts? Not sure how that would play into cmake integration.

FrozenSource · 2021-11-25T07:58:58Z

Hey! A couple of thoughts.

First of all, this repo is really three parts: The library header, the tests and the demos. The tests aren't really all that comprehensive. And the demos lack the s9w library to even compile. While that could be fixed, I'm not sure if they even should be part of this cmake thing.

So how about we limit the cmake integration to the actual library? It can't possibly be that hard to write a cmake file for a single file?

Also, some people in the reddit thread brought up their dislike of this "single header with implementation macro" style of code. One way to make everyone happy could be to add an oof.cpp with these contents:
#define OOF_IMPL
#include <oof.h>
That would be optional then.

Thoughts? Not sure how that would play into cmake integration.

Completely agree with the oof.cpp which might be better. Or do it like the std and template/inline it all.

We could leave out the demos and tests; but it would be nice to have them on a OSS-repo. Otherwise there was no reason for pushing them and as such should be in the CMake when ready. Will leave this out for now.

s9w · 2021-11-25T11:17:14Z

"OSS" meaning opensource in this context?

I think the demo code has value even if it doesn't compile. It would only compile on windows, anyways. But I'm not opposed to include those s9w headers if people want to.

friendlyanon · 2021-11-25T13:07:31Z

"Optional" anything doesn't usually play nice with packaging, since with each option you double the configuration space. What you could do is make the CMake scripts offer just a static only library. People who are content with copying headers around will still have the option and those who don't want to deal with that will get a nice static library already compiled to their liking. This would require adding an additional ifndef to the existing one to prevent accidental ODR violations.

#ifndef OOF_STATIC_LIBRARY
#ifdef OOF_IMPL
...
#endif
#endif

Then the CMake code could set that define for the target and all is good.

Added cmake file and partial testing support.

cb785b5

friendlyanon suggested changes Nov 24, 2021

View reviewed changes

Merge branch 's9w:master' into master

51a266d


		file(GLOB_RECURSE TEST_SOURCES ${PROJECT_SOURCE_DIR}/tests/**.cpp)

		add_library(${This} INTERFACE)

Added cmake file and partial testing support. #3

Are you sure you want to change the base?

Added cmake file and partial testing support. #3

Conversation

FrozenSource commented Nov 22, 2021

FrozenSource commented Nov 22, 2021

s9w commented Nov 22, 2021

FrozenSource commented Nov 22, 2021

friendlyanon 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

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

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

friendlyanon Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s9w commented Nov 25, 2021

FrozenSource commented Nov 25, 2021

s9w commented Nov 25, 2021

friendlyanon commented Nov 25, 2021 • edited Loading

friendlyanon Nov 25, 2021 •

edited

Loading

friendlyanon commented Nov 25, 2021 •

edited

Loading