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.

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

merged 8 commits into from
Jan 21, 2021

Conversation

bowb
Copy link
Contributor

@bowb bowb commented Jan 5, 2021

Specify library name and version: paho-mqtt-cpp/1.2.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

This depends on #4063 being done first.

Doe the iOS CMakeFile patch need to be applied?

closes #3760

@conan-center-bot
Copy link
Collaborator

Failure in build 1 (b7a59a9b14e585bf7d1b15f6e1276b6f47fcb05f):

  • paho-mqtt-cpp/1.0.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [CONFIG.YML HAS NEW VERSION (KB-H052)] The version "1.2.0" exists in "conandata.yml" but not in "../config.yml", so it will not be built. Please update "../config.yml" to include newly added version "1.2.0". (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H052)
  • paho-mqtt-cpp/1.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [CONFIG.YML HAS NEW VERSION (KB-H052)] The version "1.2.0" exists in "conandata.yml" but not in "../config.yml", so it will not be built. Please update "../config.yml" to include newly added version "1.2.0". (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H052)

@conan-center-bot
Copy link
Collaborator

Failure in build 2 (083a154be7404b39720e7630b16af4a325c390b9):

  • Error processing recipe (ref 'paho-mqtt-cpp/1.0.1'): Linux x86_64, Release, gcc 4.9, libstdc++ . Options: paho-mqtt-cpp:shared-False
    You are depending on 'paho-mqtt-c/1.3.8' but it is not in the repository

@conan-center-bot
Copy link
Collaborator

Failure in build 3 (bd2076c60a306504bc945a7bd6424bbba73fc1a9):

  • paho-mqtt-cpp/1.0.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO FINAL ENDLINE (KB-H041)] File '/home/conan/w/cci_PR-4096/recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch' does not end with an endline (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H041)
  • paho-mqtt-cpp/1.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO FINAL ENDLINE (KB-H041)] File '/home/conan/w/cci_PR-4096/recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch' does not end with an endline (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H041)
  • paho-mqtt-cpp/1.2.0
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO FINAL ENDLINE (KB-H041)] File '/home/conan/w/cci_PR-4096/recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch' does not end with an endline (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H041)

@conan-center-bot
Copy link
Collaborator

Failure in build 4 (3c5b6896a1e75b3d5f449b2f86a32c0471d69b21):

  • paho-mqtt-cpp/1.0.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO FINAL ENDLINE (KB-H041)] File '/home/conan/w/cci_PR-4096/recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch' does not end with an endline (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H041)
  • paho-mqtt-cpp/1.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO FINAL ENDLINE (KB-H041)] File '/home/conan/w/cci_PR-4096/recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch' does not end with an endline (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H041)
  • paho-mqtt-cpp/1.2.0
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO FINAL ENDLINE (KB-H041)] File '/home/conan/w/cci_PR-4096/recipes/paho-mqtt-cpp/all/patches/1.0.1/0004-fix-win32-static-config-cmake.patch' does not end with an endline (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H041)

@conan-center-bot
Copy link
Collaborator

Failure in build 5 (97518af5e84de81f764f6f24899e916c03be3579):

  • Error processing recipe (ref 'paho-mqtt-cpp/1.0.1'): Linux x86_64, Release, gcc 4.9, libstdc++ . Options: paho-mqtt-cpp:shared-False
    You are depending on 'paho-mqtt-c/1.3.8' but it is not in the repository

@conan-center-bot
Copy link
Collaborator

All green in build 6 (58739e044c9e34d6baae695d6d9019fecec19d4a)! 😊

Comment on lines 9 to 14
-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

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

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

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

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

Copy link
Contributor Author

@bowb bowb Jan 13, 2021

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

@martinpeniak
Copy link

Hi guys, any idea when I could start using this version please?

paho-mqtt-cpp/1.2.0

I still get:
ERROR: Unable to find 'paho-mqtt-cpp/1.2.0' in remotes

@prince-chrismc
Copy link
Contributor

prince-chrismc commented Jan 12, 2021

@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

@martinpeniak
Copy link

martinpeniak commented Jan 12, 2021

Thanks @prince-chrismc, very much appreciated! For now I am using the workaround from #3760
as I had issues building the previous version for arm due to a bug that was fixed in this PR.

@SSE4 SSE4 requested a review from uilianries January 12, 2021 16:52
@prince-chrismc
Copy link
Contributor

Just so I can find it later conan-io/conan#8053 (review)

@martinpeniak
Copy link

Hi guys, when do you think I will be able to use the version 1.2 please?
I've build it locally and all works fine for me.

Thanks!

@prince-chrismc
Copy link
Contributor

bind upstream version requirements on deps
@conan-center-bot
Copy link
Collaborator

All green in build 7 (95f020fae8e5cb9e50f93680944634ec1fda775c)! 😊

@martinpeniak
Copy link

martinpeniak commented Jan 18, 2021

@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

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
terminate called after throwing an instance of 'mqtt::exception'
what(): MQTT error [-14]: Invalid protocol scheme
Aborted (core dumped)

I've checked out this branch: https://github.com/bowb/conan-center-index/tree/paho-mqtt-cpp-1.2.1
and run: conan create . paho-mqtt-cpp/1.2.0@
in /conan-center-new/recipes/paho-mqtt-cpp/all

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!

@martinpeniak
Copy link

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@
Then need to use paho-mqtt-cpp-1.2.1 branch to build 1.2.0 using: conan create . paho-mqtt-cpp/1.2.0@

After I do this and rebuild everything it works ok. Any idea when the 1.2.0 will be in the conan center please?

@prince-chrismc
Copy link
Contributor

very confusing but it works

What you described makes no sense 🙃

Any idea when the 1.2.0 will be in the conan center please?

⚠️ I am little peeved from you asking this again, if I sound rude I am terribly sorry.

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.

@prince-chrismc prince-chrismc mentioned this pull request Jan 18, 2021
4 tasks
@martinpeniak
Copy link

very confusing but it works

What you described makes no sense

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

@prince-chrismc
Copy link
Contributor

did not mean to upset you

I am more upset with my 9-5 than with you 🤗 A rough monday morning 🙊

so I was just curious if the release might be delayed

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!

@prince-chrismc
Copy link
Contributor

please add 'closes #3760' in the description

@bowb bowb changed the title Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0 Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0 closes #3760 Jan 19, 2021
@bowb bowb changed the title Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0 closes #3760 Update to paho-mqtt-c 1.3.8 and paho-mqtt-cpp 1.2.0 Jan 19, 2021
@conan-center-bot
Copy link
Collaborator

All green in build 8 (95f020fae8e5cb9e50f93680944634ec1fda775c)! 😊

@conan-center-bot conan-center-bot merged commit 1f521e3 into conan-io:master Jan 21, 2021
@bowb bowb deleted the paho-mqtt-cpp-1.2.1 branch January 21, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants