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

[assimp] assimp CMake package name is Assimp, not consitent with other platforms where it is assimp #14256

Closed
traversaro opened this issue Oct 27, 2020 · 3 comments · Fixed by #14554
Assignees
Labels
category:port-bug The issue is with a library, which is something the port should already support

Comments

@traversaro
Copy link
Contributor

traversaro commented Oct 27, 2020

Describe the bug
The assimp vcpkg port install a CMake config file called AssimpConfig.cmake, that is not consistent with the assimp-config.cmake installed by other package managers such as homebrew or apt.

Environment

  • OS: Windows
  • Compiler: Not Relevant

To Reproduce
Steps to reproduce the behavior:

  1. ./vcpkg install assimp
  2. In share/assimp you can find the AssimpConfig.cmake file, differently from apt and homebrew that install an assimp-config.cmake file (see for example https://packages.debian.org/sid/amd64/libassimp-dev/filelist)

Expected behavior
I would expect the vcpkg port to install a assimp-config.cmake file as well.

Failure logs
-(please attached failure logs)

Additional context
assimp CMake config files have been historically very problematic. PR #13264 solve most of the problems, but introduced this regression.

@traversaro traversaro changed the title assimp CMake package name is Assimp, not consitent with other platforms where it is assimp [assimp] assimp CMake package name is Assimp, not consitent with other platforms where it is assimp Oct 27, 2020
@LilyWangL LilyWangL self-assigned this Oct 28, 2020
@LilyWangL LilyWangL added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Oct 29, 2020
@PhoebeHui
Copy link
Contributor

@traversaro, the official config name update to assimpConfig.cmake, so we may not change it to assimp-config.cmake

See https://github.com/assimp/assimp/blob/master/CMakeLists.txt#L406.

@traversaro
Copy link
Contributor Author

@traversaro, the official config name update to assimpConfig.cmake, so we may not change it to assimp-config.cmake

See https://github.com/assimp/assimp/blob/master/CMakeLists.txt#L406.

I know, I was the one adding that line (see assimp/assimp#3455). : )

Both assimpConfig.cmake and assimp-config.cmake for me are ok, as it means that the official way of finding the searching for assimp is:

find_package(assimp REQUIRED)

The problem is that instead vcpkg is suggesting to use find_package(Assimp REQUIRED) that, is not compatible with the config file names assimpConfig.cmake . I suggested to use assimp-config.cmake just because the -config variant will work (on case sensitive filesystem) with both find_package(Assimp) and find_package(assimp), providing back-compatibility. This is because ***Config.cmake and ***-config.cmake files are not equivalent, but rather (from https://cmake.org/cmake/help/v3.19/command/find_package.html#full-signature-and-config-mode):

The command searches for a file called <PackageName>Config.cmake or <lower-case-package-name>-config.cmake for each name specified.

So, brief recap:

  • Switching to assimp-config.cmake as originally suggested by this issue ensures back-compatibility with both find_package(assimp) and find_package(Assimp)
  • Using assimpConfig.cmake as suggested by @PhoebeHui for me is perfectly fine, the important thing (and the main point of the issue) is not using AssimpConfig.cmake

@PhoebeHui
Copy link
Contributor

I will summit a PR to fix this, thanks for bringing this up!

@PhoebeHui PhoebeHui added category:port-bug The issue is with a library, which is something the port should already support and removed category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants