Spec for sub target modularization #510

eile · 2016-09-22T15:36:09Z

pretty-formatted: https://github.com/eile/CMake/blob/master/doc/modules.md

hernando · 2016-09-22T16:37:28Z

doc/modules.md

+    Lunchbox::Test
+    Lunchbox::Thread
+    Lunchbox::URI
+    Lunchbox::UnorderedIntervalSet


After checking the code with Daniel I think this needs be renamed to OrderedIntervalSet; there was an error when the class was moved from the original project into Lunchbox.

Or just IntervalSet. std.set is also ordered, unordered_set not.

rdumusc

In general, I think this can be a very good improvement to our cmake build system

rdumusc · 2016-09-22T18:41:26Z

doc/modules.md

+  Lunchbox, see PersistentMap). If they are necessary, e.g, in hwsd,
+  downstream code needs to test for the target's availability (as done
+  e.g. in eq/server).
+* Servus will eventually be reintegrated as a lunchbox sub library.


Freedom for Servus!! 😮

Freedom is Slavery!! We've always been happy with Lunchbox::Servus!!

(not sure how to read your comment, so going full Orwell here)

rdumusc · 2016-09-22T18:51:15Z

doc/modules.md

+_Resolution: Avoid them as possible, downstream projects need to check
+with if(TARGET) in cmake:_
+
+We could (re)use the COMPONENTS parameter in common_find_package(), but


I would rather suggest to do like Qt and create one find script per target, which avoids the problem and allows much finer grained use in downstream projects. Real world example:

common_find_package(Qt5Core REQUIRED)
common_find_package(Qt5WebEngine 5.6)
if(NOT TARGET Qt5::WebEngine)
common_find_package(Qt5WebKitWidgets SYSTEM)
endif()

See above and updated spec.

rdumusc · 2016-09-22T18:54:25Z

doc/modules.md

+  NAME_OMIT_LIBRARY_HEADER.
+* The old common_library is deprecated, and stays during the transition
+  period.
+* Optional sub-targets are to be avoided and hidden by facades (esp. in


Doesn't sound very practical, see my comment below about having one find script per library instead.

We discussed this, see added comment in Issue 1. We already did it like this for other reasons, but let's discuss on Monday.

rdumusc · 2016-09-22T18:55:35Z

doc/modules.md

+    #include <zeroeq.h>
+    #include <lunchbox/log.h>
+
+### Desired Lunchbox Granularity


Looks great!

rdumusc · 2016-09-22T18:58:59Z

doc/modules.md

+
+    common_add_library(Lunchbox DEPENDS PUBLIC Lunchbox::Log)
+
+Generates: .../include/lunchbox.h


I wonder if this one is actually useful

We use it quite a lot, if you just don't want to care about sub components.

I would argue that generally you should care about including only the components you use. Could be OK for a small project, but I wouldn't include boost/boost.h for instance. But that's just a detail.

eile · 2016-09-23T09:41:09Z

@favreau FYI

hernando · 2016-09-23T09:46:45Z

doc/modules.md

+## Implementation
+
+* All sub project targets are excluded from the default build, and
+  required targets are built only based on the dependencies.


What if you are building Lunchbox as a standalone project to install the libraries in your system and use them from there afterwards?

Then Lunchbox is the superproject and all targets are in the default build.

hernando · 2016-09-23T09:54:27Z

doc/modules.md

+## Naming
+
+    common_add_library(Lunchbox::Log HEADERS logImpl.h PUBLIC_HEADERS log.h
+      SOURCES log.cpp DEPENDS PUBLIC Lunchbox::Bar PRIVATE Lunchbox::Foo)


If there any magic associated with the :: or is it just a convention because CMake doesn't tokenize target identifiers at :?

Iirc it's the CMake convention/namespacing for sub-targets. We found this somewhere in the CMake documentation dungeons. @tribal-tec: do you remember there?

Since CMake 3, it denotes imported or alias targets: https://cmake.org/cmake/help/v3.0/policy/CMP0028.html. We will/have to use for export() of each subtarget.

There is no magic to ::, but yes I think it's a convention. find_package(Qt5Core) -> Qt5::Core.

thanks @tribal-tec for the clarification, althogh I am not sure I understand the docuementation correcly. So inside the Lunchbox project, are the targets named differently (wihtout the ::) because it is not an imported target? So there is LunchboxLog depends on LunchboxAny instead of Lunchbox::Any, for instance?

hernando · 2016-09-23T10:00:29Z

doc/modules.md

+means that each sub library becomes a separate git project: find_package
+is designed to find a project, and we eventually want to extend the
+common_find_package syntax to include a git url and SHA to get rid of
+the redundant .gitsubprojects.


I guess that what a package is is ill-defined in general, so we should agree on what does it really mean. Qt is not the only example where there's no one-to-one relationship between repository - git module - CMake package, VTK is similar in this regard.

Whatever is implemented, I'd prefer to have a single place where the required Lunchbox subtargets are requested. It can be done just after common_find_package_post, but is looks more correct to do what Raphael suggests or use COMPONENTS (which by the way I don't understand why you say it overloads semantics, Boost works exactly that way).

I think it's mostly a non-issue today. The only optional targets I am aware of is hwsd and ZeroEQ::Qt support today, and they are already handled downstream correctly with nobody complaining.

Then, I understood it wrong. When Lunchbox is detected with common_find_package it won't be necessary to test for Lunchbox::Foo because everything is going to be there, right? Just that it won't be built by default if not needed.

I haven't given much thought about how the merging of common_find_package and .gitsubproject will look like in practice. Maybe it should be a new command, that calls common_find_package internally to avoid the repetition but does not cause confusion with finding system packages. In any case, I don't think it's a blocker, and that it implies creating separate git projects. I does point to something interesting though, that maybe each library could be versionned separately.
I agree with Juan that what a package is is ill-defined in general. For instance, boost or qt come as several pacakges on linux, but as one install on macports. Also Poppler, which can be considered as one small project, but is split into Poppler, poppler-qt, poppler-glib, etc...
COMPONENTS finding look like the right thing to do at first but they are a pain to handle in practice. I know it from writing a FindPoppler.cmake with separate components and from the problems that we have (had) with finding all those boost components. The way Qt works now is much better IMO.
On the contrary, there are plenty of such optional targets: ServusQt, DeflectServer (today part of Deflect, but should be separated), probably 90% of the upcoming Lunchbox targets, etc.

There is no test for Lunchbox::Foo. Config mode package configs simply declare an imported target Lunchbox::Foo.

hernando · 2016-09-23T10:02:17Z

doc/modules.md

+    Lunchbox::Test
+    Lunchbox::Thread
+    Lunchbox::URI
+    Lunchbox::OrderedIntervalSet (was UnorderedIntervalSet)


Do you have any idea of whether this level of fragmentation can affect the loading time of applications in a noticeable way?

On slow filesystems potentially. If this really is an issue, static linking is your friend 👿

Yes, there is a static per-shared-library ld.so cost. Shall we use static linking? ;)

rdumusc · 2016-09-28T07:57:55Z

CommonCompiler.cmake

-# features for the given target.
+# This defines the common_compile_options() function to apply compiler
+# flags and features for the given target, and adds the target to the
+# project-all meta target.


I don't think adding the target to the project-all target has anything to do with the compile options (setting compilation flags).
I would move this to the three places where this function is needed: CommonApplication, CommonLibrary, and CommonCPPCtest, probably where ${PROJECT_NAME}_ALL_DEP_TARGETS is already filled.

In fact, couldn't the ${PROJECT_NAME}_ALL_DEP_TARGETS be removed entirely? DoxygenRule could also depend on ${PROJECT_NAME}-all, right?

Ok, I placed it in compile_flags to not C&P it in three places, but since you are the second one to complain I moved it up now. ;)

Good point, ALL_DEP_TARGETS removed and DoxygenRule cleaned up.

great! then this file should be reverted (no changes, comment no longer applies)

rdumusc · 2016-09-29T09:21:58Z

Common.cmake

@@ -115,6 +117,12 @@ if(NOT PROJECT_namespace)
  set(PROJECT_namespace ${PROJECT_INCLUDE_NAME})
 endif()

