Fix CMake config generation problems #3455
Conversation
Backport of assimp/assimp#3455 to v5.0.1
Backport of assimp/assimp#3455 to v5.0.1
Backport of assimp/assimp#3455 to v5.0.1
|
@traversaro we can alias |
| @@ -0,0 +1,9 @@ | |||
| @PACKAGE_INIT@ | |||
|
|
|||
| include("${CMAKE_CURRENT_LIST_DIR}/@TARGETS_EXPORT_NAME@.cmake") | |||
There was a problem hiding this comment.
You can also put conditionals into the config file, so you can consolidate the Hunter and non-Hunter config files. See https://github.com/cpp-pm/meshoptimizer/blob/hunter-0.14-a507623/config.cmake.in#L3 for an example.
There was a problem hiding this comment.
Do you think it is more readable with the conditional ? I personally find that two separate files will help in avoid confusion, also because I imagine that the assimp-plain-config.cmake variant will get more logic with time. For example, in a coming PR I want to also switch ZLIB to use the imported targets provided by the official CMake FindZLIB, and I don't think that mixing find_package(ZLIB CONFIG REQUIRED) and find_package(ZLIB MODULE REQUIRED) in the same file would be helpful for readability and maintainability, but this is just my opinion.
There was a problem hiding this comment.
Typically, only CMake reads these config files. In addition, the @ASSIMP_HUNTER_ENABLED@ will be set to ON or OFF in the generated config file, in case a human wants to read it. I think it is worth consolidating everything and simplifying the config file generation into a single one.
There was a problem hiding this comment.
Typically, only CMake reads these config files. In addition, the
@ASSIMP_HUNTER_ENABLED@will be set toONorOFFin the generated config file, in case a human wants to read it. I think it is worth consolidating everything and simplifying the config file generation into a single one.
Sorry, regarding the readability comment I was referring to the .cmake.in file that will be read by any contributor that needs to modify it for the future. I find that having the two separate files is more mantainable, but it is just a personal opinion, anything that is ok for the Assimp mantainers is ok for me.
I don't know, |
However, I think it could make sense to define |
|
More in general, it seems that the However I don't want to start bikeshadding about the CMake namespace, so any solution that is fine to the mantainer is fine for me. |
Use standard CMake function to generate the CMake config files.
|
We can keep the |
I think that make sense, especially if Hunter is ensured to always be used with CMake >= 3.11 . |
|
However, I think that reducing the amount of code duplication between Hunter and non-Hunter can also done in future PRs, as the PR is already quite complex as it is. Perhaps we can open a dedicated issue for that? |
|
Merged, thanks a lot for your contribution. |
Thanks a lot @kimkulling ! |
Use standard CMake functions to generate the CMake config files.
In particular, this PR fixes the following issues:
Fix #1517
Fix #3260
Fix #3365
In particular, it reduces the difference between the normal CMake build and the CMake build with Hunter enabled (option
ASSIMP_HUNTER_ENABLEDenabled). Unfortunately the Hunter build at the moment generates imported targets that are namedAssimp::assimpwhile the regular build have generated imported targets namedassimp::assimpfor a long time, so it is still necessary to account for that for backcompatibility.