init try to bring iceoryx into CCI #5541
Conversation
This comment has been minimized.
This comment has been minimized.
|
very very stupid question here : we are running in a docker container or sth like that right? so <sys/acl.h" not present? just asking to understand the error better. ^^' |
yes, it's docker container |
|
Some more question related to the error "missing <sys/acl.h> here :
|
Yes. But this PR will be blocked, we usually don't release partial packages. We will merge acl first, then unblock iceoryx.
It would be great!
I didn't check your package, but the thing is, if you need a requirement only when building, like headers or executable tools, it's a build requirement, which is much better in terms of system package (you don't want to see apt or whatever running when installing a simple Conan package).
Yes. CCI doesn't do it because only native build (x64) generates more 100 packages per build. Now imagining adding cross building support or other archs ... resources, cloud, time, ...
No, thank you for contributing! |
|
About your test package files, reduce to a simple and small file. The idea about test package is, of course, testing the package, not running an entire unit test from the project. We just need to validate linkage, if the library and headers are there. Some extra info about test package here. |
recipes/iceoryx/all/conanfile.py
Outdated
| self._patch_sources() | ||
|
|
||
| self.cmake = CMake(self) | ||
| self.cmake.definitions["BUILD_SHARED_LIBS"] = self.options.shared |
There was a problem hiding this comment.
| self.cmake.definitions["BUILD_SHARED_LIBS"] = self.options.shared |
CMake helper does it, it's magic: https://docs.conan.io/en/latest/reference/build_helpers/cmake.html?highlight=build_shared_libs#definitions
|
@uilianries thanks for the information here. i was looking a little bit deeper into the problem. Yes you absolutely right. The "libacl" is absolute nothing belong to the system. my bad here. It's a lib like any other and for that reason i would suggest i bring the "libacl" first into CCI. Benefit would be that iceoryx can target systems that doesnt have acl in their sysroot. also nice. i would not stop the PR but simple let it open until acl is part of the CCI. thanks again @uilianries for your support and explaination here. |
This comment has been minimized.
This comment has been minimized.
| project(examples) | ||
| include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake) | ||
| conan_basic_setup() | ||
|
|
There was a problem hiding this comment.
Regarding this comment: #5541 (comment)
If we only want to test linkage then only these examples are sufficient:
- icedelivery
- icedelivery_in_c
- singleprocess for roudi daemon build
All other example code can be removed.
recipes/iceoryx/all/conanfile.py
Outdated
| class IceoryxConan(ConanFile): | ||
|
|
||
| name = "iceoryx" | ||
| license = " Apache-2.0" |
|
A small question before i do the next push ^^' i noticed that the project was just able to build when the libcxx == libstdc++11 for gcc6, gcc7, gcc8 on linux. it effected not just linking but also the compilation. I saw that other recipes like also dealing with such things by raising invalid configuration.My question now - is this "allowed practise" in Conan Center or is there an alternative which is prefered? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@SSE4 @uilianries is there a way to try out MAC OS build local? i have a VM with Linux, a Windows OS and so validate local Linux and Windows - but how about MAC OS? is there a Container image? or some other trick? |
There was a problem hiding this comment.
Hi, @dotChris90 . I'm not sure if you have something pending before reviewing this recipe. It's not clear to me from your last comments.
So just very high-level comments:
- It's not needed to make it work for all the configurations. Sometimes it is better to merge a first working recipe and then improve it and make it work for other architectures.
- On the
test_packageI want to ask if all the files are needed/used. At least I see oneREADME.mdthat can be removed.
|
@jgsogo thanks for asking - i was hoping to make the recipe test_packages runnable inside the container but seems not easy because shared memory reason. but maybe you are right - my last local trials didnt show success. plus i was trying different ways to make recipse running for libstdc++ and not just libstdc++11 - but until now no succes. Maybe i simple "open" the review and do improvements in the next PRs. .... |
|
the other question : i would like to test linkage of multiple artifacts to be sure i met also the "systemlibs" and requirements between the artifacts. The Readme - yes agree - can be removed. :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
@dotChris90 Thank you for your contribution, please, take a look on my review.
recipes/iceoryx/all/conanfile.py
Outdated
| class IceoryxConan(ConanFile): | ||
|
|
||
| name = "iceoryx" | ||
| version = "1.0.0" |
There was a problem hiding this comment.
| version = "1.0.0" |
Keep the recipe generic. The user and CI should pass the version according the conandata.yml
| "toml_config": [True, False] | ||
| } | ||
| default_options = { | ||
| "shared": False, |
There was a problem hiding this comment.
| "shared": False, | |
| "shared": False, |
No fPIC as option?
There was a problem hiding this comment.
that is a good point - if i remember well i raised an issue on iceoryx and discussed with them. but seems they already set this in their CMAKE-Files via POSITION_INDEPENDENT_CODE - and I was not 100% sure if it is a "must" for CCI - since the hookups didnt show red lines ^^' so my basic questions here is : is it a must if the library itself / the project already set everything in their CMakefiles? That is now sth i will follow Conan community wishes.
There was a problem hiding this comment.
fPIC is only a must when the library is supposed to be used in a shared library or really required by some platform.
It looks like POSITION_INDEPENDENT_CODE is used all over the code: https://github.com/eclipse-iceoryx/iceoryx/search?p=1&q=POSITION_INDEPENDENT_CODE
It is possible to patch it out, but I don't think it's worth the effort.
There was a problem hiding this comment.
Usually we do
conan-center-index/recipes/glbinding/all/conanfile.py
Lines 42 to 44 in 41d4fbb
There was a problem hiding this comment.
@prince-chrismc i will give it a try - i never saw this so far - but cool it exist.
recipes/iceoryx/all/conanfile.py
Outdated
| if self.options.toml_config: | ||
| tools.rename( | ||
| os.path.join(self._pkg_etc, "roudi_config_example.toml"), | ||
| os.path.join(self._pkg_bin, "roudi_config_example.toml") |
There was a problem hiding this comment.
| os.path.join(self._pkg_bin, "roudi_config_example.toml") | |
| os.path.join(self._pkg_bin, "roudi_config_example.toml") |
Is it really needed? The file name is example. If is really necessary, please, move to self.package_folder/res, which is the used for configuration files in the package.
There was a problem hiding this comment.
hmmm actually again a good question - i would like to keep it and will move to res folder because : i think iceoryx is mostly used for embedded area and the component "roudi" is a executable. It could be deployed with the "deploy" generator of conan. (Actually we also do this in projects - the -g "deploy" functionality is pretty cool). and here when deploying a default configuration file would be cool. - i think i simple rename it to roudi_config.toml.
| @@ -0,0 +1,318 @@ | |||
| # icedelivery | |||
There was a problem hiding this comment.
Let's remove the readme files
| @@ -0,0 +1,168 @@ | |||
| # icedelivery in C | |||
There was a problem hiding this comment.
Let's remove the readme files
|
All green in build 11 (
|
| def _pkg_cmake(self): | ||
| return os.path.join( | ||
| self.package_folder, | ||
| "lib/cmake" |
There was a problem hiding this comment.
| "lib/cmake" | |
| "lib", "cmake" |
| self.cpp_info.names["cmake_find_package"] = "iceoryx" | ||
| self.cpp_info.names["cmake_find_multi_package"] = "iceoryx" | ||
| # platform component | ||
| self.cpp_info.components["platform"].name = "platform" |
There was a problem hiding this comment.
The components are a little on the generic side
Perhaps something like?
| self.cpp_info.components["platform"].name = "platform" | |
| self.cpp_info.components["iceoryx_platform"].name = "platform" |
There was a problem hiding this comment.
yes, it would be nice to fix it, and not use global name for all generators.
| self.cpp_info.components["utils"].build_modules["cmake_find_package"] = [ | ||
| os.path.join(self._module_subfolder, "conan-official-iceoryx_utils-targets.cmake") | ||
| ] |
There was a problem hiding this comment.
Could we not follow the usual template with one file per project?
It's a lot less code bloat
| iox-cpp-publisher iox-cpp-publisher-untyped | ||
| PROPERTIES | ||
| CXX_STANDARD_REQUIRED ON | ||
| POSITION_INDEPENDENT_CODE ON |
There was a problem hiding this comment.
Not required for executables AFAIK
| POSITION_INDEPENDENT_CODE ON |
| def validate(self): | ||
| os = self.settings.os | ||
| compiler = self.settings.compiler | ||
| version = tools.Version(self.settings.compiler.version) |
There was a problem hiding this comment.
For my own tracking purposes conan-io/conan#8002
| add_executable(iox-cpp-publisher ./iox_publisher.cpp) | ||
| target_link_libraries(iox-cpp-publisher | ||
| iceoryx_posh::iceoryx_posh | ||
| ) | ||
|
|
||
| add_executable(iox-cpp-publisher-untyped ./iox_publisher_untyped.cpp) | ||
| target_link_libraries(iox-cpp-publisher-untyped | ||
| iceoryx_posh::iceoryx_posh | ||
| ) |
There was a problem hiding this comment.
Are both required? they seem to be testing the same part of the library... I am undering if the test_package could be trimmed a little bit?
There was a problem hiding this comment.
@prince-chrismc i will "shrink" them again - but probably you are right. the main thing is the C++ target can be found and the C target can be found
|
@prince-chrismc i will bring your suggestions into the code and let it rebuild. thanks for the comments. |
|
@prince-chrismc - i will pay back my depth in a separate PR .... |
I honestly would have been less picky if I saw it was so close to being merged... any future PRs are always welcomed! |
* init try to bring iceoryx into CCI * according to FAQs * add cmakewrapper and remove patch * add dependency acl * reduced tests for linkage * adding more validation * included atomic and force libstdc++11 on clang too * acl just required by linux * allow windows and force C++14 in test * rt lib just for linux * just support Visual Studio compiler 16 and higher * corrected smalles changes. config now in res folder
Specify library name and version: iceoryx/1.0.0
Iceoryx https://iceoryx.io/v1.0.0/ is a new, modern and speeding fast way to do IPC on embedded devices - several other projects currently use it as their dependency like cyclone dds https://projects.eclipse.org/proposals/eclipse-cyclone-dds and eCAL https://continental.github.io/ecal/ - currently by cloning .... i would like to change this. Maybe also new projects will follow when bring them to CCI.
conan-center hook activated.