Entt: Please use find_package & ExternalProject for GTest

[DonKult]

Hi,

for some reason make'ing entt _ends_ in an infinite loop while seemingly trying to run cmake for gtest… the looping might be due to my strange environment, but ignoring this its still a bit strange to perform such calls given that alternatives exist. In the APT project (= Debian's package manager) we use this snippet to pick up pre-built binaries (as available on some versions of Fedora, Debian, …) or build gtest from source (some other versions of Debian, …). Proof of concept wise I can give you an adaption of this for entt which worked for me to give you an idea.

Note that I am not a cmake expert (far from it), a real patch would adapt the other instances for this pattern as well & lastly while I am reasonably sure this works on Linux distributions (and BSDs) I have no idea about Mac or Windows, so no PR from me.

Additional pointer: Documentation for cmakes ExternalProject

[skypjack]

What does it mean that it _ends in an infinite loop_? I cannot reproduce it in any way and I don't know how it could happen.
The approach use in EnTT is described here, that is an article linked by the README of googletest (search for section _Incorporating Into An Existing CMake Project_).
It's likely the problem is in the way you're including EnTT and it doesn't make much sense to change the build system because of this, unless we can prove it's broken.

An user that has a cmake expertise higher than mine is @m-waja. Not sure it has the time to follow this thread, but let's see.

[DonKult]

[Just for the record: As said, the loop is due to strange environment on my part which I haven't debugged completely at that point in time – details: I have a ~/bin/make which does various things before calling good old make which seems to interfere here negatively – but that isn't your problem & not the point]

Lets put it differently: I and likely other linux-based developers have gtest already installed (on Debian e.g. is the package called googletest/libgtest-dev – aka you could use that on Travis as well –, similar on Fedora) so it would be nice if entt wouldn't pull needlessly data from offsite sources if the data is already there.

[skypjack]

I'm a Debian Sid user. It has the most updated version of gtest of all the other versions of Debian. It's gtest 1.8.x, that is two years old.
Unfortunately, gtest does no longer create new tags for its releases and they suggest to use the upstream version (that is master at the end of the day). Moreover, version 1.8.x has several problems with some compilers because of deprecated features and some other fancy things you can find all around in the issues of gtest.
So, EnTT doesn't _pull needlessly data from offsite sources_ when _the data is already there_, it downloads a meaningful version from the web because it's likely that the one users have on their machines is too old, buggy and partially relies on stuff that doesn't exist anymore.
Look also at #92 - it was an issue of EnTT due to the fact that the project was using gtest version 1.8.x until a few weeks ago.

but that isn't your problem & not the point

This is probably the problem instead.
If you're experiencing issues because of something wrong made to integrate EnTT, we are willing to help you, but we cannot change the build system because of this.
Moreover, as a side note, if compiling tests ends in an infinite loop, just set BUILD_TESTING to OFF and get rid of the problem. Does it compile if turn off them?

[m-waka]

I think for GTest it currently works best to pull the exact needed version from external. I am using Arch Linux which is despite being rolling release also stuck on version 1.8.0. The Project version on master is on 1.9 but I'm not sure if they update it frequently.

The findModule for GTest does not provide any means for identifying the version of GTest which is a problem as @skypjack noted. Of course it would be better if GTest would provide a proper CMake config file and a config version file but this is unfortunately not the case. So we should stick to the recommendations of the authors.

[DonKult]

for GTest it currently works best to pull the exact needed version from external

Well, currently EnTT pulls master, so not the version which is needed (which might be ~ 1.8 to compile what you have), but at any time 2.0~pre13 which breaks the build due to changed API, but I see why you want to follow upstream recommendations for the time being and hence I am fine with resting my case, I am just not happy about the recommendation. :disappointed:

Note that this topic will come up again at the latest if EnTT becomes a dependency of a (at least mildly) popular opensource game/app as distribution packagers tend to be as allergic to pulling build-artifacts from the internet as I am. So for the benefit of those: distro provided 1.8 works just fine to build EnTT at the time of writing (given that its the point of a distro to make the packages it ships play nicely together). I guess at that time they will come up with a i-know-what-i-am-doing flag to use distro provided stuff.

Oh and of course disabling tests via config flag works, but that is cheating. :wink:

Best regards

David Kalnischkies

[skypjack]

@DonKult Tests don't compile with gtest 1.8.0 on Windows (see #92), that is quite a common platform for game development. So I don't see the point of not using the upstream version of gtest as recommended or excluding tests. I'm sorry.
However, I'm still willing to help you to solve the actual problem you're facing, if you want.

[DonKult]

I think we are talking past each other.

All I am saying is that it is sad that EnTTs buildsystem hardcodes pulling GTest from git – but I can understand that you do it and to some extend also why upstream recommends it as it is the easy way and works for everyone (equally good or bad depending on PoV). My "problem" is that Linux as a platform might be heterogeneous given the large set of different distros, but each distro (release) by itself is a perfectly coherent package of [packaged] tools working together, so pulling GTest from git compared to using what is available on my platform is a net-negative for my platform (security of the downloaded data, being at the whim of upstream API changes, …). In an ideal world the buildsystem would check if GTest is available already and use that if sufficient, but m-waka already mentioned that there are practical problems. In a less ideal world the buildsystem will have an option to disable pulling GTest and using the system resources (at your own risk). I suspect the later to happen at some point as that is the usual requirement of a distro packager, but my cmake-voodoo is too low to fast-track this & I was reading your responses as not willing to support this that early in development which is fine by me for the moment.

The looping I casually mentioned which you seem to have picked up on was a failure entirely on my end, not really related to EnTT (expect that it is the first project I encountered using this style of pulling GTest in, so it seemed logical that this style is the cause and should be replaced by the style I usually encounter which works just fine – for me, see above) and is resolved.

[skypjack]

In a less ideal world the buildsystem will have an option to disable pulling GTest and using the system resources (at your own risk).

A kind of _use system-wide gtest at your own risk_ option set to OFF by default could work. This won't break working code.
Unfortunately we cannot just pick up gtest library from your system (if any) and see if it's fine, because the package doesn't export the version. But, of course, we can stick with the _almost ideal solution_ and let users decide.
What about @m-waka? I think it could work after all.

[m-waka]

@skypjack yes we can do that, seems to be a good compromise. Unfortunately I'm away from my notebook today.

[skypjack]

@m-waka Not a problem, I'll do it today evening or tomorrow. Don't worry. Just wanted to know your opinion for I trust you on this topic. ;-)

[skypjack]

@DonKult Does it work for you?
The default will be to download gtest, but you can use your own local version if any by setting the right option.
This is probably the best compromise so far.

[DonKult]

Sure! Feel free to ask me to test the -DMAKE_DONKULT_HAPPY=ON flag then you have the code. :wink:

[skypjack]

@DonKult Just out of curiosity, can I ask you for what you're using EnTT? If you can tell me, of course.

[m-waka]

@skypjack I can also review your changes if you like.

[skypjack]

@m-waka Sure, I'll ping you as soon as they are online on a dedicate branch. Thank you very much for your help.

[skypjack]

@DonKult @m-waka See branch build_system. I added the option FIND_GTEST_PACKAGE to force using the installed version of gtest. Let me know if it works as expected before to merge it on master. Thank you.

[skypjack]

On master. Let me know if you find any problem. Thank you.

[m-waka]

Seems to be fine. But I realized errors (concerning the scheduler) I'm getting when building the tests. However this seems not to be related to the option:

entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<SucceededProcess>(entt::Scheduler<int>::ProcessHandler*&)' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...);
...
entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<FailedProcess>(entt::Scheduler<int>::ProcessHandler*&)' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...);
...
entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>, int> >(entt::Scheduler<int>::ProcessHandler*&, Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>)' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...);
...
entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>, int> >(entt::Scheduler<int>::ProcessHandler*&, Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>)'

