-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add open62541/1.1 #1856
Add open62541/1.1 #1856
Conversation
Some configurations of 'open62541/1.0.1' failed in build 1 (
|
I don't know of any official convention. What I did until now is assume What you can do is add a cpython build requirement if |
At Bincrafters we also have a recipe for this which might be helpful |
Some configurations of 'open62541/1.0.1' failed in build 2 (
|
Thanks @Croydon, I updated this PR with bincrafters recipe ! |
Having python in %PATH% is not exactly the same, because FindPython.cmake check in windows registry to get installed path. Anyway I don't know exactly why but I am currently in a state where I have a broken python installation on windows. I think this is interseting because it proves that this recipe can't be build without python. |
Some configurations of 'open62541/1.0.1' failed in build 3 (
|
Some configurations of 'open62541/1.0.1' failed in build 4 (
|
An unexpected error happened and has been reported. Help is on its way! 🏇 |
Some configurations of 'open62541/1.0.1' failed in build 6 (
|
Some configurations of 'open62541/1.0.1' failed in build 7 (
|
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Some configurations of 'open62541/1.0.1' failed in build 9 (
|
Some configurations of 'open62541/1.0.1' failed in build 10 (
|
@syoliver It seems the package is built with the address sanitizer enabled. |
An unexpected error happened and has been reported. Help is on its way! 🏇 |
1 similar comment
An unexpected error happened and has been reported. Help is on its way! 🏇 |
I think it is better to patch the sources to disable the sanitizer because this incurs a runtime overhead. |
If it is acceptable, I would prefer because there seems to be a bug with some version of the sanitizer... And I can't test locally... |
Some configurations of 'open62541/1.0.1' failed in build 13 (
|
All green in build 14 (
|
Some configurations of 'open62541/1.0.1' failed in build 15 (
|
Some configurations of 'open62541/1.1' failed in build 16 (
|
All green in build 17 (
|
@@ -0,0 +1,6 @@ | |||
submodules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a mechanism for downloading submodules in this recipe, would it make sense to add an option to tools.get or add a new function in conan for it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense when you use the scm feature of conan, which is forbidden on cci.
The aim of cci is to provide packages which can be consumed using conan alone.
So in this case, mdnsd should be conanized and added as a conan dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the general case it would make sense, but it points to specific version of mdnsd, no idea if it matches a stable version (according to mdnsd itself) at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice before, but it is even worse : it point to a fork of the original mdnsd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When in doubt: ask, or look what the linux distributions are doing
It looks like the fork has added cmake support.
Maybe the goal of the fork is only to provide cmake support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Differences to the base repo (https://github.com/troglobit/mdnsd):
- Use CMake for the build
- Strict compilation flags for better portability
- Support of Linux, MinGW, OS X and Windows
- Continuous Integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base repo is way more active.
And looking at recent commits, actual bugs are fixed.
It is possible to build using autotools with MSVC when using appropriate wrapper scripts.
Maybe the changes of the Pro fork can help what changes should be made.
If possible, they can be upstreamed so the autotools build system improves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK understood, but I find it better to do in two steps here. Have the opcua package and improve msdns after because I feel like there is much work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that is that your recipe will get merged only and only if it passes all hook checks.
And this means that adding recipes to cci is a bottom-up process.
What is that opcua package? It's the first mention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it passes the hooks checks now.
OPCUA is an industrial protocol. It is related to the IoT world. This library is the most popular open source implementation.
Co-authored-by: Uilian Ries <uilianries@gmail.com>
All green in build 19 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
I'd really like to see mdnsd
merged first and then build this package using the requirement. Is it too hard to build? Has anyone tried to open the PR from Pro/mdnsd
in order to add CMake support? Maybe the author will merge it right away.
OTH, I'm also worried about ua-nodeset
, it has been moved to a private repo. What's the reason for that movement? Will the repository OPCFoundation/UA-Nodeset
be there forever? And also, we should use a released version, not a commit.
or self.options.enable_pubsub_informationmodel_methods \ | ||
or self.options.enable_pubsub_custom_publish_handling \ | ||
or self.options.enable_pubsub_eth_uadp_etf: | ||
raise ConanInvalidConfiguration("The option 'enable_pubsub' must be True, if pubsub is required." ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only enter this block if if self.options.enable_pubsub
, so enable_pubsub
is already True.
"enable_pubsub_eth_uadp_xdp": [True, False], # Enable Subscribe UADP over Ethernet using XDP | ||
"enable_pubsub_eth_uadp_etf": [True, False], # Use ETF implementation for the ETH_UADP publish | ||
"enable_pubsub_mqtt": [True, False], # Enable publish/subscribe with mqtt (experimental) | ||
"namespace_zero": ["minimal", "reduced", "full"] # Completeness of the generated namespace zero (minimal/reduced/full) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see options grouped together:
- move
enable_pubsub_eth_uadp_xdp
,enable_pubsub_eth_uadp_etf
,enable_pubsub_mqtt
together with the others related toenable_pubsub...
enable_discovery_semaphore
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to group enable_pubsub_eth_uadp , enable_pubsub_eth_uadp_xdp, enable_pubsub_eth_uadp_etf
under one option as an enumeration, since, as I understand, these options are mutually exclusive
"multithread": "ANY", # Level of multithreading (0-99: No Multithreading, 100-199: Thread-Safe API, >=200: Internal Threads | ||
# multithread configuration may be | ||
# 1.0 : False, True | ||
# 1.1 : False, thread-safe, internal-threads or an integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that integer work? It is the same 10
or 30
? If it is, I would list here only the meaningful values... if every number has a different meaning... well, that's how it is 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extracted meaningful values here : False, thread-safe, internal-threads
But @madebr made a suggestion where he leaved the possibility to the user to set the integer if he prefers. I think it can make sense.
homepage = "http://open62541.org" | ||
license = "MPL-2.0" | ||
exports_sources = ["CMakeLists.txt", "patches/**"] | ||
exports = "submoduledata.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in the exports_sources
, it is only needed if we are going to build the package.
please be aware that there is a second PR #1687 in progress |
I will try, Yes I don't think it will be easy, but I have to try. I miss time to do it right now 🙄 |
cmake_minimum_required(VERSION 3.1.2) | ||
|
||
project(cmake_wrapper) | ||
message(WARNING "Conan Open62541 Wrapped CMake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message(WARNING "Conan Open62541 Wrapped CMake") |
message(WARNING "Conan Open62541 Wrapped CMake") | ||
|
||
include(conanbuildinfo.cmake) | ||
conan_basic_setup(NO_OUTPUT_DIRS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required?
@@ -0,0 +1,8 @@ | |||
sources: | |||
"1.1": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to bump the version to 1.1.2
?
|
||
def requirements(self): | ||
if self.options.tls == "openssl": | ||
self.requires("openssl/1.1.1g") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.requires("openssl/1.1.1g") | |
self.requires("openssl/1.1.1h") |
# multithread configuration may be | ||
# 1.0 : False, True | ||
# 1.1 : False, thread-safe, internal-threads or an integer | ||
"tls": [False, "openssl", "mbedtls-apache", "mbedtls-gpl"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"tls": [False, "openssl", "mbedtls-apache", "mbedtls-gpl"], | |
"tls": [False, "openssl", "mbedtls"], |
Eventually create a auto
option:
"tls": [False, "auto", "openssl", "mbedtls"],
...
See the cmake recipe how to use this:
https://github.com/conan-io/conan-center-index/blob/master/recipes/cmake/3.x.x/conanfile.py#L17
"shared": False, | ||
"fPIC": True, | ||
"multithread": False, | ||
"tls": "mbedtls-apache", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"tls": "mbedtls-apache", | |
"tls": "mbedtls", |
def configure(self): | ||
self.options["libwebsockets"].with_ssl = self.options.tls | ||
|
||
if self.options.enable_pubsub: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't override options of dependencies, but check in build
.
def configure(self): | |
self.options["libwebsockets"].with_ssl = self.options.tls | |
if self.options.enable_pubsub: | |
def configure(self): | |
if self.options.enable_pubsub: |
self.info.options.multithreaded = self._ua_multithreaded() | ||
|
||
def package_info(self): | ||
self.cpp_info.libs = tools.collect_libs(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In package
, you remove lib/cmake
and lib/pkgconfig
.
Please check + add the cmake_find_package/cmake_find_package_multi/pkg_config
names.
In the future, .pc
will files might be created only for those recipes that set pkg_config
explicitly. So that one should be set anyhow.
"1.1": | ||
deps/mdnsd: | ||
sha256: b1847047c0c4a20ab6a132fed6d6b6e50db6b07f0068a95213306603279a3b9a | ||
url: https://github.com/Pro/mdnsd/archive/5af316c4b79460c0c1b6c2a674a37e28a85441a3.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pro-mdnsd/0.8.4
is now available as a conan package!
Alas, nodeset not yet.
I detected other pull requests that are modifying open62541 recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
self._cmake.definitions["UA_DEBUG_DUMP_PKGS"] = self.options.debug_dump_pkgs | ||
self._cmake.definitions["UA_ENABLE_HARDENING"] = self.options.enable_hardening | ||
self._cmake.definitions["OPEN62541_VERSION"] = self.version | ||
self._cmake.definitions["UA_DEBUG" ] = self.settings.build_type == "Debug" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be set to Release? Are you running the internal test and thus need this flag (you shouldn't run unit tests on conan though...).
Also if I am not mixing things up, isn't this flag also changing optimization levels?
If you really want to allow the user to change from release to debug, set it through settings.build_type
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this option is set by CMakeLists.txt file itself when CMAKE_BUILD_TYPE
is Debug, so you can skip this option, since it is already handled by settings.build_type
"enable_pubsub_eth_uadp_xdp": [True, False], # Enable Subscribe UADP over Ethernet using XDP | ||
"enable_pubsub_eth_uadp_etf": [True, False], # Use ETF implementation for the ETH_UADP publish | ||
"enable_pubsub_mqtt": [True, False], # Enable publish/subscribe with mqtt (experimental) | ||
"namespace_zero": ["minimal", "reduced", "full"] # Completeness of the generated namespace zero (minimal/reduced/full) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to group enable_pubsub_eth_uadp , enable_pubsub_eth_uadp_xdp, enable_pubsub_eth_uadp_etf
under one option as an enumeration, since, as I understand, these options are mutually exclusive
self._cmake.definitions["UA_ENABLE_TYPEDESCRIPTION"] = self.options.enable_typedescription | ||
self._cmake.definitions["UA_NAMESPACE_ZERO"] = self.options.namespace_zero | ||
|
||
self._cmake.definitions["UA_BUILD_EXAMPLES"] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this option defaults to Off in CmakeLists.txt, no need to set it
self._cmake.definitions["UA_NAMESPACE_ZERO"] = self.options.namespace_zero | ||
|
||
self._cmake.definitions["UA_BUILD_EXAMPLES"] = False | ||
self._cmake.definitions["UA_BUILD_UNIT_TESTS"] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this option also defaults to Off in CmakeLists.txt, thus again, no need to set it
Looks quite good, pretty much everything that I tried to achieve with my pull request. If all code discussions are resolved I am willing to close my PR and accept this as the proper one. (Also there is documentation PR on open62541 that would need slight updates to cover all of the options that you provide, I would update it once the PR is merged ) One question thought, have you checked if include directories are properly set in the final package? I had some problems with those myself and had to explicitly set them in my package_info section of my conanfile.py Also it would be nice if you simplified the names of your options, in my opinion, they don't need to be exact representations of cmake option names. You can check my conanfile.py for some inspiration. |
Specify library name and version: open62541/1.1
conan-center hook activated.