Modified build system for itemmodels

[Alexander Neundorf]

On Monday 30 April 2012, Stephen Kelly wrote:
> Alexander Neundorf wrote:
> > Hi,
> > 
> > attached is a patch for kdelibs/tier1/itemmodels/.
> > 
> > It removes the usage of ECMQtFramework.cmake and uses instead the (new)
> > cmake features directly.
> > 
> > The biggest difference is that itemmodels_version.h.in,
> > itemmodelsConfig.cmake.in and itemmodelsUseFile.cmake.in are now part of
> > itemmodels and also configured() directly in its CMakeLists.txt.
> 
> itemmodels_version.h.in should be able to be a template in ecm (it works
> for ConfigVersion.cmake files). The problem with copies is that they get
> changed in a different way in each project, so you'll end up with 20
> slightly different instances of this. There is also no central place to
> add features. If I want to add K_ITEMMODELS_DEPRECATED_SINCE(x, y),
> similar to the Qt macro, I want to add it once, not 20 times.
> 
> For the config file, I would also prefer not to copy everything around. I
> think it would be better to have a macro/function in ecm generate the
> common stuff in a Config-common.cmake, and then each framework maintainer
> will write a config file like this:
> 
> include(itemmodelsConfig-common.cmake)
> 
> macro(my_framework_macro)
>   # ...
> endmacro()
> 
> Again, this would allow the frameworks to get the new features when we add
> them to the ${project}Config-common.cmake template.
> 
> > Despite of the code duplication (in trivial cases) this causes, why do I
> > think this is better ?
> > 
> > Without the patch, all you see in the CMakeLists.txt is
> > include(ECMQtFramework)
> > without any following function/macro calls.
> 
> Your patch is indeed an improvement in that area. However, we can also
> equally have
> 
> ecm_write_version_header(...) to create the itemmodels.h, and
> ecm_write_config_common(...) to create the cmake file for config stuff.

Yes, that's an option, see below for more.

> > So, this basically doesn't tell the developer anything about what it
> > does. As result, he ends up with a bunch of created files in his build
> > dir, which also get installed.
> 
> Yes, having macro/function invokations is more clear than just the include.
> 
> This makes it obvious enough to developers where the stuff comes from.

Ok.
 
> > This IMO easily qualifies as "magic" most developers will not understand,
> > they will not know where these files come from, what they are good for,
> > what they should contain.
> > 
> > By having the
> > write_basic_package_version_file()
> > configure_package_config_file()
> > and
> > configure_file()
> > 
> > calls, followed by their respective install() calls directly in the
> > CMakeLists.txt, it becomes immediately obvious where these generated
> > files come from.
> > By having the .in files in the same directly, it makes it also much
> > easier for the developer to have a look at them, and to understand them,
> > especially since they are simpler now too (since they are less general).
> 
> I don't think they'll have a look anyway tbh. Most people in KDE avoid the
> buildsystem.

I know.
For me this is a reason to make it as visible as possible, so it becomes as 
easy to understand as possible.

> > Especially the FooConfig.cmake.in file should IMO not be provided
> > somewhere, or generated automatically, since it can and should carry
> > information specific to the respective project.
> 
> Right I agree. The common stuff can be generated though.
> 
> > That's way it is IMO not a
> > good idea to provide a generic one in e-c-m. Developers will start to
> > simply use it, which will keep them from learning what they can do with
> > such files, and which information can and should be put in them.
> 
> I think that's also the case if we generate 'common' stuff.
> 
> > The same probably applies for the Use-file.
> 
> Why do we need a use file? Qt 5 doesn't create or install them.

You added it, so I thought you want to have it.
If not, let's remove it.


> > We can discuss about providing some wrapper which in some way bundles the
> > calls to write_basic_package_version_file(),
> > configure_package_config_file() and configure_file(), but it should be in
> > a way that it stays obvious where the generated files come from, and easy
> > for the developer to modify them depending on the project-specific needs.
> 
> Maybe. I'm not sure it's needed.

With "wrapper" I mean some way to provide the generic stuff, like the  
ecm_write_version_header() and ecm_write_config_common() you mention above.

I like this much more than include(ECMQtFramework).

Still, I wouldn't do it.
The code for the version header is trivial, both on the cmake side as on the 
header side. I think any developer using cmake is able to do that.
This also applies to the ecm_version/ecm_set_version_variables() macro. They 
are even more trivial, and maybe more confusing than helping.
Both (setting version variables and configuring a version header) are basic 
cmake usage. As a cmake developer I would object to adding this to cmake, 
since it is trivial.
For e-c-m my opinion is less strong, but I still don't consider it necessary.
By adding such things to e-c-m, again we start to drift away from pure cmake 
usage, so KDE cmake files will again look different than non-KDE projects. 
This is ok if it is for a good reason.
Here the reason is only to save some code, not making something which is hard 
easier.
You know, if I see

configure_file(version.h.in ${CMAKE_BINARY_DIR}/version.h)

I know immediately what is going on, and I can easily have a look at 
version.h.in which must be located in the source dir.
If I see

ecm_create_version_header(${CMAKE_BINARY_DIR}/version.h
                          NAME itemmodels VERSION 1.2.3 )

I could guess that it must be equivalent to the call above, but I would have 
to actually look at the documentation and at the generated file (because the 
version.h.in is somewhere in the system inside the e-c-m installation) to know 
exactly what it is doing. Once I found out I would start to wonder "why don't 
they simply use configure_file() ?".


So for these two things I weakly object, for two reasons:
* it is not really necessary because it is trivial
* it adds API to learn additionally to what is in cmake

For the Config.cmake file it is different.
This is not trivial, but every library maintainer needs to understand it.

So I want to make this as easy as possible.
With "easy" I don't mean little code to write, but code which is easy to 
understand.
Code is read more often than it is written, I want the Config.cmake.in files 
to be as easy as possible to read, even if it requires more code to write.

Creating a Config.cmake file by combining a self-written and an auto-generated 
part adds complexity.
To understand it, you have to understand the generated code, and why it is 
generated just as it is. It will be seen as kind of The Right Thing, and 
adding to it will be seen as something which should be avoided if possible.

Also, by putting code for a common part of Config.cmake files into e-c-m we 
would add a common dependency (just hidden because it is on e-c-m), which we 
say we don't want to have.


Having more buildsystem code is the price we have to pay for separating the 
libraries from one big kdelibs to multiple independent libraries.

For the Config.cmake file I object more strong, because it makes the process 
of creating a Config.cmake file look even more complicated than it is.


Maybe we can find other means to achieve consistency ?
Like some tests or scripts which are part of the test suite ?
Or some macro which checks something at the end of a CMakeLists.txt ?
Some generic way to check whether an installed Config.cmake file follows our 
conventions/expectations ?


So, what do we expect from a Config.cmake file for a typical library ?

It should set
* Foo_INSTALL_PREFIX or Foo_INSTALL_DIR
* Foo_INCLUDE_DIRS
* Foo_LIBRARIES

Usually it will do
include(${CMAKE_CURRENT_LIST_DIR}/FooLibraryTargets.cmake)
set(Foo_LIBRARIES Foo)
or 
set(Foo_LIBRARIES SomeNamespace::Foo)

I prefer to use a namespace, so the identifiers look different than just a 
library name -lfoo , ideally the namespace should give a hint that this thing 
is an imported library, so maybe ImportedCMakeLib::Foo or something.

Alex