and lot of notes about template argument deduction/substitution failed
I am using gcc 8.1.1 on Arch Linux.
Should I open a new issue, or am I just doing something wrong?

[skypjack]

:thinking: I don't think it's related to the commit about gtest. Does the hash immediately before compile correctly with the same compiler and on the same platform? I cannot reproduce it actually. @m-waka

[skypjack]

@m-waka I tested it on Debian Sid. Everything works fine with g++8 as expected. Unfortunately versions differ, here it is 8.1.0 - damn it!! Btw, I'm not that sure it's something for which to blame EnTT...

[m-waka]

Okay strange. Downgrading to gcc 7.3 fixed it for me... gcc 8.1.0 did not work either. Yeah no blame to EnTT. The hash before has the same error for me, so not related to the changes.

[m-waka]

I get these errors from 229500347de0471dcefcc4bed3028bff64251166 on.

[skypjack]

@m-waka Can you copy and paste the whole error somewhere and post the link? I didn't manage to reproduce it yet.

[m-waka]

sh Scanning dependencies of target scheduler [ 31%] Building CXX object test/CMakeFiles/scheduler.dir/entt/process/scheduler.cpp.o In file included from entt/test/entt/process/scheduler.cpp:3: entt/src/entt/process/scheduler.hpp: In instantiation of 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = SucceededProcess; Args = {}; Delta = int]': entt/test/entt/process/scheduler.cpp:78:37: required from here entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<SucceededProcess>(entt::Scheduler<int>::ProcessHandler*&)' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ entt/src/entt/process/scheduler.hpp:63:24: note: candidate: 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = SucceededProcess; Args = {entt::Scheduler<int>::ProcessHandler*&}; Delta = int]' <near match> decltype(auto) then(Args &&... args) && { ^~~~ entt/src/entt/process/scheduler.hpp:63:24: note: passing 'entt::Scheduler<int>::Then' as 'this' argument discards qualifiers entt/src/entt/process/scheduler.hpp:70:24: note: candidate: 'template<class Func> decltype(auto) entt::Scheduler<Delta>::Then::then(Func&&) && [with Func = Func; Delta = int]' decltype(auto) then(Func &&func) && { ^~~~ entt/src/entt/process/scheduler.hpp:70:24: note: template argument deduction/substitution failed: entt/src/entt/process/scheduler.hpp:65:45: note: cannot convert '((entt::Scheduler<int>::Then*)this)->entt::Scheduler<int>::Then::handler' (type 'entt::Scheduler<int>::ProcessHandler*') to type 'SucceededProcess&&' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ^~~~~~~ entt/src/entt/process/scheduler.hpp: In instantiation of 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = FailedProcess; Args = {}; Delta = int]': entt/test/entt/process/scheduler.cpp:79:34: required from here entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<FailedProcess>(entt::Scheduler<int>::ProcessHandler*&)' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ entt/src/entt/process/scheduler.hpp:63:24: note: candidate: 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = FailedProcess; Args = {entt::Scheduler<int>::ProcessHandler*&}; Delta = int]' <near match> decltype(auto) then(Args &&... args) && { ^~~~ entt/src/entt/process/scheduler.hpp:63:24: note: passing 'entt::Scheduler<int>::Then' as 'this' argument discards qualifiers entt/src/entt/process/scheduler.hpp:70:24: note: candidate: 'template<class Func> decltype(auto) entt::Scheduler<Delta>::Then::then(Func&&) && [with Func = Func; Delta = int]' decltype(auto) then(Func &&func) && { ^~~~ entt/src/entt/process/scheduler.hpp:70:24: note: template argument deduction/substitution failed: entt/src/entt/process/scheduler.hpp:65:45: note: cannot convert '((entt::Scheduler<int>::Then*)this)->entt::Scheduler<int>::Then::handler' (type 'entt::Scheduler<int>::ProcessHandler*') to type 'FailedProcess&&' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ^~~~~~~ entt/src/entt/process/scheduler.hpp: In instantiation of 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>, int>; Args = {Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>}; Delta = int]': entt/src/entt/process/scheduler.hpp:72:81: required from 'decltype(auto) entt::Scheduler<Delta>::Then::then(Func&&) && [with Func = Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>; Delta = int]' entt/test/entt/process/scheduler.cpp:103:6: required from here entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>, int> >(entt::Scheduler<int>::ProcessHandler*&, Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>)' handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ entt/src/entt/process/scheduler.hpp:63:24: note: candidate: 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>, int>; Args = {entt::Scheduler<int>::ProcessHandler*&, Scheduler_Functor_Test::TestBody()::<lambda(auto:6, void*, auto:7, auto:8)>}; Delta = int]' <near match> decltype(auto) then(Args &&... args) && { ^~~~ entt/src/entt/process/scheduler.hpp:63:24: note: passing 'entt::Scheduler<int>::Then' as 'this' argument discards qualifiers entt/src/entt/process/scheduler.hpp:70:24: note: candidate: 'template<class Func> decltype(auto) entt::Scheduler<Delta>::Then::then(Func&&) && [with Func = Func; Delta = int]' decltype(auto) then(Func &&func) && { ^~~~ entt/src/entt/process/scheduler.hpp:70:24: note: template argument deduction/substitution failed: entt/src/entt/process/scheduler.hpp:65:44: note: candidate expects 1 argument, 2 provided handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ entt/src/entt/process/scheduler.hpp: In instantiation of 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>, int>; Args = {Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>}; Delta = int]': entt/src/entt/process/scheduler.hpp:72:81: required from 'decltype(auto) entt::Scheduler<Delta>::Then::then(Func&&) && [with Func = Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>; Delta = int]' entt/test/entt/process/scheduler.cpp:105:6: required from here entt/src/entt/process/scheduler.hpp:65:44: error: no matching function for call to 'entt::Scheduler<int>::Then::then<entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>, int> >(entt::Scheduler<int>::ProcessHandler*&, Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>)' entt/src/entt/process/scheduler.hpp:63:24: note: candidate: 'decltype(auto) entt::Scheduler<Delta>::Then::then(Args&& ...) && [with Proc = entt::ProcessAdaptor<Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>, int>; Args = {entt::Scheduler<int>::ProcessHandler*&, Scheduler_Functor_Test::TestBody()::<lambda(auto:9 ...)>}; Delta = int]' <near match> decltype(auto) then(Args &&... args) && { ^~~~ entt/src/entt/process/scheduler.hpp:63:24: note: passing 'entt::Scheduler<int>::Then' as 'this' argument discards qualifiers entt/src/entt/process/scheduler.hpp:70:24: note: candidate: 'template<class Func> decltype(auto) entt::Scheduler<Delta>::Then::then(Func&&) && [with Func = Func; Delta = int]' decltype(auto) then(Func &&func) && { ^~~~ entt/src/entt/process/scheduler.hpp:70:24: note: template argument deduction/substitution failed: entt/src/entt/process/scheduler.hpp:65:44: note: candidate expects 1 argument, 2 provided handler = Scheduler::then<Proc>(handler, std::forward<Args>(args)...); ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ make[2]: *** [test/CMakeFiles/scheduler.dir/build.make:63: test/CMakeFiles/scheduler.dir/entt/process/scheduler.cpp.o] Error 1 make[1]: *** [CMakeFiles/Makefile2:488: test/CMakeFiles/scheduler.dir/all] Error 2 make: *** [Makefile:141: all] Error 2

[morbo84]

@m-waka I encountered the same error a while ago. It's a GCC bug that affects just the 8.1.0 version and a _subset_ of the 8.1.1.

Starting from the EnTT compilation error I was able to extract a simple example affected by the bug. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86195

The bad news is that e.g. on Arch Linux, the current version of GCC (8.1.1 20180531) is still affected by the bug. The good one is that it seems to be fixed since GCC 8.1.1 20180615, so I think it's a matter of days for this version to become available.

Workarounds? Change the private then function name to another one, or just use clang in the meanwhile.