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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 21, 2021
Merged

Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0 #4096

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b7a59a9
Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0
bowb Jan 5, 2021
083a154
Fix config.yml add version 1.2.0
bowb Jan 5, 2021
bd2076c
Fix FindPahMqttC.cmake for static on WIN32
bowb Jan 6, 2021
3c5b689
Fix patch.
bowb Jan 6, 2021
97518af
Add final newline to patch.
bowb Jan 6, 2021
58739e0
some newlines.
bowb Jan 11, 2021
a62891c
bind upstream version requirements on deps
prince-chrismc Jan 17, 2021
95f020f
Merge pull request #1 from prince-chrismc/update/paho-mqtt-cpp
bowb Jan 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions recipes/paho-mqtt-cpp/all/conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
sources:
"1.2.0":
sha256: 435e97e4d5b1da13daa26cadd3e83fe9d154930abaa78b8ff1b8c854b5345d8b
url: https://github.com/eclipse/paho.mqtt.cpp/archive/v1.2.0.tar.gz
"1.1":
sha256: cb0343349ed91ef51d0e76ae860d19435a730d3d355e57886bb090014cb70bbe
url: https://github.com/eclipse/paho.mqtt.cpp/archive/v1.1.tar.gz
Expand All @@ -7,11 +10,16 @@ sources:
url: https://github.com/eclipse/paho.mqtt.cpp/archive/v1.0.1.tar.gz

patches:
"1.2.0":
- base_path: "source_subfolder"

"1.1":
- patch_file: "patches/1.1/0001-deadlock_and_remlog-for-1-1.patch"
base_path: "source_subfolder"
- patch_file: "patches/1.1/0002-ios_fix.patch"
base_path: "source_subfolder"
- patch_file: "patches/1.1/0003-fix-win32-static-config-cmake.patch"
base_path: "source_subfolder"

"1.0.1":
- patch_file: "patches/1.0.1/0001-fix-cmake-module-path.patch"
Expand All @@ -20,4 +28,6 @@ patches:
base_path: "source_subfolder"
- patch_file: "patches/1.0.1/0003-fix-paho-mqtt-cpp-config-cmake.patch"
base_path: "source_subfolder"
- patch_file: "patches/1.0.1/0004-fix-win32-static-config-cmake.patch"
base_path: "source_subfolder"

4 changes: 2 additions & 2 deletions recipes/paho-mqtt-cpp/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class PahoMqttCppConan(ConanFile):
homepage = "https://github.com/eclipse/paho.mqtt.cpp"
topics = ("MQTT", "IoT", "eclipse", "SSL", "paho", "Cpp")
license = "EPL-1.0"
description = """The open-source client implementations of MQTT and MQTT-SN"""
description = "The open-source client implementations of MQTT and MQTT-SN"
exports_sources = ["CMakeLists.txt", "patches/*"]
generators = "cmake", "cmake_find_package"
settings = "os", "arch", "compiler", "build_type"
Expand Down Expand Up @@ -46,7 +46,7 @@ def configure(self):


def requirements(self):
self.requires("paho-mqtt-c/1.3.5")
self.requires("paho-mqtt-c/1.3.8")

def source(self):
tools.get(**self.conan_data["sources"][self.version])
Expand Down
17 changes: 17 additions & 0 deletions recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
diff --git a/cmake/FindPahoMqttC.cmake b/cmake/FindPahoMqttC.cmake
index 53f15a3..7e2d756 100644
--- a/cmake/FindPahoMqttC.cmake
+++ b/cmake/FindPahoMqttC.cmake
@@ -6,8 +6,10 @@ else()
set(_PAHO_MQTT_C_LIB_NAME paho-mqtt3a)
endif()
# add suffix when using static Paho MQTT C library variant
-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()
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 -cpps will work with newer -cs

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?

Copy link
Contributor Author

@bowb bowb Jan 13, 2021 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@bowb bowb Jan 13, 2021 •
edited
Loading

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

Copy link
Contributor

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

conan-center-index/recipes/paho-mqtt-cpp/all/conanfile.py

Line 14 in 58739e0

generators = "cmake", "cmake_find_package"

Will create the correct Find<Package>.cmake files

conan-center-index/recipes/paho-mqtt-c/all/conanfile.py

Line 93 in 58739e0

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

Copy link
Contributor Author

@bowb bowb Jan 13, 2021 •
edited
Loading

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()

Copy link
Contributor

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...

Copy link
Contributor

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....

endif()

find_library(PAHO_MQTT_C_LIBRARIES NAMES ${_PAHO_MQTT_C_LIB_NAME})
17 changes: 17 additions & 0 deletions recipes/paho-mqtt-cpp/all/patches/1.1/0003-fix-win32-static-config-cmake.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
diff --git a/cmake/FindPahoMqttC.cmake b/cmake/FindPahoMqttC.cmake
index 53f15a3..7e2d756 100644
--- a/cmake/FindPahoMqttC.cmake
+++ b/cmake/FindPahoMqttC.cmake
@@ -6,8 +6,10 @@ else()
set(_PAHO_MQTT_C_LIB_NAME paho-mqtt3a)
endif()
# add suffix when using static Paho MQTT C library variant
-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()
endif()

find_library(PAHO_MQTT_C_LIBRARIES NAMES ${_PAHO_MQTT_C_LIB_NAME})
2 changes: 2 additions & 0 deletions recipes/paho-mqtt-cpp/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ versions:
folder: "all"
"1.1":
folder: "all"
"1.2.0":
folder: "all"