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

cmake_find_package_multi and version information #5888

Closed
3 tasks done
Morwenn opened this issue Oct 10, 2019 · 13 comments · Fixed by #6063
Closed
3 tasks done

cmake_find_package_multi and version information #5888

Morwenn opened this issue Oct 10, 2019 · 13 comments · Fixed by #6063

Comments

@Morwenn
Copy link
Contributor

Morwenn commented Oct 10, 2019

To help us debug your issue please explain:

  • I've read the CONTRIBUTING guide.
  • I've specified the Conan version, operating system version and any tool that can be relevant.
  • I've explained the steps to reproduce the error or the motivation/use case of the question/suggestion.

I was writing a Conan recipe for ITK 5 which uses Eigen3 and requires it through the following CMake line (see the corresponding issue for more background information):

find_package(Eigen3 REQUIRED CONFIG)

Long story short, after having bypassed a recipe-specific issue, I managed to get CMake to find the right CMake config file generated with the Conan generator cmake_find_package_multi, but the configuration step eventually failed with the following error:

CMake Error at source_subfolder/Modules/ThirdParty/Eigen3/CMakeLists.txt:56 (find_package):
  Could not find a configuration file for package "Eigen3" that is compatible
  with requested version "3.3".

  The following configuration files were considered but not accepted:

    ~/.conan/data/itk/5.0.1/myteam/stable/build/a9e815bf664154117de467f08af021beb20b7cc4/Eigen3Config.cmake, version: unknown

The recipe I used was that for Eigen 3.3.7, so Conan has the version information, but it seems that this information wasn't propagated to the files generated by cmake_find_package_multi, triggering the error above.

Would it be possible to forward the version information to the files generated by this generator so that examples like the one above work?

Conan version: 1.19.1
CMake version: 3.10.2

@lasote lasote self-assigned this Oct 10, 2019
@lasote
Copy link
Contributor

lasote commented Oct 10, 2019

From the CMake docs I read that we should be generating a <config-file>-version.cmake or <config-file>Version.cmake adjusting the following variables, right?

PACKAGE_VERSION: full provided version string
PACKAGE_VERSION_EXACT: true if version is exact match
PACKAGE_VERSION_COMPATIBLE: true if version is compatible
PACKAGE_VERSION_UNSUITABLE: true if unsuitable as any version

In case I could guess we should only fill them with true if the version declared at <package>_VERSION is the same that the package one.

Sounds good?

@lasote
Copy link
Contributor

lasote commented Oct 10, 2019

Could you try manually to generate the file setting these vars and letting us know if it works that way?
And also, I have a question, why is it looking for the version file if you didn't specify the [version] argument to the find_package? what is the switch that starts requiring the version file?

@SSE4
Copy link
Contributor

SSE4 commented Oct 10, 2019

as I can see ITK uses version argument: https://github.com/InsightSoftwareConsortium/ITK/blob/8fe66a3f1f5366e656b77773b4db3f349af6d644/Modules/ThirdParty/Eigen3/CMakeLists.txt#L56

set(_Eigen3_min_version 3.3)
...
  find_package(${_Eigen3_SYSTEM_OR_INTERNAL} ${_Eigen3_min_version} REQUIRED CONFIG)

@Morwenn
Copy link
Contributor Author

Morwenn commented Oct 10, 2019

I think that my original error was triggered by this file: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/Eigen3/itk-module-init.cmake

But it would make sense that the one I'm experiencing now is linked to the one @SSE4 linked.

I'm going to try to the proposed solution and report, but I think that it's the right thing to do :)

@lasote
Copy link
Contributor

lasote commented Oct 10, 2019

Then I think it is more a feature than a bugfix. With high priority, I would say. In the meantime, you could try to patch (from the recipe) that line to remote the version requirement from that sentence. (That by the way, in the scope of a package manager, makes no sense, because the version resolution is a package manager thing, not a build system thing)

@SSE4
Copy link
Contributor

SSE4 commented Oct 10, 2019

contents of Eigen3ConfigVersion.cmake:

# This is a basic version file for the Config-mode of find_package().
# It is used by write_basic_package_version_file() as input file for configure_file()
# to create a version-file which can be installed along a config.cmake file.
#
# The created file sets PACKAGE_VERSION_EXACT if the current version string and
# the requested version string are exactly the same and it sets
# PACKAGE_VERSION_COMPATIBLE if the current version is >= requested version,
# but only if the requested major version is the same as the current one.
# The variable CVF_VERSION must be set before calling configure_file().


set(PACKAGE_VERSION "3.3.7")

if(PACKAGE_VERSION VERSION_LESS PACKAGE_FIND_VERSION)
  set(PACKAGE_VERSION_COMPATIBLE FALSE)
else()

  if("3.3.7" MATCHES "^([0-9]+)\\.")
    set(CVF_VERSION_MAJOR "${CMAKE_MATCH_1}")
  else()
    set(CVF_VERSION_MAJOR "3.3.7")
  endif()

  if(PACKAGE_FIND_VERSION_MAJOR STREQUAL CVF_VERSION_MAJOR)
    set(PACKAGE_VERSION_COMPATIBLE TRUE)
  else()
    set(PACKAGE_VERSION_COMPATIBLE FALSE)
  endif()

  if(PACKAGE_FIND_VERSION STREQUAL PACKAGE_VERSION)
      set(PACKAGE_VERSION_EXACT TRUE)
  endif()
endif()


# if the installed or the using project don't have CMAKE_SIZEOF_VOID_P set, ignore it:
if("${CMAKE_SIZEOF_VOID_P}" STREQUAL "" OR "" STREQUAL "")
   return()
endif()

# check that the installed version has the same 32/64bit-ness as the one which is currently searching:
if(NOT CMAKE_SIZEOF_VOID_P STREQUAL "")
  math(EXPR installedBits " * 8")
  set(PACKAGE_VERSION "${PACKAGE_VERSION} (${installedBits}bit)")
  set(PACKAGE_VERSION_UNSUITABLE TRUE)
endif()

@Morwenn
Copy link
Contributor Author

Morwenn commented Oct 10, 2019

I was about to copy-paste the very same file here: exporting it from the recipe directory made the version test pass correctly, so I guess that it would be enough for cmake_find_package_multi :)

Now I have another issue because it looks for Eigen3::Eigen but the target generated is most likely Eigen3::Eigen3. It's another issue and I'm not sure that there is an easy workaround from the Conan side, I'll patch my ITK CMake instead.

@Morwenn
Copy link
Contributor Author

Morwenn commented Oct 10, 2019

Also if cmake_find_package_multi generates version files it better not reproduce the CMAKE_SIZEOF_VOID_P check for header-only libraries. It's a known issue in CMake/Conan compat for header-only libraries, generally requires workarounds, and it's probably part of the reason why write_basic_package_version_file got an ARCH_INDEPENDENT parameter in the first place.

@lasote
Copy link
Contributor

lasote commented Oct 10, 2019

Now I have another issue because it looks for Eigen3::Eigen but the target generated is most likely Eigen3::Eigen3. It's another issue and I'm not sure that there is an easy workaround from the Conan side, I'll patch my ITK CMake instead.

No, we have a new cppinfo.name but it changes both scope and name: Eigen3XXX::Eigen3XXX at the same time... We tried to create the concept of components in the cppinfo but it is a blocked feature for several different reasons: #5090

@memsharded
Copy link
Member

#6025 (comment) by @liarokapisv suggests unban Version.cmake files from CCI or to generate them automatically.

@lasote
Copy link
Contributor

lasote commented Nov 6, 2019

As I commented, it makes sense to generate the files in the cmake_find_package/cmake_find_package_multi

@lasote lasote added this to the 1.21 milestone Nov 6, 2019
@lasote lasote removed their assignment Nov 6, 2019
@lasote
Copy link
Contributor

lasote commented Nov 6, 2019

@SSE4 do you want to work on this for the next release?

@SSE4
Copy link
Contributor

SSE4 commented Nov 6, 2019

@lasote yes, sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants