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

Wrong example in readme #338

Closed
vdsbenoit opened this issue May 4, 2021 · 2 comments · Fixed by #339
Closed

Wrong example in readme #338

vdsbenoit opened this issue May 4, 2021 · 2 comments · Fixed by #339
Assignees
Milestone

Comments

@vdsbenoit
Copy link

Error in the readme

In the release 0.16 and current develop readme, the example in the conan_cmake_install description is still the example of the previous conan_cmake_run command.

cmake generator

I take the opportunity of this ticket to provide some feedback on the recent update for the documentation. I have to admit I am confused with the examples of the new API (with conan_cmake_install) because the cmake_find_package generator is used. I think it is not consistant with other examples that use the casual cmake generator.

Due to this, when I tried to update my code to the new API, it took me a while to understand that the conan_cmake_install macro does not accept any BASIC_SETUP nor CMAKE_TARGETS arguments anymore and that we should call conan_basic_setup(TARGETS) instead. I found it out when I read this issue. I think it would be worth it to document this change.

That said, thank you for the great work you have provided here, it is much appreciated 🙏

@czoido
Copy link
Contributor

czoido commented May 4, 2021

Hi @vdsbenoit,
Thanks for reporting this issue and your feedback. Also, thanks a lot for your kind words.
We'll fix the conan_cmake_install asap and also make it clearer the switch to find_package generator in the documentation. The reason for the update to the find_package generator is that it's less intrusive and more aligned with the direction we want Conan to take for 2.0. I'll try to clarify the change in the docs.

@vdsbenoit
Copy link
Author

Hi @czoido,

Thank you for updating the documentation. I had a look at your PR and it looks good to me.

May I also suggest to split the documentation in two different pages ? One for the 3 new macros (conan_cmake_install etc) and another for the previous macro (conan_cmake_run). While reading the current readme, I felt like jumping from an implementation to another which lead to a lot of confusion. Another colleague just shared the same feeling with me today so I thought it was worth sharing it with you.

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