-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0 #4096
Conversation
Failure in build 1 (
|
Failure in build 2 (
|
Failure in build 3 (
|
Failure in build 4 (
|
Failure in build 5 (
|
All green in build 6 (
|
-if(PAHO_BUILD_STATIC) | ||
- set(_PAHO_MQTT_C_LIB_NAME ${_PAHO_MQTT_C_LIB_NAME}-static) | ||
+if(WIN32) | ||
+ if(PAHO_BUILD_STATIC) | ||
+ set(_PAHO_MQTT_C_LIB_NAME ${_PAHO_MQTT_C_LIB_NAME}-static) | ||
+ endif() |
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 does not match
conan-center-index/recipes/paho-mqtt-c/all/conanfile.py
Lines 123 to 124 in 85f47df
if not self.options.shared: | |
target += "-static" |
can you explain why you added this patch?
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.
1.3.8 paho-mqtt-c build is removing -static from the library name for non WIN32 builds.
Example 1.3.8
This patch is to be able to build older versions of paho-mqtt-cpp with version 1.3.8 of paho-mqtt-c and to make sure the library name is correct for non WIN32 platforms.
paho-mqtt-cpp FindPahoMqtt
The target concat with -static should still 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.
#3903 leave's this in a grey zone....
I could not image there's an expectation from consumer that older -cpp
s will work with newer -c
s
Instead of a patch I would suggestion having a conditional requirements... and adding a validate()
method to make sure the overrides are correct
Does not sound reasonable to you?
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 build for paho-mqtt-cpp was broken for non x86_64 platforms. The patch will also fix building on other architectures. I am a bit new to conan so I don't know the correct path forward to fix non x86_64 broken builds with older -cpp consumers.
Instead of a patch I would suggestion having a conditional requirements... and adding a validate() method to make sure the overrides are correct
Do you have an example where this has been done?
I also don't know how long the non x86_64 builds have been broken, as I was unable to build anything in the 1.3 series of paho-mqtt-c before 1.3.8 with paho-mqtt-cpp consuming it with the following exception with a specific recipe; paho-mqtt-c/1.3.5@#acc9376a7232c5a50ac898a9131c6c82, which caused conan to want to run with --update and I am wanting to pin versions.
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 patching this be allowed under #3903 for the following:
They allow libraries to build in other configurations.
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 am not very familiar with these libraries, how is the -static suffix impacting non x86_64?
Version 1.2.0 fixed the static suffix for non WIN32 builds shown here 1.2.0 fix -static suffix. The patches I added were to fix this for older versions. Non x86 builds do not have a downloadable binary from the conan index repo and have to be built from source. The static suffix for older versions was expected to be there but wasn't on non WIN32 builds so building from source was failing. See #3760
Creating a validate() method and fixing up requirements might be fairly straight forward, but a large majority of static build configuration not having a pre-built paho-mqtt-c library available on conan center would trigger ConanInvalidConfiguration.
the paho-mqtt-c exported CMake targets are different from the one's which poha-mqtt-cpp is trying to consume
Does the namespace handle this? https://github.com/eclipse/paho.mqtt.c/blame/e047e25d34d53b4b265649144a3cac3b01eee76c/src/CMakeLists.txt#L308
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.
You should delete the file you are trying to patch. And change the call to find_package
to use the official one you pointed to
Why?
Conan works replicated the standard files from build systems, for CMake using theses generators
generators = "cmake", "cmake_find_package" |
Will create the correct Find<Package>.cmake
files
self.cpp_info.names["cmake_find_package"] = "eclipse-paho-mqtt-c" |
➡️ But I do not like this either
The Problem
paho-mqtt-cpp
does not use official/correct cmake generated files from paho-mqtt-c
Does the namespace handle this?
No, the -cpp
project is completely skipping the -c
files generation... the owner explained his reasoning .... sadly best practices have changed
Solution
- Fix
paho-mqtt-c
https://github.com/eclipse/paho.mqtt.c/blob/v1.3.8/src/CMakeLists.txt#L154
does not match the Code in this repository as I pointed our here #4096 (comment)
- Fix
paho-mqtt-cpp
self.version < 1.2.0
-c
MUST be less then 1.3.8
else-
= 1.3.8
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.
paho-mqtt-cpp does not use official/correct cmake generated files from paho-mqtt-c
No, the -cpp project is completely skipping the -c files generation... the owner explained his reasoning .... sadly best practices have changed
Being a conan build newb, I am trying to understand how to know if the -cpp uses the correct -c cmake files. Is the following correct?
The -c package_info is being called by the -cpp build and the cmake Find file is being placed in the -cpp build directory. The -cpp build then uses those generated cmake find files to locate the eclipse-paho-mqtt-c library files.
Is it the generators in the -cpp recipe doing this? - generators = "cmake", "cmake_find_package"
I can get it to build with generators = "cmake", and this doesn't generate the Findeclipse-paho-mqtt-c.cmake file. Is there more going on?
My changes on the -cpp conanfile.py currently looks like this:
diff --git a/recipes/paho-mqtt-cpp/all/conanfile.py b/recipes/paho-mqtt-cpp/all/conanfile.py
index 7fe996cb..63a7c2a9 100644
--- a/recipes/paho-mqtt-cpp/all/conanfile.py
+++ b/recipes/paho-mqtt-cpp/all/conanfile.py
@@ -1,6 +1,7 @@
import os
from conans import CMake, ConanFile, tools
from conans.errors import ConanInvalidConfiguration
+from conans.tools import Version
class PahoMqttCppConan(ConanFile):
@@ -11,7 +12,7 @@ class PahoMqttCppConan(ConanFile):
license = "EPL-1.0"
description = "The open-source client implementations of MQTT and MQTT-SN"
exports_sources = ["CMakeLists.txt", "patches/*"]
- generators = "cmake", "cmake_find_package"
+ generators = "cmake"
settings = "os", "arch", "compiler", "build_type"
options = {"shared": [True, False],
"fPIC": [True, False],
@@ -46,8 +47,11 @@ class PahoMqttCppConan(ConanFile):
def requirements(self):
- self.requires("paho-mqtt-c/1.3.8")
-
+ if Version(self.version) < "1.2.0":
+ self.requires("paho-mqtt-c/1.3.6")
+ else:
+ self.requires("paho-mqtt-c/1.3.8")
+
def source(self):
tools.get(**self.conan_data["sources"][self.version])
extracted_dir = self.name.replace("-", ".") + "-" + self.version
@@ -66,6 +70,8 @@ class PahoMqttCppConan(ConanFile):
return self._cmake
def build(self):
+ if Version(self.version) >= "1.2.0" and Version(self.deps_cpp_info["paho-mqtt-c"].version) < "1.3.8":
+ raise ConanInvalidConfiguration("The project {}/{} requires paho-mqtt-c >= 1.3.8".format(self.name, self.version))
for patch in self.conan_data["patches"][self.version]:
tools.patch(**patch)
cmake = self._configure_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.
First I am sorry for the delay, this is a complicated issue, and the solution is not trivial, just needed to have some free time to look at it.
Being a conan build newb
Welcome to the club 🤣 💟
The -c package_info is being called by the -cpp build and the cmake Find file is being placed in the -cpp build directory.
Yes.
The -cpp build then uses those generated cmake find files to locate the eclipse-paho-mqtt-c library files.
I am not 100% sure that is the case 🙊 I think it's using the file included in the -cpp project not the conan ones
You're comments seem to confirm I suspicion!
Is it the generators in the -cpp recipe doing this? - generators = "cmake", "cmake_find_package"
Yes.
I can get it to build with generators = "cmake", and this doesn't generate the Findeclipse-paho-mqtt-c.cmake file. Is there more going on?
I am afraid so...
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.
Changed by eclipse-paho/paho.mqtt.c@f875768
and eclipse-paho/paho.mqtt.c@c116b72
I think I found a solution....
Hi guys, any idea when I could start using this version please? paho-mqtt-cpp/1.2.0 I still get: |
@martinpeniak It needs to be merged first =D this is very new and it takes ~3-5 days for PRs to go through the review process... some weeks are slower than others You can checkout the fork and build the recipe yourself locally |
Thanks @prince-chrismc, very much appreciated! For now I am using the workaround from #3760 |
Just so I can find it later conan-io/conan#8053 (review) |
Hi guys, when do you think I will be able to use the version 1.2 please? Thanks! |
@bowb please consider https://github.com/bowb/conan-center-index/pull/1 |
bind upstream version requirements on deps
All green in build 7 (
|
Hi @prince-chrismc, by accident I deleted my local conan data so I needed to rebuild the 1.2.0 locally but now I am getting: (conan) examples/example_subscriber I've checked out this branch: https://github.com/bowb/conan-center-index/tree/paho-mqtt-cpp-1.2.1 I've seen this error before when I was trying to use a different version of paho Could you please let me know what am I missing? Any help will be appreciated. Thanks! |
I've found a way to fix this, very confusing but it works: Need to use paho-mqtt-c/1.3.5 branch to build 1.3.8 using: conan create . paho-mqtt-c/1.3.8@ After I do this and rebuild everything it works ok. Any idea when the 1.2.0 will be in the conan center please? |
What you described makes no sense 🙃
Now that CCI has passed and has been built successfully it usually 3 to 5 days... This morning there is a hefty backlog, check out prince-chrismc/conan-center-index-pending-review#1 for a list of "easy" reviews (ie the hard part that we just finished here of getting it to work has been done) Please remember CCI is in Early Access, many things are influx as this new project is slowly made available... It will be available as soon as possible. |
I am just describing how I got the version 1.2.0 working on my end. Also, I am sorry for asking again...did not mean to upset you but thought that there were still some outstanding issues to be fixed before making it available via conan center...so I was just curious if the release might be delayed...sorry my bad |
I am more upset with my 9-5 than with you 🤗 A rough monday morning 🙊
Maybe, it takes 3 reviewers... hopefully the community agrees with the approach I put forth, this is how we usually do things so I hope the answer is yes... nothing is guaranteed in life! |
please add 'closes #3760' in the description |
All green in build 8 (
|
Specify library name and version: paho-mqtt-cpp/1.2.0
conan-center hook activated.
This depends on #4063 being done first.
Doe the iOS CMakeFile patch need to be applied?
closes #3760