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

fmt, spdlog: Keep the CMake config file around after packaging #1392

Closed
wants to merge 1 commit into from
Closed

fmt, spdlog: Keep the CMake config file around after packaging #1392

wants to merge 1 commit into from

Conversation

neobrain
Copy link
Contributor

Specify library name and version: fmt/all, spdlog/all

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

I couldn't find any reason why the Conan recipes explicitly remove the CMake config files for these libraries, but they are needed to make find_package(fmt) and find_package(spdlog) work flawlessly in consumers of these libraries. If the files are removed, CMake won't find Conan's packaged library.

I've done a local test build of these and everything works fine in the default configuration.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

@neobrain
Copy link
Contributor Author

Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

I wish this had been mentioned in the project README. I couldn't find any note about it on the wiki either.

I won't be able to do a legal CLA review for such a small change (which clearly is not subject to Copyright anyway). If this is a blocker, please close this PR and let's discuss how to proceed instead.

@gocarlos
Copy link
Contributor

Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

I wish this had been mentioned in the project README. I couldn't find any note about it on the wiki either.

I won't be able to do a legal CLA review for such a small change (which clearly is not subject to Copyright anyway). If this is a blocker, please close this PR and let's discuss how to proceed instead.

Hi @neobrain,

conan removes all cmake stuff because conan auto generates those for cmake, meson, pkgconfig etc.

the only thing you need to do is to add

[generators]
cmake_find_package
cmake_paths

to your conanfile.txt to generate the FindSpdlog.cmake and Findfmt.cmake

@neobrain
Copy link
Contributor Author

Hi @gocarlos , thanks for the quick reply!

I didn't know these generators intend to replace the exported CMake config files. I tried them out for my project and indeed they generate useful output, but I feel like it's opening an unrelated can of worms now.

Are Find*.cmake files generated by Conan supposed to be consistent with the Config files provided by upstream projects? At least for Boost this does not seem the case [1]. Would this be considered a bug in the boost recipe or is this simply not in scope of the Conan generators? I find it important not to tie my CMakeLists.txt too closely too Conan, and that would become impossible if needed to use different package discovery code for Conan and non-Conan.

Meanwhile, is there a reason the upstream CMake config files need to be removed? It seems for the "cmake" generator there's no option to do clean package discovery without them, and spdlog/fmt are the first packages (of CMake-based libraries) I encounter that do not provide those files.

[1] Conan generates Findboost.cmake, but CMake consumers typically require FindBoost.cmake (as per upstream)

@gocarlos
Copy link
Contributor

Hi @gocarlos , thanks for the quick reply!

I didn't know these generators intend to replace the exported CMake config files. I tried them out for my project and indeed they generate useful output, but I feel like it's opening an unrelated can of worms now.

Are Find*.cmake files generated by Conan supposed to be consistent with the Config files provided by upstream projects? At least for Boost this does not seem the case [1]. Would this be considered a bug in the boost recipe or is this simply not in scope of the Conan generators? I find it important not to tie my CMakeLists.txt too closely too Conan, and that would become impossible if needed to use different package discovery code for Conan and non-Conan.

Meanwhile, is there a reason the upstream CMake config files need to be removed? It seems for the "cmake" generator there's no option to do clean package discovery without them, and spdlog/fmt are the first packages (of CMake-based libraries) I encounter that do not provide those files.

[1] Conan generates Findboost.cmake, but CMake consumers typically require FindBoost.cmake (as per upstream)

what you mention is actually one of the biggest issues of conan right now (I also use conan with native find package, and conan for development), though it will be fixed those days.

Until now the cmake generators could only generate something like FindPackageName without beiing able to create multiple targets. in conan 1.24 this was addressed in conan itself, and with with 1.25 probably addressed for the cmake generator...

@gocarlos
Copy link
Contributor

see also #1227 #952 #419

@uilianries
Copy link
Member

@prince-chrismc
Copy link
Contributor

Are Find*.cmake files generated by Conan supposed to be consistent with the Config files provided by upstream projects?

We are tying our best, if you see any that do not match, open an issue please 💯

Some cases are still not possible as per carlos above

@uilianries
Copy link
Member

@neobrain Some special cases which are acceptable, like when there are macros in the cmake provided by the upstream. Otherwise, Conan generators should be enough.

@neobrain
Copy link
Contributor Author

Thanks all for the background information!

We are tying our best, if you see any that do not match, open an issue please 100

Some cases are still not possible as per carlos above

Is a solution for the problem carlos outlined in the works?

Honestly it doesn't look like the cmake_find_package generator is a feasible option for project authors currently. There's too many inconsistencies still around, and apparently most of the internal conanfiles in my project need to be revisited to ensure they can work with the new generator. Even if cmake_find_package is the right technical solution, it defeats the purpose of using a package manager if I'll need to constantly tweak my project's CMakeLists.txt as the Conan-provided targets get fixed up for consistency with upstream.

I'd strongly prefer if the CMake config files were just kept around until Conan's cmake_find_package generator is more mature. The FAQ doesn't seem to answer this, so is there a technical reason these files must be removed? While they don't work with non-CMake consumers, surely they don't hurt either (or are at least better than not being able to cleanly integrate the dependency at all)?

@conan-center-bot
Copy link
Collaborator

config.yml syntax error in build 2:

Only one library can be changed in the same PR: [fmt/all, spdlog/all]|

@prince-chrismc
Copy link
Contributor

Is a solution for the problem carlos outlined in the works?

📚 read this

is there a technical reason these files must be removed?

In order to support the vast number of generators Conan was envisioned for, there was a strong requirement to digest all of a project's metadata/settings/options/configurations in order to seamlessly transition from one to another depending on the consumers needs.

it defeats the purpose of using a package manager if I'll need to constantly tweak my project's CMakeLists.txt as the Conan-provided targets get fixed up for consistency with upstream.

This is not 100% the fault of CCI. For instance some projects actually change/improve their CMake to follow the best practices, like this one. Others are so old, they do not export a target! In these cases, as a consumer, I actually prefer the fact that Conan generator will "update" the project to something that is friendlier to incorporate. Let's not even start on absorbing Makefile projects that have not been ported to windows by the maintainers to your CMake project.

There are a few that may exist, but the vast majority are from the technical limitation noted before. And once fixed, they will not have to be changed... Until upstream breaks them 💥

@neobrain
Copy link
Contributor Author

neobrain commented May 6, 2020

Looks like this isn't going anywhere 😕

As outlined before, this issue makes recipes from this repository unusable in CMake projects that try to be well-behaving (i.e. not have Conan-workarounds all over the place). I understand this is coming from good intentions, but explicitly degrading the CMake usability so severely for the sake of purism seems misguided to me.

I'll look to get my recipes from another source for now and/or use the outdated bincrafters releases where that's an option, or migrate to another package manager (I heard vcpkg is getting versioning and manifest files now!).

@neobrain neobrain closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants