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

cli: new recipe #22673

Merged
merged 17 commits into from
Feb 19, 2024
Merged

cli: new recipe #22673

merged 17 commits into from
Feb 19, 2024

Conversation

rob-baily
Copy link
Contributor

Specify library name and version: cli/2.1.0

Addresses #22564

I am not the author of the library but am a fan of it and want to help it be available to more folks. There is some discussion at daniele77/cli#113 if you are interested to follow.


@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@rob-baily rob-baily marked this pull request as ready for review February 5, 2024 19:45
@rob-baily
Copy link
Contributor Author

NOTES:

  • Do not have any experience with Apple Clang so after some failures when I changed it to require version 14 and nothing was run in CI for this compiler. Let me know if I should update this to just not allow any Apple Clang builds.
  • I did not try the configurations locally with the hooks as the documentation says it is not supported yet for Conan 2 and that is the version I am using. The CI looks fine at this point.

@AbrilRBS
Copy link
Member

AbrilRBS commented Feb 6, 2024

Hi @rob-baily, thanks a lot for your contribution! We appreciate it, as I'm sure upstream does :)

Regarding your comment:

Do not have any experience with Apple Clang so after some failures when I changed it to require version 14 and nothing was run in CI for this compiler. Let me know if I should update this to just not allow any Apple Clang builds.
I did not try the configurations locally with the hooks as the documentation says it is not supported yet for Conan 2 and that is the version I am using. The CI looks fine at this point.

No worries at all, it's more important to clearly communicate what was and wasn't tested than being able to test every possible configuration - that's what the CI is ultimately for.
We are always happy to remove support for certain problematic configurations in the validate method and leave it for future PRs to deal with, but let me first check if I can help with apple-clang on my end :)

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution! Some comments, but the recipe looks great overall :)

"clang": "6",
"Visual Studio": "16",
"msvc": "192",
"apple-clang": "14",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick with this. It only affects Conan v1 which are on our way to deprecate from CCI, so I don't want to spend much energy on it :)

Comment on lines 52 to 53
deps = CMakeDeps(self)
deps.generate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the recipe does not declare any requirements, this can be safely skipped.

Nevertheless, I see in the upstream CMakeLists that it has some options for using asio that were skipped here. Any insight into why you decided to omit them? I don't oppose it if it makes contributing the recipe easier, it can be left as an exercise for the next PR if someone needs it, but happy to hear your thoughts :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a header only library it is left to the user of the library if they want to include the CLI_UseBoostAsio flag when compiling their project. If we added it as an option here then we would need to ensure this flag was used in any consuming code for the option to make sense. I am not familiar with how that would work but if you have an example we can give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RubenRBS Let me know if you think this should be changed or if you have any comments based on my comment. I believe everything else is addressed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @rob-baily Looking into https://github.com/daniele77/cli/blob/master/CMakeLists.txt#L70 it seems like you have both Boost and asio as requirement. I'll send you a PR with a suggestion to include them as requirements.

Comment on lines 74 to 76
self.cpp_info.set_property("cmake_file_name", "CLI")
self.cpp_info.set_property("cmake_target_name", "CLI::CLI")
self.cpp_info.set_property("pkg_config_name", "CLI")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the rename here? I see that upstream examples use lowercase for their find_package, but maybe I'm missing something!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following what CLI11 did originally and did not pay enough attention to the examples in the main repo. I have changed it to lower case.

recipes/cli/all/conanfile.py Outdated Show resolved Hide resolved
recipes/cli/all/conanfile.py Outdated Show resolved Hide resolved
recipes/cli/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Rubén Rincón Blanco <[email protected]>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

Hi @rob-baily Please take a look in https://github.com/Xcelerator-Group/conan-center-index/pull/1. That PR adds support to asio.

@RubenRBS This project is a bit odd. Usually, header-only projects use #ifdef to include or not something, or even cmake definitions to install or not some headers. It's not the case here, no defines neither separated headers (as far as I found). So, you need to know what you are including as header when using CLI. The CMakeLists.txt from that project is only a helper to check if Boost or Asio are installed and make them a public to your CMake (this recipe could copy *.hpp instead of running cmake). So even when we change the option value, and cmake definition for asio, the package content is the same, only dependencies change here.

@conan-center-bot

This comment has been minimized.

@rob-baily
Copy link
Contributor Author

Hi @rob-baily Please take a look in Xcelerator-Group#1. That PR adds support to asio.

@RubenRBS This project is a bit odd. Usually, header-only projects use #ifdef to include or not something, or even cmake definitions to install or not some headers. It's not the case here, no defines neither separated headers (as far as I found). So, you need to know what you are including as header when using CLI. The CMakeLists.txt from that project is only a helper to check if Boost or Asio are installed and make them a public to your CMake (this recipe could copy *.hpp instead of running cmake). So even when we change the option value, and cmake definition for asio, the package content is the same, only dependencies change here.

@uilianries I merged your suggestions into the PR as they seem fine. With that I was trying to see if it was possible to have a way to test the different with_asio options in the test_package. Is that supported and if so is there an example to follow? I've been looking today and haven't seen anything that helps with this.

I was also trying to make the preprocessor_definitions dynamic for the test package based on the cli options but I could not get it to work. If I run this conan create -o cli/*:with_asio=boost -o boost/*:header_only=True all/conanfile.py --version 2.1.0 I still can't seem to get access to the option in the test package conanfile with something like self.options["cli"].with_asio

@uilianries
Copy link
Member

@rob-baily
Copy link
Contributor Author

@rob-baily You can use https://github.com/conan-io/conan-center-index/blob/master/recipes/approvaltests.cpp/all/test_package/conanfile.py as reference.

@uilianries Awesome, thanks! I pushed a new commit with a copy of https://github.com/daniele77/cli/blob/master/examples/complete.cpp that is used in the main repo as an example of how to compile withe options. Now I can run conan create all/conanfile.py --version 2.1.0 -o cli/*:with_asio=boost -o boost/*:header_only=True or conan create all/conanfile.py --version 2.1.0 -o cli/*:with_asio=standalone and the right processing happens. I think this is ready to go now but let me know if you think otherwise.

On a semi-related note I see that Conan 1 has information about dependencies here https://docs.conan.io/1/reference/conanfile/dependencies.html but I could not find anything similar in Conan 2 docs. If you point me I could possibly add some info or I can just raise an issue if you think someone else should update.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

On a semi-related note I see that Conan 1 has information about dependencies here docs.conan.io/1/reference/conanfile/dependencies.html but I could not find anything similar in Conan 2 docs. If you point me I could possibly add some info or I can just raise an issue if you think someone else should update.

https://docs.conan.io/2/reference/conanfile/methods/requirements.html

uilianries
uilianries previously approved these changes Feb 12, 2024
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@uilianries uilianries Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be reduced, but I'm fine.

Copy link
Member

@uilianries uilianries Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be reduced, but I'm fine.

@daniele77
Copy link

daniele77 commented Feb 12, 2024

Hi @rob-baily Please take a look in Xcelerator-Group#1. That PR adds support to asio.

@RubenRBS This project is a bit odd. Usually, header-only projects use #ifdef to include or not something, or even cmake definitions to install or not some headers. It's not the case here, no defines neither separated headers (as far as I found). So, you need to know what you are including as header when using CLI. The CMakeLists.txt from that project is only a helper to check if Boost or Asio are installed and make them a public to your CMake (this recipe could copy *.hpp instead of running cmake). So even when we change the option value, and cmake definition for asio, the package content is the same, only dependencies change here.

As the author of the library, I'd like to clarify a few points regarding its usage and dependencies.

The cli library offers flexibility in its usage, accommodating various scenarios such as integration with boost-asio, standalone-asio, or functioning without any external library, depending on the required features. This is explained in detail in the "Dependencies" section of the README.md:

The library has no dependencies if you don't need remote sessions.

The library depends on asio (either the standalone version or the boost version)
only to provide telnet server (i.e., remote sessions).

To integrate the library into your application, simply instantiate the appropriate scheduler and include the corresponding header file. We provide options for boost-asio, standalone asio, and versions with no dependencies. Therefore, there's no need to define any macros in your code or CMake configuration.

The options CLI_UseBoostAsio and CLI_UseStandaloneAsio in the CMake configuration are specifically used for compiling the examples and unit tests bundled with the library. They serve no other purpose.

In summary, determining the dependencies of the cli library can be somewhat nuanced, as it largely depends on the requirements of the client code. It can function with no dependencies or may require either boost-asio or standalone-asio, depending on the specific needs of the application incorporating it.

I hope this clarifies any confusion regarding the library's usage and dependencies.

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecking after @daniele77 comments, thanks a lot for your insights :)

@rob-baily
Copy link
Contributor Author

@RubenRBS I see you requested changes but don't know what you are asking for. @uilianries requested the change to add Boost and standalone Asio as requirements based on configurations. That seemed reasonable to me as it should make it easier for users of the library to get the right dependencies if they plan to use the functions that require them. I don't think this goes against what @daniele77 said. Can you please clarify what changes you are requesting?

@AbrilRBS
Copy link
Member

Hi @rob-baily sorry for the confussion, I just requested changes so the bot would not auto merge until we have a chance to review it again :)

@uilianries
Copy link
Member

@daniele77 Thank you so much for giving us a detailed explanation. So we should be good by distributing the package without these dependencies, and when someone wants boost, for instance, will need to add as extra requirement. It's not first case using optional backend CCI, they are just not common here.

@rob-baily I'll send you a PR to simplify everything. Sorry the delay.

@uilianries
Copy link
Member

@rob-baily I created a second PR https://github.com/Xcelerator-Group/conan-center-index/pull/2 which reduces this PR by a lot, almost the same thing that you committed at first try.

/cc @RubenRBS

Signed-off-by: Uilian Ries <[email protected]>
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 14 (3a7956f6dcbceb93b896c55fed7cb1639dd86d04):

  • cli/2.1.0:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 14 (3a7956f6dcbceb93b896c55fed7cb1639dd86d04):

  • cli/2.1.0:
    All packages built successfully! (All logs)

@rob-baily
Copy link
Contributor Author

@rob-baily I created a second PR Xcelerator-Group#2 which reduces this PR by a lot, almost the same thing that you committed at first try.

/cc @RubenRBS

I merged it in and the pipeline checks look clean.

@conan-center-bot conan-center-bot merged commit f4213db into conan-io:master Feb 19, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants