Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CMake package config file #3068

Merged
merged 2 commits into from
Jan 14, 2022
Merged

Fix CMake package config file #3068

merged 2 commits into from
Jan 14, 2022

Conversation

gmgunter
Copy link
Contributor

There are currently a few issues with how the COIN-OR dependencies are resolved when using find_package(ortools CONFIG) to add or-tools to an external CMake project.

  • With CMake >=3.9.6, we try to find the Clp and Cbc packages using the CONFIG mode of find_package. This always fails since neither coin-or/Clp nor coin-or/Cbc provides a CMake config file.

  • If we address the above issue by instead using the MODULE mode of find_package, then we need to add FindCbc.cmake and FindCpl.cmake scripts to the CMAKE_MODULE_PATH in order to teach CMake how to find these dependencies. I propose that or-tools should install these scripts alongside its config file so that they're available to external CMake projects.

  • Finally, the FindCbc.cmake script included with or-tools defines the variable CBC_FOUND when it's successful, rather than Cbc_FOUND as expected (CMake variables are case-sensitive). The FindCpl.cmake script has a similar issue. As a result, even when the above two points are addressed, find_package(ortools CONFIG) will still fail because CMake erroneously thinks that these two dependencies weren't succesfully found.

This PR attempts to address the above issues with the following changes:

  • The CMake config file is modified to allow searching for Cbc and Cpl using the MODULE mode of find_package.

  • FindCbc.cmake and FindCpl.cmake are installed with the CMake package config files and are added to the CMAKE_MODULE_PATH if USE_COINOR was truthy.

  • The FindCbc.cmake and FindCpl.cmake files are modified to change the case of the variables they export.

There are currently a few issues with how the COIN-OR dependencies are
resolved when using `find_package(ortools CONFIG)` to add or-tools to
an external CMake project.

* With CMake >=3.9.6, we try to find the Clp and Cbc packages using the
  CONFIG mode of `find_package`. This always fails since neither
  library provides a CMake config file.

* If we address the above issue by instead using the MODULE mode of
  `find_package`, then we need to add `FindCbc.cmake` and
  `FindCpl.cmake` scripts to the CMAKE_MODULE_PATH in order to teach
  CMake how to find these dependencies. I propose that or-tools should
  install these scripts alongside its config file so that they're
  available to external CMake projects.

* Finally, the `FindCbc.cmake` script included with or-tools defines the
  variable `CBC_FOUND` when it's successful, rather than `Cbc_FOUND` as
  expected (CMake variables are case-sensitive). The `FindCpl.cmake`
  script has a similar issue. As a result, even when the above two
  points are addressed, `find_package(ortools CONFIG)` will still fail
  because CMake erroneously thinks that these two dependencies weren't
  succesfully found.

This commit attempts to address the above issues with the following
changes:

* The or-tools CMake config file is modified to allow searching for Cbc
  and Cpl using the MODULE mode of `find_package`.

* `FindCbc.cmake` and `FindCpl.cmake` are installed with the CMake
  package config files and are added to the CMAKE_MODULE_PATH if
  `USE_COINOR` was truthy.

* The `FindCbc.cmake` and `FindCpl.cmake` files are modified to change
  the case of variables they export.
@Mizux
Copy link
Collaborator

Mizux commented Jan 11, 2022

  1. I've forked coin-or repos (Cbc, Clp, Cgl, Osi, CoinUtils) few years ago to add CMake support, they never want to integrate it...
    see: Add CMake-based build support coin-or/CoinUtils#101
    exhaustive list: Add CMake-based build support coin-or/CoinUtils#101 (comment)

    1. Currently we only test/use with options USE_COINOR=ON and BUILD_DEPS=ON aka using coin-or through
      FetchContent()
  2. vcpkg already contains a CMake port for Clp but it's not complete...
    src: https://github.com/microsoft/vcpkg/tree/master/ports/clp

  3. Good catch for CBC/Cbc and CLP/Clp need to double check pkg_check_modules

    1. Need a job to test the CMake based build of or-tools against coin-or installed system-wide
      e.g. using an ubuntu 20.04 LTS image
      ref: https://repology.org/project/coin-or-cbc/versions

@Mizux Mizux self-assigned this Jan 11, 2022
@Mizux Mizux added Build: CMake CMake based build issue Feature Request Missing Feature/Wrapper labels Jan 11, 2022
@Mizux Mizux added this to the Backlog milestone Jan 11, 2022
@gmgunter
Copy link
Contributor Author

gmgunter commented Jan 12, 2022

I've forked coin-or repos (Cbc, Clp, Cgl, Osi, CoinUtils) few years ago to add CMake support, they never want to integrate it...

That's too bad. The most elegant solution would definitely be for these upstream packages to add CMake support.

Need a job to test the CMake based build of or-tools against coin-or installed system-wide

This would be really nice. I installed or-tools via the conda-forge package, which builds with BUILD_DEPS=OFF and relies on finding independently installed packages for all the dependencies.

@Mizux Mizux merged commit 68f4e80 into google:master Jan 14, 2022
@Mizux
Copy link
Collaborator

Mizux commented Jan 19, 2022

FYI I'll need to change the module CMake since:

  1. It does not support super build i.e. if Cbc target already present the module should be a noop
  2. It does not support cbc_config.cmake file (like we have when building cbc or using vcpkg) so first module should try a to perform a find_package( CONFIG) then if found -> nothing to do, otherwise try to locate it using the pkg plugin...

On my way to fix it, if you have some regression in your own workflow, please let me know...

@gmgunter
Copy link
Contributor Author

Sounds good 👌

@gmgunter gmgunter deleted the cmake-config-fix branch January 19, 2022 18:43
@Neumann-A
Copy link

@Mizux I'll probably be the one to rip out all the CMakeLists.txt vcpkg vendors. Including any exported targets which have been added and are currently in disagreement with the maintainer guidelines of vcpkg. Either you get upstream to accept your CMake versions or pkg-config will be the preferred way to consume coin-or libraries.

@Mizux
Copy link
Collaborator

Mizux commented Feb 3, 2023

@Neumann-A

  1. Do you have a link to this maintainer guidelines assertion ?
  2. Does pkg-config build will work on Windows and nicely integrate with vcpkg ?
  3. How to deal (nicely) with a CMake toolchain file if using pkg-config/autotools based build ?
  4. How about having a Community supported CMake fork ?

note: here pkg-config/autotools based build means what coin-or provide if I understand correctly your point
note2: IIRC it was a user of openframework who provide a first ugly CMakeLists.txt patch on vcpkg

@Neumann-A
Copy link

Do you have a link to this maintainer guidelines assertion ?

a) The current name of most coin-or ports in vcpkg is wrong according to https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#check-names-against-other-repositories (basically the ports need a prefix coin-or-*

b) unofficial:: namespace: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#add-cmake-exports-in-an-unofficial--namespace

Does pkg-config build will work on Windows and nicely integrate with vcpkg ?
How to deal (nicely) with a CMake toolchain file if using pkg-config/autotools based build ?

vcpkg has a port for pkgconf. If it is installed CMake'sFindPkgConfig will automatically start working. be aware that it breaks multi-config usage (e.g. no interactive switching of the build type, as long as you stay with one build type you are fine). For ports internal to vcpkg it will start to automatically work due to how the vcpkg_ function are setup and if additional downstream usage agrees with upstream provided usage.

How about having a Community supported CMake fork ?

No. Upstream it or leave it. Source packages have to come from the authoritative source.

So I already started ripping it out: microsoft/vcpkg#29398. basically the only thing left to do is fixing openmvg and getting the linux VM to have autoconf 2.71

@Mizux Mizux modified the milestones: Backlog, v9.10 Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build: CMake CMake based build issue Feature Request Missing Feature/Wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants