ipaddress: add recipe #23206
ipaddress: add recipe #23206
Conversation
This comment has been minimized.
This comment has been minimized.
|
Feel free to ping me once this is ready for review :) The recipe looks good as-is tho! |
|
Hi! Thanks for the comment. I think PR is ready for review, I’ll change it to ready |
This comment has been minimized.
This comment has been minimized.
recipes/ipaddress/all/conanfile.py
Outdated
| options = {"exceptions": [True, False], "overload_std": [True, False], "ipv6_scope": [True, False], "ipv6_scope_max_length": ["ANY"]} | ||
| default_options = {"exceptions": True, "overload_std": True, "ipv6_scope": True, "ipv6_scope_max_length": 16} |
There was a problem hiding this comment.
| options = {"exceptions": [True, False], "overload_std": [True, False], "ipv6_scope": [True, False], "ipv6_scope_max_length": ["ANY"]} | |
| default_options = {"exceptions": True, "overload_std": True, "ipv6_scope": True, "ipv6_scope_max_length": 16} | |
| options = { | |
| "exceptions": [True, False], | |
| "overload_std": [True, False], | |
| "ipv6_scope": [True, False], | |
| "ipv6_scope_max_length": ["ANY"], | |
| } | |
| default_options = { | |
| "exceptions": True, | |
| "overload_std": True, | |
| "ipv6_scope": True, | |
| "ipv6_scope_max_length": 16, | |
| } |
For readability.
recipes/ipaddress/all/conanfile.py
Outdated
| if compiler == "gcc" and compiler_version < "7.5": | ||
| raise ConanInvalidConfiguration(f"{self.ref} doesn't support gcc < 7.5") | ||
| if compiler == "clang" and compiler_version < "6.0": | ||
| raise ConanInvalidConfiguration(f"{self.ref} doesn't support clang < 6.0") | ||
| if compiler == "apple-clang" and compiler_version < "13.0": | ||
| raise ConanInvalidConfiguration(f"{self.ref} doesn't support Apple clang < 13.0") | ||
| if is_msvc(self) and compiler_version < "16": | ||
| raise ConanInvalidConfiguration(f"{self.ref} doesn't support MSVC < 16") |
There was a problem hiding this comment.
Not wrong, but it would be better to follow the conventional format:
https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/header_only/all/conanfile.py#L30-L75
There was a problem hiding this comment.
Yes, it will probably be better. I'll do it as in the template. But let me change the error text to more accurately convey the cause of the error.
If the new text is unacceptable, please respond to me. I will return the text from template.
|
@valgur Thank you! Yes, these will be really good changes. Thanks for the feedback! Thank you everyone for your help and work on my PR! Let me know if any other changes are needed |
This comment has been minimized.
This comment has been minimized.
|
|
||
| add_executable(${PROJECT_NAME} test_package.cpp) | ||
| target_link_libraries(${PROJECT_NAME} ipaddress::ipaddress) | ||
| target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14) |
There was a problem hiding this comment.
This is setting C++ 14, but the recipe indicates min to be C++ 11 (And the readme does too)
| target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14) | |
| target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_11) |
To ensure we work for the min version :)
Unless there's a reason this is set to 14, any insight would be appreciated in that case!
There was a problem hiding this comment.
Hi, Rubén! I almost understand what needs to be done, but I want to clarify just in case:
target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_11)does not configure compilation with C++11. It only checks that the compiler supports C++11 and higher. In reality, the compiling will be with the default version (usually 14 or 17).
In order to configure compilation with standard C++11, need to do the following:
set_target_properties(${PROJECT_NAME}
PROPERTIES
CXX_STANDARD 11
)or
set_target_properties(${PROJECT_NAME}
PROPERTIES
CXX_STANDARD 11
CXX_STANDARD_REQUIRED ON
CXX_EXTENSIONS OFF
)In this case, cmake will pass the necessary flag during compilation (-std=gnu++11 or -std=c++11 etc).
Do I need to set a minimum standard or force the test to compile with C++11? In other words, should I use target_compile_features or set_target_properties?
There was a problem hiding this comment.
@VladimirShaleev Thank you for your feedback. The reason behind is because in Conan 1.x , the settings.compiler.cppstd is not configured by default, so the test package will use what is configured by the compiler as standard version (e.g. GCC5 will fail). In Conan 2.x, the c++ standard always follows what is configured by settings.compiler.cppstd, so it would not be needed to use target_compile_features neither set_target_properties in that case (future, when dropping Conan 1.x). But answering your question: target_compile_features is more than fine because it will check if the compiler supports c++11 and accommodate in case needed.
There was a problem hiding this comment.
@uilianries Thanks for your reply! I'll follow your advice and save target_compile_features with cxx_std_11
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 5 (
Conan v2 pipeline ✔️
All green in build 5 (
|
|
Thank you, Rubén, for your insightful feedbacks during the pull request review) I really appreciate it! |
* ipaddress: add recipe * Improved readability and template compliance from package_templates in conanfile.py * Compiling a test package using C++11 * Target compile features cxx_std_11
Specify library name and version: ipaddress/1.0.1
Hi! This is a new library for working with IP addresses and networks.
It is inspired by the well-thought-out API from the Python module for working with addresses and networks. Therefore, it is familiar and understandable for use by many programmers.
This library fills gaps in the implementation of other libraries: network parsing, working with stringize scope id, as well as network manipulation