Modified build system for itemmodels

Sun May 6 12:11:40 UTC 2012

On Sunday 06 May 2012, Stephen Kelly wrote:
> 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?

No concrete plans yet.
Manually would already be a big improvement over the current situation, where 
you have to write your own example user for each Config file you install, to 
see whether it works.

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

Ok.

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

Ok.
I think I haven't needed the components variables for some time.
Especially since the ConfigVersion.cmake files exist, and since if() supports 
the version-comparisons.

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

No.
It's relocatable using the @PACKAGE_INIT@ stuff. In the Config.cmake.in it 
will be
set(FOO_INSTALL_DIR @PACKAGE_FOO_INSTALL_DIR@)
which will be turned into a relocatable location via @PACKAGE_INIT at .

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

Where should this macro come from ?
Should this be expanded from @PACKAGE_INIT@ or from an included file ?
From an included file would be bad, this would add compatibility issues for 
the included file.
It can't be in @PACKAGE_INIT@, we would need something like an additional 
@KDE_PACKAGE_INIT@ . This goes against the goal of reducing the KDE-specific 
stuff in KDEs buildsystem.

extra-cmake-modules is an extension of cmake, not an extension of KDE.

We have two conflicting goals here:
* reduce the KDE-specific stuff in our buildsystem
* make maintaining many buildfiles in KDE more convenient

Additionally we have the restriction that we must not have a common 
(buildtime) dependency for all of kdelibs/frameworks, i.e. a kf5-cmake package 
where such stuff could be put.

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

Slightly modified:

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
                  FILES_TO_INCLUDE  FooLibraryTargets.cmake;FooMacros.cmake
                  EXTRA_CONFIG_STRING "#something more for the Config file" )

You can put the macros into FooMacros.cmake, which would be included by the 
Config.cmake file, or you could put them into EXTRA_CONFIG_STRING.

Alex