Modified build system for itemmodels

Stephen Kelly steveire at gmail.com
Tue May 1 21:03:18 UTC 2012


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.

> 
> 
>> > 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).

Yes, it is better than include(ECMQtFramework).

However, won't it leave us with the same visibility issue? If that's not a 
problem, then we can approach the problem for the best solution, not the 
most visible one.

> 
> Still, I wouldn't do it.
> The code for the version header is trivial, both on the cmake side as on
> the header side. 

Writing a ConfigVersion.cmake file is also trivial, but I'm glad it can be 
automated with one command.

This is the same. However, if we have a command for the version we can do 
much more. We can create a source file which we then compile into the 
frameworks so that they get the right version number, we can create a 
<framework>_DEPRECATED_SINCE macro, similar to what Qt has etc etc. If the 
way to create these things is copy+paste, then we will get divergent messes.

This can also be sufficiently visible by taking a variable to store the 
source file etc:

ecm_set_version(itemmodels version_file 5.1.2)

add_library(itemmodels ${version_file} ${itemmodels_SRCS})

> I think any developer using cmake is able to do that.

Maybe, but not your typical KDE developer.

> 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.

That's not all it has to do though.

> 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,

... and preventing divergent messy copy+pasted 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. 

And if you see it 20 times (once for each framework), that doesn't mean you 
have to check 20 times.

If we have copy+pasted+modified stuff, you will have to check each framework 
to see what it's 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.

I don't think I share that goal. I'd prefer it to be easy to get right, and 
that means less copy+paste.

> 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.

> 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.

> 
> 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?

> * Foo_INCLUDE_DIRS
> * Foo_LIBRARIES

* Foo_INCLUDE_DIRS
* Foo_LIBRARIES
* Foo_VERSION_STRING
* Foo_VERSION_MAJOR
* Foo_VERSION_MINOR
* Foo_VERSION_PATCH

If at some point, it makes sense to add others which are generic, we can 
that once in the generated section instead of 20 times.

> 
> 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.

> 
> Alex

Thanks,

Steve.





More information about the Kde-frameworks-devel mailing list