Export CMake Config file and "modernize" CMake scripts #87

m-waka · 2018-05-31T18:56:06Z

In modern CMake it's prefered to export a CMake configuation file for a package which can then be found by find_package(). This is also the prefered way to import external packages instead of find module scripts.
There is also no install target for entt, yet. Even for header only libraries one can use add_library as an INTERFACE library for generation of a target.

Some references for modern CMake are found in: https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1 and in Daniel Pfeifer’s C++Now 2017 talk Effective CMake (and cmake - Introduction and best practices, 2015).

So this PR tries to "modernize" the CMake scripts and exports a CMake configuration, which can be found by find_package(). It also adds an option for building the documentation, preventing an error when calling make install without having build the docs before.

coveralls · 2018-05-31T19:04:48Z

Coverage remained the same at 100.0% when pulling 3fee2ab on m-waka:master into bdc7bbd on skypjack:master.

skypjack

Thank you very much for your PR.
I'm far from being an expert of cmake, so help on this is really appreciated!!
I put some comments around, can you address them before to merge?

skypjack · 2018-06-01T06:58:04Z

CMakeLists.txt

+    HAS_LIBCPP)
+
+    cmake_pop_check_state()
+    # set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--no-undefined")


Can you please get rid of commented lines? This is git, we won't miss them! :-)

skypjack · 2018-06-01T06:58:26Z

CMakeLists.txt

 #

-include_directories(${entt_SOURCE_DIR}/src)
+#message(STATUS "Compile features: ${CMAKE_CXX_COMPILE_FEATURES}")


As above, I would delete this.

skypjack · 2018-06-01T07:00:36Z

CMakeLists.txt

@@ -4,6 +4,8 @@

 cmake_minimum_required(VERSION 3.2)

+include(GNUInstallDirs)


EnTT fully supports also VS. Isn't this inherently wrong in that case?

Actually this only makes some variables avialable where only CMAKE_INSTALL_INCLUDEDIR is used and which evaluates to include. This should be correct or okay for most platforms. The other variables from GNUInstallDirs are not needed because the library is header only. We could also omit the include and replace the variable with a simple include if prefered.

Don't worry, often I make comments just to better understand what's going on. If you confirm that it works also on VS, it's fine for me.

I confirm it works under VS. Just tested it for playing safe.

Ok, I just noticed that I also used the ${CMAKE_INSTALL_LIBDIR} variable from GNUInstallDirs for the exported and installed config file. This also works like a charm on Windows (VS) though I install it to one of the most common path from the unix conventions: <prefix>/(lib/<arch>|lib|share)/cmake/<name>*/. But there seems to be different conventions where to install the config file on different platforms: https://cmake.org/cmake/help/v3.2/command/find_package.html

CMake constructs a set of possible installation prefixes for the package. Under each prefix several directories are searched for a configuration file. The tables below show the directories searched. Each entry is meant for installation trees following Windows (W), UNIX (U), or Apple (A) conventions.

This is merely a convention, so all (W) and (U) directories are still searched on all platforms.

Is there a reason you didn't add -pedantic -Wall for MSVC?

If a reasons existed, I forgot it actually.

I think the optimization and the libc++ options are also belonging to usage requirements. Do you want to keep them on board? - is libc++ improving performance?

If I remember it right, it was due to the fact that on mac it didn't compile without including libc++. Because of this I put a line to prefer libc++ in general. We can rearrange it differently, not a problem. PR are compiled on the fly both on Travis and appveyor, so we know immediately if changes ruin something.

On my system [...]

Well, I don't think we can assume this on all systems and putting twice those options isn't a problem, so...

In general, it looks like you are very skilled with cmake, so I tend to consider your suggestions.
If you find we can further improve the build system of EnTT, feel free to continue pushing on the PR. We aren't in a hurry to merge it. ;-)

