COIN-OR::LEMON - Graph Library

Opened 5 weeks ago

Last modified 35 hours ago

#658 new enhancement

CMake Rework

Reported by: David Torres Sanchez Owned by: Alpar Juttner
Priority: minor Milestone: LEMON 1.4 release
Component: core Version: hg main
Keywords: Cc:
Revision id:

Description

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

Attachments (5)

cmake_rework.diff (72.9 KB) - added by David Torres Sanchez 5 weeks ago.
76253ddee3ce.patch (53.0 KB) - added by Alpar Juttner 4 weeks ago.
cmake_rework.2.diff (84.1 KB) - added by David Torres Sanchez 4 weeks ago.
Fixed attempt, added new option -> LEMON_MAINTAINER
779318694d8b.patch (73.4 KB) - added by Alpar Juttner 4 days ago.
ef70d6e9d8b6.patch (4.0 KB) - added by Alpar Juttner 4 days ago.

Download all attachments as: .zip

Change History (16)

Changed 5 weeks ago by David Torres Sanchez

Attachment: cmake_rework.diff added

comment:1 Changed 4 weeks ago by 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.

Last edited 4 days ago by Alpar Juttner (previous) (diff)

comment:2 Changed 4 weeks ago by 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.

comment:3 in reply to:  2 ; Changed 4 weeks ago by 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

Last edited 4 weeks ago by Alpar Juttner (previous) (diff)

Changed 4 weeks ago by Alpar Juttner

Attachment: 76253ddee3ce.patch added

comment:4 in reply to:  3 Changed 4 weeks ago by Alpar Juttner

Replying to Alpar Juttner:

I suggest doing this in a separate changeset.

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

Last edited 4 weeks ago by Alpar Juttner (previous) (diff)

Changed 4 weeks ago by David Torres Sanchez

Attachment: cmake_rework.2.diff added

Fixed attempt, added new option -> LEMON_MAINTAINER

comment:5 Changed 4 weeks ago by 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!)
Last edited 4 weeks ago by David Torres Sanchez (previous) (diff)

comment:6 in reply to:  5 ; Changed 4 days ago by 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?

comment:7 Changed 4 days ago by 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.

Changed 4 days ago by Alpar Juttner

Attachment: 779318694d8b.patch added

Changed 4 days ago by Alpar Juttner

Attachment: ef70d6e9d8b6.patch added

comment:8 Changed 4 days ago by Alpar Juttner

Please find two changesets attached.

Please review and comment.

comment:9 Changed 3 days ago by Alpar Juttner

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

comment:10 in reply to:  6 Changed 3 days ago by 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.

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

comment:11 Changed 35 hours ago by 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.

Note: See TracTickets for help on using tickets.