CMake Rework

[David Torres Sanchez]

Major rework of CMake for easier consumption in other projects. Minimum CMake version now 3.15.

The major benefit of this is that consumption of LEMON from other projects is now really easy using FetchContent as shown in the demo.

Also:

• Added copyright headers
• Explicit building options and disabled Maintainer options by default

[Alpar Juttner]

Thanks for the patch. It is a substantial refactoring indeed.

Some random comments, questions:

• Is there any reason for changing (almost) all cmake command to lower case?
• The testing does not work properly. BUILD_TESTING is not defined, and the custom target check has disappeared.
• It would be important to be able to build and run the tests even in release mode by make check. It regularly happens that a bug disappears when the optimization is turned off.
• The demos, docs, tools should also be possible to build in release mode.
• If fact, the Maintainer mode should be the same as Debug with the following two exceptions.
  • It uses -Werror compiler flag.
  • Automatically builds and runs the tests.

Actually, the Maintainer mode could even be a separate flag from CMAKE_BUILD_TYPE, thus it could be used in combination with any build type.

[David Torres Sanchez]

Thanks for the review!

I think it's pretty standard to have lower case CMake commands, I just used ​/ cmake-lang. With the default config commad_case='canonical' makes everything into lower case.

I will fix and update conform with those rules and resubmit the patch. Also, I'll add the files I missed.

[Alpar Juttner]

Replying to David Torres Sanchez:

I think it's pretty standard to have lower case CMake commands,

In fact, the policy has changed to the opposite since the fist version of the LEMON cmake config was made. I'm not a big fan of these kind of changes. Especially, because I find the new convention pretty ugly. The function names are case insensitive but "mandated" to be lower case, but e.g. STREQUAL is case sensitive and must be upper case.

Anyway, you are right that this seems to be the policy.

I just used ​/ cmake-lang. With the default config commad_case='canonical' makes everything into lower case.

I suggest doing this in a separate changeset. It wouldn't change any reformatting other than the lower case functions names. Those massive amount of white space changes make it very difficult to track the changes.

Another issue is the project versioning. Unfortunately, the "standard" way of adding a VERSION attribute to the project() call is not flexible enough as we also add the hg revision hash (and the number of the changesets since the last tagged version) for the development versions. In fact, this standard versioning scheme is so limiting that even CMAKE uses a custom scheme for its own versioning, see e.g. Brad King's comment here: ​https://gitlab.kitware.com/cmake/cmake/-/issues/16716#note_590854

[Alpar Juttner]

Replying to Alpar Juttner:

I suggest doing this in a separate changeset.

Please find it in the attached​ chset [76253ddee3ce].

[David Torres Sanchez]

Fixed attempt, added new option -> LEMON_MAINTAINER

[David Torres Sanchez]

Thanks for helping with this!

I've tried to adhere to all the suggestions, though I may have missed some.

• I added a new option LEMON_MAINTAINER to avoid setting a custom build type.
• Brought back the original way of versioning.
• Brought back the check target with its logic: runs automatically when LEMON_MAINTAINER is ON, ow is created but not run.
• Flags are all the same as original except for MinGW and MSVC flags (not sure if I got the logic for Debug vs Maintainer flags, but it is the same as the original just added as a list instead)
• Added all cmake files (I think!)

[Alpar Juttner]

Two questions about test/CMakeLists.txt, please.

1. What is the reason for using target_include_directories() instead of a global include_directories() setting?
   • Aren't similar settings missing now for lp_test and mip_test?
2. Could you please elaborate the set_target_properties(... PROPERTIES INSTALL_RPATH ...) settings? Why are they needed?

[Alpar Juttner]

I do not like the new way of configuration of optional parts, such as GLPK. If you run cmake -LA .., you see only one relevant setting - LEMON_ENABLE_GLPK. The fundamental variable GLPK_ROOT_DIR remaind hidden even when you run cmake -LA -DLEMON_ENABLE_GLPK=ON ... It only appears if you actually set it.

I think the related variables should always be there, even when LEMON_ENABLE_XYZ=OFF.

The right logic is that GLPK related stuff are built if and only if GLKP is properly discovered by FindGLPK and LEMON_ENABLE_GLPK=ON.

FindGLPK could be executed even when LEMON_ENABLE_GLPK=OFF, but at least corresponding CACHE variables should be created.

[Alpar Juttner]

Please find two changesets attached.

• Changeset 779318694d8b​ is just cmake_rework.2.diff​​ applied on top of 76253ddee3ce​
• Changeset ef70d6e9d8b6​ contains some bugfixes (missing files and tests), and applies the new-old logic for testing and configuring the LP stuff - basically as I described above.

Please review and comment.

[Alpar Juttner]

I forgot to mention that I also changed to config variable LEMON_MAINTAINER to LEMON_MAINTAINER_MODE.

[David Torres Sanchez]

Thanks Alpar!

Replying to Alpar Juttner:

Two questions about test/CMakeLists.txt, please.

1. What is the reason for using target_include_directories() instead of a global include_directories() setting?
   • Aren't similar settings missing now for lp_test and mip_test?

I think the use of target_include_directories is preferred as indicated in the note at the bottom of the CMake include_directories ​page.

2. Could you please elaborate the set_target_properties(... PROPERTIES INSTALL_RPATH ...) settings? Why are they needed?

Forgot to get rid of that, I don't it's necessary in this case as it's only in the test folder. It's for creating shared libraries and making sure that the package is relocatable across different OS (as the handling is different, for future reference see ​here).

Also, we need a fallback version for when hg is not found. e.g. add

else() # Hg not found and no environment variable is set
  set(LEMON_VERSION "unknown")
endif()

I can't seem to get the hang of patch files so if you change these two things that'd be grand. I'll keep an eye for the push to the main branch and then I'll pull directly from there once you're happy.

[Alpar Juttner]

Thanks for the answers. I noticed two more issues.

1. I think that PROJECT_VERSION must be set to ${LEMON_VERSION}. As least, PROJECT_VERSION is used to set the version string in lemon/config.h.in.
2. When setting LEMON_VERSION, it is CACHE at one if case and not at the other. It should be the same everywhere. The question is which one is better. If it is a cache variable, then we can overwrite it with -DLEMON_VERSION=, but ii also means that it doesn't update automatically to the current version. On the other hand, if it is not cached, it can still be overwritten using the OS environment (LEMON_VERSION=ver cmake ..), but it will only be effective until the next run of cmake.

Otherwise I'm happy with these changes.

I'm also considering to drop support of C++98, but I'll make a separate ticket about it.

[Szabolcs Horvát]

Could this be merged please?

I came here to suggest a few small changes to the build system, so that it would work better with recent CMake versions, but this patch already takes care of that.