Modified build system for itemmodels

[Alexander Neundorf]

On Tuesday 01 May 2012, Stephen Kelly wrote:
> Alexander Neundorf wrote:
> >> > 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.
> 
> Given that we can't expect too much cmake knowledge from the majority of
> KDE developers, do we need to make it *that* visible?
> 
> The people interested in the buildsystem for any reason would find out
> what's going on anyway.
> 
> >> 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.
> 
> Yes.

Ok. I'll remove them in the next days.
Are they used anywhere already ?

...
> > Creating a Config.cmake file by combining a self-written and an
> > auto-generated part adds complexity.
> 
> I don't know. I think people understand 'inheritance' and 'inclusion'. By
> putting the part that must be correct and can be generated into the
> generated file, we ensure that it's correct and present in the end result.
> 
> If we're expecting people to copy+paste we'll end up with a big mess. No
> matter how easy you or I think the stuff is, people will not get it.
> 
> When I have copy+pasted stuff in the past, I'd always look at the result
> and say 'I don't know what this part is, but I don't think I need it', and
> I took it out.
> 
> Divergence, and hard to maintain.
> 
> > 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.
> 
> It's most likely that someone with a clue would add to the template if
> necessary, yes. Someone without a clue will add to the place where they
> feel safe adding to, but if features make sense, they can be generalized
> and 'upstreamed', and all frameworks will get them.
> 
> > 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.
> 
> All frameworks already depend on ecm. Maybe I don't understand.


One of the major goals for KDE frameworks from the buildsystem side is to use 
as little as possible KDE-specific extensions.
This is what I heared from many people, e.g. in Randa.

We, especially you, have spent a lot of effort on getting stuff into cmake to 
reach this goal.

Now we are starting to put stuff into e-c-m only because it makes things more 
convenient for KDE.

Whether I consider something KDE-specific or not I ask myself "does anybody 
outside KDE need this ?".

Now look at ecm_version():