+if(NOT TARGET ${PROJECT_NAME}-all)
+  # Create <project>-all target. Deps are added by common_compile_options()


not anymore

rdumusc · 2016-09-29T09:23:14Z

CommonCompiler.cmake

-# features for the given target.
+# This defines the common_compile_options() function to apply compiler
+# flags and features for the given target, and adds the target to the
+# project-all meta target.


great! then this file should be reverted (no changes, comment no longer applies)

rdumusc · 2016-09-29T09:33:00Z

DoxygenRule.cmake

-add_dependencies(doxygen_html doxygen_html_${PROJECT_NAME})
-
-add_custom_target(${PROJECT_NAME}-doxygen DEPENDS doxygen_html_${PROJECT_NAME})
+add_custom_target(${PROJECT_NAME}-doxygen DEPENDS ${PROJECT_NAME}-html)


This looks much better! Now do we need to keep two separate targets which do the same thing? I would keep only ${PROJECT_NAME}-doxygen and remove ${PROJECT_NAME}-html.
Also, the new PROJECT-install target is missing from the documentation above.

The -html and -install targets are internal and undocumented on purpose.

Removed -html target.

Tweak clang check options for 16.04

5b094c6

eile force-pushed the master branch 3 times, most recently from a820b93 to fe77de9 Compare September 22, 2016 15:38

eile assigned tribal-tec, hernando and rdumusc and unassigned tribal-tec, hernando and rdumusc Sep 22, 2016

hernando reviewed Sep 22, 2016

View reviewed changes

rdumusc reviewed Sep 22, 2016

View reviewed changes

eile force-pushed the master branch 2 times, most recently from 2abce0c to 1f70a04 Compare September 23, 2016 09:35

hernando reviewed Sep 23, 2016

View reviewed changes

eile force-pushed the master branch 3 times, most recently from 9c8327b to 31f2c7e Compare September 26, 2016 12:56

Spec for sub target modularization

2b876c4

eile force-pushed the master branch from 31f2c7e to 2b876c4 Compare September 26, 2016 13:30

Also create project-all target for super project

be9c024

rdumusc reviewed Sep 28, 2016

View reviewed changes

eile force-pushed the master branch from 4ecea6e to 74a5210 Compare September 29, 2016 08:33

eile pushed a commit to eile/CMake that referenced this pull request Sep 29, 2016

CR Eyescale#510

74a5210

rdumusc requested changes Sep 29, 2016

View reviewed changes

eile force-pushed the master branch from 74a5210 to 5bc98fe Compare September 29, 2016 11:39

eile pushed a commit to eile/CMake that referenced this pull request Sep 29, 2016

CR Eyescale#510

5bc98fe

CR Eyescale#510

fdee136

eile force-pushed the master branch from 5bc98fe to fdee136 Compare September 29, 2016 12:07

rdumusc approved these changes Sep 29, 2016

View reviewed changes

Fix PROJECT_NAME-update-gitexternal naming

294a164

tribal-tec merged commit 483f0eb into Eyescale:master Sep 29, 2016

tribal-tec pushed a commit that referenced this pull request Sep 29, 2016

CR #510

ebd64ca


		common_add_library(Lunchbox DEPENDS PUBLIC Lunchbox::Log)

		Generates: .../include/lunchbox.h

Spec for sub target modularization #510

Spec for sub target modularization #510

Conversation

eile commented Sep 22, 2016 • 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

rdumusc left a comment

Choose a reason for hiding this comment

rdumusc Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdumusc Sep 22, 2016 • 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

rdumusc Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eile commented Sep 23, 2016

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

rdumusc Sep 23, 2016 • 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

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

rdumusc Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdumusc Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eile commented Sep 22, 2016 •

edited

Loading

rdumusc Sep 22, 2016 •

edited

Loading

rdumusc Sep 22, 2016 •

edited

Loading

rdumusc Sep 22, 2016 •

edited

Loading

rdumusc Sep 23, 2016 •

edited

Loading

rdumusc Sep 29, 2016 •

edited

Loading

rdumusc Sep 29, 2016 •

edited

Loading