Modified build system for itemmodels

[Stephen Kelly]

Alexander Neundorf wrote:
>> >> 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 ?

Thanks. Nope they're not used already afaik.

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

It is also intended to create a cpp file with the version which can be 
compiled into the targets in the module.

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

The intention is for it to do more.

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

Right.

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

Yes.

> So my point is, to get this right, the developer needs to think anyway
> about what he is doing.

Yes.

> If manual work is required, merging information from two files doesn't
> make it easier than a plain straighforward file.

If someone knows what they're doing, and they see the same pattern 20 times 
in config files, I don't see the problem with creating the common parts 
using an include(Foo-ConfigCommon.cmake) or macro(set_common_variables).



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

Would it ever be invoked automatically, or just manually?

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

Then I'd prefer the _PREFIX one.

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

cmake/Modules/readme.txt says it should exist.

Additionally, grepping in kdelibs shows that _VERSION_MAJOR variables are 
used sometimes.

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

Yes.

If people creating the config files know what they're doing though, as you 
suggest, then reducing duplication is preferable.

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

This would hardcode it in the Config file, making the package non-
relocatable.

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


I'd prefer not to have a ecm_write_basic_config_file and to instead have a 
Config.in file with something like this:

@PACKAGE_INIT@

kf5_do_common_stuff() # TODO: Get better name

# Use of COMPONENTS is uncommon in KDE, outside of 
# the umbrella KF5Config.cmake
set(FOO_WITH_Bat @OPTION_BAT_ENABLED@) 
set(FOO_WITH_Blub @OPTION_BLUB_ENABLED@)

set(FOO_LIBRARIES "KF5::Foo;KF5::Blub")

macro(foo_macro ...)
endmacro()

...

macro(kf5_do_common_stuff)
 set(${PACKAGE_FIND_NAME}_VERSION_STRING @itemmodels_VERSION_STRING@ )
 set(${PACKAGE_FIND_NAME}_VERSION_MAJOR @itemmodels_VERSION_MAJOR@ )
 set(${PACKAGE_FIND_NAME}_VERSION_MINOR @itemmodels_VERSION_MINOR@ )
 set(${PACKAGE_FIND_NAME}_VERSION_PATCH @itemmodels_VERSION_PATCH@ )

 set_and_check(${PACKAGE_FIND_NAME}_INSTALL_PREFIX 
"${CMAKE_CURRENT_LIST_DIR}")
 set_and_check(${PACKAGE_FIND_NAME}_INCLUDE_DIR    "include")

 include("${CMAKE_CURRENT_LIST_DIR}/${PACKAGE_FIND_NAME}Targets.cmake")
endmacro()

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

It also sort of turns into a macro with a lot of API. How would I define 
macros in my Config file or include() macros file, or do any of the other 
powerful things that Config files allow me to do?

These questions might be answered in your follow up. I'll read that now.

Thanks,

Steve.