macro(ecm_version _major _minor _patch)
  set(ECM_VERSION_MAJOR ${_major})
  set(ECM_VERSION_MINOR ${_minor})
  set(ECM_VERSION_PATCH ${_patch})
  set(ECM_SOVERSION ${_major})
  set(ECM_VERSION_STRING "${ECM_VERSION_MAJOR}.${ECM_VERSION_MINOR}.
${ECM_VERSION_PATCH}")
endmacro()


IMO, nobody outside KDE needs this. Everybody else will wonder why he should 
use this macro instead of simply doing 

set(FOO_VERSION_MAJOR 1)
set(FOO_VERSION_MINOR 2)
set(FOO_VERSION_PATCH 3)
set(FOO_SOVERSION ${FOO_VERSION_MAJOR})
set(FOO_VERSION_STRING
   "${FOO_VERSION_MAJOR}.${FOO_VERSION_MINOR}.${FOO_VERSION_PATCH}")

which really every developer using cmake understands.
So this macro in e-c-m is KDE-specific. It's of no use for anybody else.


Similar for an ecm_create_version_header() as it is now. It can be replaced 
with a trivial version.h.in and one call to configure_file().
configure_file() is easy to explain: it reads a file, replaces some stuff, and 
writes the output file. Then you can do with this file whatever you want.

Again, but here a bit less, there is no need to have such a macro, since it 
basically replaces one cmake call.

As I said, these are weak objections, I think a -0 on the scale you used.


Writing a Config.cmake file OTOH is IMO not trivial.
The developer has to at least know of the concept of exporting and importing 
targets in cmake, he has to care for relative and absolute 
CMAKE_INSTALL_PREFIX (now doable using configure_package_config_file()), he 
has to know how cmake searches packages, and he has to think about what 
information he should provide along with his library for users of his library. 
This needs thinking.

So my point is, to get this right, the developer needs to think anyway about 
what he is doing.
If manual work is required, merging information from two files doesn't make it 
easier than a plain straighforward file.

> > Having more buildsystem code is the price we have to pay for separating
> > the libraries from one big kdelibs to multiple independent libraries.
> 
> True, but we don't need to create more buildsystem code than we need to.
> 
> > 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.
> 
> I think it makes it easier to get right.
> 
> > Maybe we can find other means to achieve consistency ?
> > Like some tests or scripts which are part of the test suite ?
> 
> Maybe. I don't know if that's realistic though. Most people don't run the
> tests.
> 
> > Or some macro which checks something at the end of a CMakeLists.txt ?
> 
> Maybe. But then we'd expect the CMakeLists authors to put in a macro, and
> then they might wonder what it does etc, so it's not actually a solution to
> what you say the problem is.
> 
> > Some generic way to check whether an installed Config.cmake file follows
> > our conventions/expectations ?
> 
> Where would this check be? If it's generic, would we expect it to be
> dropped into a Config file or expect it to be a macro used after every
> find_package call, or do you want to wrap find_package with it? That would
> be even more divergent from the CMake-norm.

Maybe something like 
cmake --check-config-file /path/to/MyConfig.cmake
which checks whether an installed Config.file looks reasonable.
This would be not only useful for KDE, but for everybody installing libraries 
built with cmake.

These were a just a bunch of ideas, to get us thinking whether the same effect 
can be achieved with other means. Of course this were not yet solutions.
 
> > So, what do we expect from a Config.cmake file for a typical library ?
> > 
> > It should set
> > * Foo_INSTALL_PREFIX or Foo_INSTALL_DIR
> 
> What are these for? Ensuring that Bar installs to the same place?

So FindKF5.cmake can use it to search the following components in the same 
prefix. It may be useful in other situations too.
 
> > * Foo_INCLUDE_DIRS
> > * Foo_LIBRARIES
> 
> * Foo_INCLUDE_DIRS
> * Foo_LIBRARIES
> * Foo_VERSION_STRING
> * Foo_VERSION_MAJOR
> * Foo_VERSION_MINOR
> * Foo_VERSION_PATCH

I think Foo_VERSION_STRING is sufficient. For what do you need the others ?

I understand all your points about code duplication, copy'n paste, etc.
The difference is how we weight code duplication vs. straight-forwardness 
against each other.


I'm not at all against making Config.cmake files easier to create. But it 
should not make it in any way harder to add project-specific information to 
them (which IMO already happens if you end up with a somewhat working version 
without doing anything).


How about something like this:


set(FOO_INSTALL_DIR ${CMAKE_INSTALL_PREFIX})
set(FOO_INCLUDE_DIR include)
set(FOO_DATA_DIR share/foo/ )
set(FOO_WITH_Bat ${OPTION_BAT_ENABLED})
set(FOO_WITH_Blub ${OPTION_BLUB_ENABLED})
set(FOO_LIBRARIES "KF5::Foo;KF5::Blub")

ecm_write_basic_config_file(${CMAKE_BINARY_DIR}/FooConfig.cmake
                  PATH_VARS     FOO_INSTALL_DIR FOO_INCLUDE_DIR FOO_DATA_DIR
                  OTHER_VARS    FOO_WITH_Bat FOO_WITH_Blub FOO_LIBRARIES
                  TARGETS_FILE  FooLibraryTargets.cmake )


This could create a Config.cmake which handles the given PATH_VARS correctly, 
and which also sets the given OTHER_VARS to the values as they were when the 
macro was called.

This way we would not need a Config.cmake.in for every project.
Additionaly code we find out we need in a Config.cmake file could be added 
easily in one place (...but then this macro might turn into a 
ecm_write_kde_config_file()).


> > Usually it will do
> > include(${CMAKE_CURRENT_LIST_DIR}/FooLibraryTargets.cmake)
> > set(Foo_LIBRARIES Foo)
> > or
> > set(Foo_LIBRARIES SomeNamespace::Foo)
> 
> Yes. I also agree that the namespace is a good idea.
> 
> > 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.
> 
> I think KF5::ItemModels is enough.

Sounds good.

Alex