added initial cmakelists.txt file. #17
Conversation
There was a problem hiding this comment.
I like the idea of adding a decent build system. What I don't like however, is the complexity of yours. Build systems are difficult enough to get right and to understand. Your reliance on Hunter seems to add a lot of extra required knowledge without any clear benefit over plain CMake.
I had already written my own CMakeLists.txt for this last week. It contains warning detection and color-enabling for Ninja and still is only 98 lines. I've pushed it to my private branch as 4c6f6e1. Please tell me what you think your PR offers that the simple approach doesn't.
| # | ||
|
|
||
| project(ImplPtr) | ||
| set(CMAKE_CXX_STANDARD 14) |
There was a problem hiding this comment.
- This is wrong: right now C++11 can be used for building
- This fixes it to a specific version instead of requiring a minimal version. Try
target_compile_features(impl_ptr INTERFACE cxx_std_11)instead (or better: the list of C++11 features we use).
| cmake_policy(VERSION 3.8) | ||
|
|
||
| ############################################### | ||
| # NOTE FOR THOSE UNFORTUNATES WHO DON'T KNOW OF HUNTER |
There was a problem hiding this comment.
This seems like a lot of overkill for just getting Boost. Not to mention that "those unfortunates" is a rather condescending tone to take. I get the impression from this text that you just like hunter a lot and as a result it's become your hammer for which every problem needs to be a nail.
Could you explain why find_package(Boost REQUIRED ${MIN_VERSION}) isn't enough? That approach will work with a system-installed boost, which I doubt this Hunter thing does as from the little info I've gleaned about it it's basically it's own package manager.
|
|
||
| add_library(impl_ptr INTERFACE) | ||
| target_include_directories(impl_ptr SYSTEM INTERFACE | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include> |
There was a problem hiding this comment.
There are no generated sources, so adding the binary dir is not necessary.
| add_library(impl_ptr INTERFACE) | ||
| target_include_directories(impl_ptr SYSTEM INTERFACE | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include> | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>) |
There was a problem hiding this comment.
You're adding a BUILD_INTERFACE, why not an INSTALL_INTERFACE?
There was a problem hiding this comment.
I Haven't got round to the that bit yet. The first commit was simply to get this building and running tests on any platform without having to install any dependencies.
| if (HUNTER_ENABLED) | ||
| sugar_include(test) | ||
| else () | ||
| find_package(Boost REQUIRED) |
There was a problem hiding this comment.
Why do you repeat the package detection that you've already done above?
There was a problem hiding this comment.
That's a harmless oversight. Repeating a dependency in cmake has no ill effects.
There was a problem hiding this comment.
No, but it is a reason to not merge this PR in. "It works" isn't good enough, it needs to be understandable too.
| add_subdirectory(test) | ||
| endif () | ||
|
|
||
| add_executable(impl_ptr_tests ${TEST_SOURCES}) |
There was a problem hiding this comment.
Erm? Why aren't you creating this target directly in the test dir?
There was a problem hiding this comment.
That's one way. Easy to do if that's where you want it.
| @@ -0,0 +1,22 @@ | |||
| # | |||
| # this file is used if the developer has opted not to use Hunter cmake extensions | |||
There was a problem hiding this comment.
What's wrong with plain CMake? This adds a lot of indirection, and I'm not seeing any advantage right now.
There was a problem hiding this comment.
Try building a project for OSX, linux, windows, ISO and android. You'll soon start to appreciate not having to pre-build all your dependencies for every platform, keep those dependencies up to date, and remember where you built them.
This is what hunter does for you. It's the 80% of cmake that's missing.
There was a problem hiding this comment.
Remove ISO from that list, add QNX and WinCE and you're describing the list of platforms that I've had to target with CMake for a single (large) project. Pre-building is effectively a caching strategy: it can be useful (during development it certainly is), but it's very hard to get right and the benefits are non-existent for small projects.
I do thank you for pointing it out to me, because it may be useful at work.
This however, is a header-only library, so there's no external dependency to be built. I.e. the tests are the only build artifact and those only use Boost's header-only part too.
There was a problem hiding this comment.
WinCE and QNX are covered with hunter by using the toolchain files that have been conveniently supplied by ruslo. the project on github is ruslo/polly.
ISO == iOS, my apologies.
Let's re-discuss when you have come to get to know hunter.
The author is not english so the documentation can take some decoding. Feel free to ping me if you have questions.
| @@ -0,0 +1,12 @@ | |||
| sugar_files(TEST_SOURCES | |||
There was a problem hiding this comment.
This just looks like a duplicate of test/CMakeLists.txt, violating DRY (Don't Repeat Yourself). If you actually need to duplicate things with hunter/sugar to be able to build without it then I think you should just build without it. This just seems to double required maintenance effort.
There was a problem hiding this comment.
In my projects (globally deployed services for very large companies with massive user bases) I always use hunter because I value the reliability, good integration with CI and automatic cross-compilation.
However, I am aware that not everyone will be persuaded of the benefits on first sight (usually the moment someone has actually used Hunter, they never stop using it).
So I thought I'd put equivalent code so that people could use it both ways.
I am a busy person so for me to contribute to a project which has dependencies (even just one), it is helpful to have automatic dependency management.
I don't keep a copy of the boost libraries in my usr/lib because I am often cross-compiling against a number of different systems and versions of c++ and I can't risk cmake etc picking up the wrong version.
Hunter solves that too.
However, this is your project. If the commit is not welcome, no problem. I'll just play on my fork and just contribute code changes.
Another solution might be to have a separate subdirectory for (vanilla) cmake and one for hunterised cmake.
If you ever want this package to be adopted as a hunter package (you do). Then a hunterised version of the cmakelists.txt file will be made in the hunter packaging fork in any case, because the dependency on boost needs to be managed.
Thanks for reading the commit.
R
There was a problem hiding this comment.
From your description I get the impression that Hunter, while it uses CMake, is in practice a completely different build system that isn't interchangeable with plain CMake. I had hoped it would be able to just, at CMake-generation-time, walk all targets, extract its properties, and use those to produce an exportable "package". If it requires interfacing with CMake through it's own abstraction layer that seems rather unfortunate because that's a very invasive approach.
There was a problem hiding this comment.
It's not completely different.
You can use Sugar without deploying it from hunter, it's just that hunter supports it out of the box.
Sugar is about source file management (something that cmake is bad at - since source file properties don't migrate out of sub_directories).
Hunter is about dependency management.
You can simply ignore all hunter code in a cmake project by setting the cache variable HUNTER_ENABLED=NO
As long as your packages can be found, it will all still work.
| ) | ||
|
|
||
| foreach (file ${TEST_FILES}) | ||
| list(APPEND TEST_SOURCES "test/${file}") |
There was a problem hiding this comment.
I think you can just add ${CMAKE_CURRENT_SOURCE_DIR}/ in the set() statement above.
There was a problem hiding this comment.
They would then be absolute paths. Sure that can work.
| @@ -0,0 +1,68 @@ | |||
| cmake_minimum_required(VERSION 3.8) | |||
| cmake_policy(VERSION 3.8) | |||
There was a problem hiding this comment.
This is not necessary, the cmake_minimum_required already defaults to the same policy version.
|
I'll look at Hunter during the next week, because the problem that you mention, easy pulling of external dependencies, is one that I would very much like to see be less of a problem. |
|
cmake is already pretty good at producing both cmake packages and
pkg.config outputs.
…On 10 September 2017 at 16:59, Giel van Schijndel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/sugar.cmake
<#17 (comment)>:
> @@ -0,0 +1,12 @@
+sugar_files(TEST_SOURCES
From your description I get the impression that Hunter, while it uses
CMake, is in practice a completely different build system that isn't
interchangeable with plain CMake. I had hoped it would be able to just, at
CMake-generation-time, walk all targets, extract its properties, and use
those to produce an exportable "package". If it requires interfacing with
CMake through it's own abstraction layer that seems rather unfortunate
because that's a very invasive approach.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE7syfgjwabhHEOrwsEkT4rtmanuL-s6ks5sg_lVgaJpZM4PR_lR>
.
|
|
Guys, have you come to an agreement regarding this pull request? I do not use/know cmake so I'll go with what you think is best. There seems to be a branch conflict also to resolve... |
|
I'm waiting for feedback but am happy to make is more vanilla in the
meantime. I'll create a new pull request when I get a moment - I'm a bit
under the cosh at the moment as I have a production release in 4 days.
…On 15 September 2017 at 23:09, Mike ***@***.***> wrote:
Guys, have you come to an agreement regarding this pull request? I do not
use/know cmake so I'll go with what you think is best. There seems to be a
branch conflict also to resolve...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE7syVdh30BeSW4d-dFnqLvl41ASqr_wks5siueHgaJpZM4PR_lR>
.
|
|
Nice. Much appreciated. Maybe you, guys, might agree on and incorporate as
big as possible a non-controversial bit; then maybe have the debatable bit
as a separate file (like #include). That way you get the functionality you
consider important but then it's not "in their face" for others. Just a
suggestion... And certainly no rush... I was not sure where you, guys, were
on the topic. So, I pinged.
P.S. I mentioned Hunter to our cmake guy... briefly. He said he considered
and evaluated it a couple years ago; it left him cold... Don't shoot the
messenger. :-) I'll keep you posted when I hear more.
…On Sat, Sep 16, 2017 at 7:34 AM, Richard Hodges ***@***.***> wrote:
I'm waiting for feedback but am happy to make is more vanilla in the
meantime. I'll create a new pull request when I get a moment - I'm a bit
under the cosh at the moment as I have a production release in 4 days.
|
|
Hunter is a lot more comprehensive now.
I cross-compile for iOS, Android, windows, linux and OSX. I have to write no special code in my cmake files, and don’t have to build and maintain dependent libraries. It’s all automatic.
Try doing that in cmake without hunter and polly.
But horses for courses :)
R
… On 16 Sep 2017, at 00:01, Mike ***@***.***> wrote:
Nice. Much appreciated. Maybe you, guys, might agree on and incorporate as
big as possible a non-controversial bit; then maybe have the debatable bit
as a separate file (like #include). That way you get the functionality you
consider important but then it's not "in their face" for others. Just a
suggestion... And certainly no rush... I was not sure where you, guys, were
on the topic. So, I pinged.
P.S. I mentioned Hunter to our cmake guy... briefly. He said he considered
and evaluated it a couple years ago; it left him cold... Don't shoot the
messenger. :-) I'll keep you posted when I hear more.
On Sat, Sep 16, 2017 at 7:34 AM, Richard Hodges ***@***.***>
wrote:
> I'm waiting for feedback but am happy to make is more vanilla in the
> meantime. I'll create a new pull request when I get a moment - I'm a bit
> under the cosh at the moment as I have a production release in 4 days.
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#17 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE7sySlip0wYtraYjR50Ik9Mo-6Du8Xiks5sivPXgaJpZM4PR_lR>.
|
|
@madmongo1 I rebased this branch on my plain CMakeLists.txt in this branch: madmongo1/pull-cmake. After looking for a bit I do see some value in the Hunter portion, especially the virtualenv-like behaviour may be relevant here. The Sugar part of it doesn't seem to add any value over plain CMake however. So if you agree, could you rebase your Hunter support (without the Sugar part) on top of latest master then we can get that in? PS excuse the delayed reply, I've been doing (or trying to) way too much things at the same time lately. |
|
Looking a bit more at Sugar I'm not clear what problem it's trying to solve, but it looks like it has some overlap with https://github.com/tomtom-international/cpp-dependencies. The latter has the advantage of not being necessary while building, only while running this tool after having added/removed a source file. It's even possible to automate this such that it's only necessary to execute this in CI (if you give these CI systems commit/push access). |
|
I've just been at Meeting C++ where this, build systems and dependency management, are pretty hot topics. I've discussed this with a few people here (mostly Mathieu Ropert and Diego Rodriguez-Losada) and I believe the main problem with this approach is that it confuses the build system and package management. Dependency fetching is part of package management, not of building. That being said: dependency and package management are necessary, confusing the two just makes solving this more harder though. I've been pointed at Conan, I haven't yet had the time to look at it, but from what I've been told it may solve this problem in a better way because it keeps package management split out from the build system and instead better integrates the two. Requirements can be specified in a similar format to Python's |
|
If it’s properly cross-compiling-aware I think it’s worth a look.
…On Sat, 11 Nov 2017 at 23:22, Giel van Schijndel ***@***.***> wrote:
I've just been at Meeting C++ where this, build systems and dependency
management, are pretty hot topics. I've discussed this with a few people
here (mostly Mathieu Ropert and Diego Rodriguez-Losada) and I believe the
main problem with this approach is that it confuses the build system and
package management. Dependency fetching is part of package management, not
of building. That being said: dependency and package management are
necessary, confusing the two just makes solving this more harder though.
I've been pointed at Conan <https://www.conan.io/>, I haven't yet had the
time to look at it, but from what I've been told it may solve this problem
in a better way because it keeps package management split out from the
build system and instead better integrates the two. Requirements can be
specified in a similar format to Python's requirements.txt in a
conanfile.txt
<http://docs.conan.io/en/latest/reference/conanfile_txt.html>. I'm not
sure how usable this really is at this time, I have to look into that. It
seems promising at least.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE7sycH9wEA2EmgNJNg6LSSdf56pGjxqks5s1h4cgaJpZM4PR_lR>
.
|
Adds support for the CMake build manager.
Also adds [optional] support for the hunter dependency manager which is fast becoming the de-facto standard for seamless cross-platform management of dependant libraries.
after pulling, you can build the tests with
mkdir build
cd build
cmake <path_to_impl_ptr_source>
make