Skip to content
Snippets Groups Projects

WIP: Change the way dune module versions are checked in cmake

Open Simon Praetorius requested to merge feature/version-config-files into master
3 unresolved threads

Summary

In CMake we parse the dune.module file, extract the dependencies and their versions and perform a manual version check for find_package. This is the second time versions of modules are checked and the way it is implemented is not compatible to dunecontrol, see #173. This MR proposes a different workflow:

  • A dune module knows best to which version it is compatible. Typically, this compatibility is SameMinorVersion.
  • We let cmake automatically generate dune-<module>-config-version.cmake files using write_basic_package_version_file with a specified version compatibility.
  • When configuring a module and when searching for its dependencies, we use find_package(<module> <version>) with just the version string we want to be compatible with.

Closes #173

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • One question is: why do we check versions at all again. The module version compatibility is already checked by dunecontrol.

    • The actual question is, should we remove the version check from CMake or from dunecontrol? dunecontrol has some bugs like #112 that might be easier get fixed within CMake.
      From a historic point of view, we started with the CMake modules without dunecontrol and added it later. And on Windows I worked without dunecontrol. That are reasons, but no arguments to not change it today.

    • We should at least not have two different implementations of a version check (and maybe not run the version check twice). I'm not sure whether the cmake code is better readable than the dunecontrol shell script. It is also completely undocumented code in cmake.

    • Please register or sign in to reply
  • added 1 commit

    • 0dfba6d0 - Extract the >= version from the version relation string

    Compare with previous version

    • I like the changes. But I am under the impression, that the dune.module file is leading. And whatever is written there, is the truth. CMake has to conform with this. Will CMake be able to represent all varieties of dune.module version strings? From reading the code, I don't know.

    • Is the dune.module version format specified anywhere? Or is it again just guessing which format should be supported? Is it compatible to debian package format version requirements? Are there all logical operators allowed? Are brackets allowed? Do we need to implement a full logical expression parser? I don't know. Thus, neither dunecontrol nor cmake is wrong nor correct. They just parse the version string differently.

      I have simplified the version check in cmake to a simple compatibility definition. Request version x.y.z and allow all version that are compatible w.r.t. some contract that is properly defined. The default contract is "SameMinorVersion", but each dune module could choose its own compatibility contract, either from the list of default cmake compatibility formats or by providing a version file template itself.

    • The problem with current version specification in dune.module is that we have to translate this to multiple other formats, e.g. debian package format, pkg-config format, cmake find_package format and maybe more. All these formats are possibly different and allow different logical relations.

    • For the book I had to read the code to understand how those versions work. I would prefer a lot if we could settle on some standard format.

    • As we check the version compatibility at three places (dunecontrol, CMake code to create the acyclic graph of dependency, CMake's find_package for Dune modules), we should reduce this to a singe point and call this code from the other places.

    • Please register or sign in to reply
  • An example, where my approach behaves differently to the old one, is the pdelab pipeline: The master branch of pdelab requests versions >= 2.7, while the pdelab version is 2.8-git and all the other modules also have a 2.8 developer version. This is by our versioning scheme typically not classified as compatible. Thus, the cmake configuration crashes with an error of incompatible version:

    Could not find a configuration file for package "dune-grid" that is
    compatible with requested version "2.7".

    Is pdelab really compatible to 2.7 or is this just an error in the version specification?

    So, one question remains: How to specify a version requirement that violates the compatibility statement of a module? E.g. you have tested your module with dune 2.6, 2.7, and master. This needs to be investigated.

  • added 1 commit

    Compare with previous version

    • Is pdelab really compatible to 2.7 or is this just an error in the version specification?

      The curreent PDELab works with core 2.7 and the development branch.

      To be totally correct, we would have to specify something like "dune-grid >=2.7 && <2.9". But this is something that didn't work up to now and also didn't cause many problems. Still to specify a minimal version is something done by many downsteam modules and often people try to stay compatible to a range of versions

      We should not break this behavior in cmake.

    • I disagree. Having multiple conditions make the code complicated for little gain. Speaking from a CMake perspective, if = and >= is not enough, you should manually write the code you need to process the version number.
      Edit I think we don't disagree, you want to have = and >= and that's all, right?!

      Edited by Christoph Grüninger
    • @christi you are right, we should not break behavior in cmake that is introduced in other places. That is why this situation needs further investigation.

      My proposal is just an experiment. Maybe it can not work with our more involved version constraints. After thinking about it a bit more, I think that having the version contract defined by the module itself covers only half the story. Your restriction of a wider version range could not be covered by this approach easily. The module can only specify that it tries to have a stable interface within the minor version number, e.g. 2.7 and 2.7.1 and 2.7.9 should be interface compatible. This is what the "contract" specifies. So if you ask for a package version compatible to, e.g., 2.7. CMake accepts everything of the version 2.7.x if SameMinorVersion is specified.

      But this does not cover the requirement that we want to support a wider version range, e.g., 2.6-2.8, for some packages - because we have tested this or have implement version switches or use only a subset of functions or ...

      A solution could be, that we do not test for compatible version in find_package of CMake directly, but perform a version compatibility check afterwards. This, however, needs to be a compatible check to what dunecontrol performs, maybe even using the same code for the check.

    • Hi Simon, I didn't want to sound harsh.

      I think it good to clean up this part of the buildsystem, but it is a bit more involved.

      I suggest, that we

      • first write down a proper definition of the supported version strings
      • we should try to stay compatible with the current dunecontrol parser
      • ideally we should use the same code to do the version check in cmake and in dunecontrol
      • lastly we should then also add support to read these version strings and transform them to version strings of other systems, e.g. pkg-config.
    • I agree :thumbsup: Whenever I find some time, I will try to write things down.

    • Please register or sign in to reply
  • mentioned in merge request !1249 (merged)

Please register or sign in to reply
Loading