I think for MSVC, we miss much things, especially as CMake Interface EnTT should use those flags on MSVC ->
/Zi - Produces a program database (PDB) that contains type information and symbolic debugging information for use with the debugger.
/FS - Allows multiple cl.exe processes to write to the same .pdb file
/DEBUG - Enable debug during linking
/Od - Disables optimization
/Ox - Full optimization
/permissive- (c++ conformance)

and on linux i'm in favor to use:

-Weff-c++

@skypjack Alright I think it's best to add the flags to the test targets then.
@Milerius I think these options/flags are no real requirements for using the library and so they don't have to be set as usage requirements on the EnTT target. But we can add some flags individually for the tests if needed. Projects depending on EnTT can define their own flags.

@m-waka Fine for me. Do you plan to do it on your branch and therefore with this PR?

@skypjack yes I will implement it on my branch with this PR tomorrow.

skypjack · 2018-06-01T07:02:43Z

CMakeLists.txt

+add_library(EnTT INTERFACE)
+
+target_include_directories(EnTT INTERFACE
+  $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>


I know it's probably annoying, but currently EnTT uses 4 spaces for indentation. I'd prefer to keep it consistent throughout the whole project. Thank you.

…h Config Version file, Export CMake package

…r using libc++, Remove from EnTT target and move it to test targets

m-waka · 2018-06-05T16:52:15Z

Alright, perhaps the added -pedantic and -Wall is the reason I will test and try to fix it on my Windows machine. So further commits will come.

MSBUILD : error MSB4017: The build stopped unexpectedly because of an unexpected logger failure.
Microsoft.Build.Exceptions.InternalLoggerException: The build stopped unexpectedly because of an unexpected logger failure. ---> System.Net.WebException: The remote server returned an error: (500) Internal Server Error.

…dows

The problem with -Wall is not due to the platform but due to the compiler MSVC

m-waka · 2018-06-06T14:03:55Z

So this builds now with MSVC2017 (at least on my system, let's wait for appveyor). Adding -Wall on MSVC was creating lots of warnings... I'm not sure whether these are problematic in any way.

skypjack · 2018-06-06T14:26:42Z

I think appveyor isn't starting because of a conflict within googletest.in.
I updated GIT_TAG to master because of #92, that's all.

m-waka · 2018-06-06T14:41:17Z

So we have now two new CMake options which are by default ON:

option(ENTT_COMPILE_OPTIONS "Use compile options from EnTT." ON)
option(USE_LIBCPP "Use libc++ by adding -stdlib=libc++ flag if availbale." ON)

skypjack · 2018-06-06T14:47:31Z

Really THANK YOU for your efforts!!
Let's see if travis and appveyor like this PR.

skypjack · 2018-06-07T13:45:33Z

You did it!! Thank you!!
I'll review everything during the weekend and merge the PR.
Hope it's fine for you. Good job, welcome on board!! ;-)

…_VARS CMAKE_INSTALL_INCLUDE_DIR of configure_package_config_file, Remove redundant options, correct target_include_directory for INSTALL_INTERFACE, set the Version in EnTTConfig file and check CMake version

m-waka · 2018-06-08T13:16:07Z

Some tiny improvements/fixes, sorry...

skypjack · 2018-06-08T13:47:53Z

Not a problem. Do you want to take your time to review it before to merge on master?
If you need more time, we can leave open the PR for some days, as you prefer.

m-waka · 2018-06-08T14:56:46Z

Okay, I will review it a bit and tell you if I think everything works and do some small commits if necessary.

m-waka · 2018-06-11T06:47:29Z

I think this is fine now. We have now an additional build tree CMake config file generated besides the config file for the install tree.

skypjack · 2018-06-11T07:18:06Z

Is it ignored by git or at least created within the build directory?

m-waka · 2018-06-11T07:54:06Z

It is here: cmake/in/EnTTBuildConfig.cmake.in and put into the buildtree by configure_file().

skypjack · 2018-06-13T08:22:15Z

If it's fine for you, I'm merging it on a dedicated branch (experimental?), so as to be able to review it as a whole in a single commit before to port everything on master.
I can change directly the PR in this sense, nothing to do your side.

m-waka · 2018-06-13T08:34:27Z

As you want. 👍 If there are any questions concerning the PR, feel free to ask.

skypjack · 2018-06-13T12:08:49Z

So, what do we expect an user to do now to be able to easily use EnTT?
I mean, if I were a new user that wants to use the library in a cmake based project, what should I do?

skypjack · 2018-06-13T13:31:47Z

Another question. Why this:

target_compile_features(
    EnTT
    INTERFACE cxx_std_14
)

Instead of this?

set(CMAKE_CXX_STANDARD 14)

m-waka · 2018-06-13T14:02:31Z

If it is already installed on your system you can easily call:

find_package(EnTT 2.6 REQUIRED)
add_executable(myexec main.cpp)
target_link_libraries(myexec
    PRIVATE EnTT)

If it is not installed you can either clone and install it or add it as ExternalProject.
If you install it to a none default location (by setting the CMAKE_INSTALL_PREFIX variable to it) you have to add this location to the CMAKE_PREFIX_PATH variable to be able to find EnTT.

If you as developer of EnTT want to, you can also provide releases which include only the installed files (header files and install tree config files) or we can generate an installer with CPack.

You can still add it as subdirectory and as we also have a build tree config file it can then also be used without installing.

target_link_libraries is the way to express dependency relations. This way all the usage requirements (INTERFACE properties) of EnTT get imported by the target.
As I also provided the variable ENTT_INCLUDE_DIRS within both build tree and install tree config file you can still just call:
target_include_directories(myexec PRIVATE ${ENTT_INCLUDE_DIRS})
But this way you won't get the other usage requirements of EnTT imported.

To your second question:
Using target_compile_features for setting the C++ standard is the prefered way. This way you are expressing on a target basis the usage requirements of EnTT. These are then imported by dependent targets as explained above. You can even define the requirement on a feature basis e.g. cxx_lambdas and others.
On the other hand if you are setting the variable CMAKE_CXX_STANDARD and the project which is using EnTT alread sets this variable via a Toolchain file to e.g. C++17. This could probably have side effects. What you really wanted to express is that if you are using EnTT the importing target needs minimum flags for the C++14 standard.

I hope this was comprehensible.

skypjack · 2018-06-13T14:16:01Z

Do you want to become the official maintainer of the EnTT build system, man? 🥇
It would be great to know that I can ask someone that knows what he's doing!! 👍

Milerius · 2018-06-13T14:25:18Z

It's shouldn't be ignored since CMake need it for the installation

skypjack · 2018-06-13T14:27:09Z

Yep, I realized it and deleted the message a couple of minutes ago... :-)

m-waka · 2018-06-13T14:27:37Z

Yeah, why not. Would be a great honor. I hope I will also improve my (modern) C++ knowledge while using EnTT.
@Milerius: in fact it's not installed it is configured for the build tree and only put there.

skypjack · 2018-06-13T14:43:36Z

In order to avoid polluting the build directory of external projects, shouldn't we replace CMAKE_CURRENT_BINARY_DIR and CMAKE_CURRENT_SOURCE_DIR with entt_BINARY_DIR and entt_SOURCE_DIR?
In particular within CMakeLists.txt and docs/CMakeLists.txt.

m-waka · 2018-06-13T14:50:18Z

As CMAKE_CURRENT_BINARY_DIR is the build directory that is currently being processed by cmake and CMAKE_CURRENT_SOURCE_DIR is the source directory that is currently being processed by cmake, this shouldn't pollute anything.
https://cmake.org/cmake/help/latest/variable/CMAKE_CURRENT_BINARY_DIR.html
https://cmake.org/cmake/help/latest/variable/CMAKE_CURRENT_SOURCE_DIR.html

mrowold added 2 commits May 29, 2018 15:25

CMake modifications and Config file generation

35ec58a

CMake minor style changes

4c8e0a5

skypjack requested changes Jun 1, 2018

View reviewed changes

mrowold and others added 6 commits June 1, 2018 10:17

Get rid of commented lines, fix indentation

efd1c3b

Respect Windows conventions for CMake config directory, Fix error wit…

c7a9af7

…h Config Version file, Export CMake package

Add CMake option for use of ENTT_COMPILE_OPTIONS, Add CMake option fo…

29bd921

…r using libc++, Remove from EnTT target and move it to test targets

Fix indentation

8fd3733

Fix indentation (again)

086e3af

Fix indentation (again)

a681c38

mrowold added 2 commits June 6, 2018 15:46

Fix Windows problems with compile option -Wall in not using it on Win…

b5b92cf

…dows

Improved generator expression

5d9ce66

The problem with -Wall is not due to the platform but due to the compiler MSVC

Merge branch 'master' into master

942ffda

mrowold added 2 commits June 8, 2018 15:09

Set compatibility for ConfigVersion file to AnyNewerVersion, Add PATH…

bd6b57d

…_VARS CMAKE_INSTALL_INCLUDE_DIR of configure_package_config_file, Remove redundant options, correct target_include_directory for INSTALL_INTERFACE, set the Version in EnTTConfig file and check CMake version

Merge branch 'master' of https://github.com/m-waka/entt

a8a6715

Add missing closing brace, Add a special config file for the build tree

3fee2ab

skypjack approved these changes Jun 13, 2018

View reviewed changes

skypjack added the enhancement accepted requests, sooner or later I'll do it label Jun 13, 2018

skypjack self-assigned this Jun 13, 2018

skypjack changed the base branch from master to experimental June 13, 2018 12:00

skypjack merged commit 6c55aaf into skypjack:experimental Jun 13, 2018

skypjack mentioned this pull request Jun 13, 2018

Help wanted: updated build system #99

Closed

		@@ -4,6 +4,8 @@

		cmake_minimum_required(VERSION 3.2)

		include(GNUInstallDirs)

Export CMake Config file and "modernize" CMake scripts #87

Export CMake Config file and "modernize" CMake scripts #87

Conversation

m-waka commented May 31, 2018

coveralls commented May 31, 2018 • edited Loading

skypjack 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

m-waka Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-waka Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Milerius Jun 4, 2018 • edited 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

m-waka commented Jun 5, 2018 • edited Loading

m-waka commented Jun 6, 2018

skypjack commented Jun 6, 2018 • edited Loading

m-waka commented Jun 6, 2018

skypjack commented Jun 6, 2018

skypjack commented Jun 7, 2018

m-waka commented Jun 8, 2018

skypjack commented Jun 8, 2018

m-waka commented Jun 8, 2018

m-waka commented Jun 11, 2018

skypjack commented Jun 11, 2018

m-waka commented Jun 11, 2018 • edited Loading

skypjack commented Jun 13, 2018

m-waka commented Jun 13, 2018

skypjack commented Jun 13, 2018

skypjack commented Jun 13, 2018

m-waka commented Jun 13, 2018 • edited Loading

skypjack commented Jun 13, 2018

Milerius commented Jun 13, 2018 via email • edited Loading

skypjack commented Jun 13, 2018

m-waka commented Jun 13, 2018 • edited Loading

skypjack commented Jun 13, 2018

m-waka commented Jun 13, 2018 • edited Loading

coveralls commented May 31, 2018 •

edited

Loading

m-waka Jun 1, 2018 •

edited

Loading

m-waka Jun 1, 2018 •

edited

Loading

Milerius Jun 4, 2018 •

edited

Loading

m-waka commented Jun 5, 2018 •

edited

Loading

skypjack commented Jun 6, 2018 •

edited

Loading

m-waka commented Jun 11, 2018 •

edited

Loading

m-waka commented Jun 13, 2018 •

edited

Loading

Milerius commented Jun 13, 2018 via email •

edited

Loading

m-waka commented Jun 13, 2018 •

edited

Loading

m-waka commented Jun 13, 2018 •

edited

Loading