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

add paho-mqtt-c/1.3.8 #4063

Merged
merged 3 commits into from
Jan 9, 2021

Conversation

Croydon
Copy link
Contributor

@Croydon Croydon commented Jan 3, 2021

Specify library name and version: paho-mqtt-c/1.3.8

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

@Croydon
Copy link
Contributor Author

Croydon commented Jan 3, 2021

I have not applied the CMake patch for this, since it is kinda unclear what it is for and it seems to work just fine without

That said, this patch goes back to the original recipe in https://github.com/conan-community/conan-paho-c

And it seems like @uilianries wrote the patch in order to "Install only SSL when required"

https://github.com/conan-community/conan-paho-c/commits/stable/1.3.0/0002-fix-cmake-install.patch

// @icraggs

@Croydon Croydon marked this pull request as ready for review January 3, 2021 03:10
@ghost
Copy link

ghost commented Jan 3, 2021

I detected other pull requests that are modifying paho-mqtt-c/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot
Copy link
Collaborator

An unexpected error happened and has been reported. Help is on its way! 🏇

@conan-center-bot
Copy link
Collaborator

An unexpected error happened and has been reported. Help is on its way! 🏇

@Croydon Croydon closed this Jan 3, 2021
@Croydon Croydon reopened this Jan 3, 2021
@conan-center-bot
Copy link
Collaborator

All green in build 3 (1f93611dcd748361ec7551f99f525f70d5c15d05)! 😊

@prince-chrismc
Copy link
Contributor

It also apears to delete some of the test targets

@SSE4 SSE4 requested a review from uilianries January 4, 2021 04:42
Copy link
Contributor

@a4z a4z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a note:
the still used pack fix MingGW and OSX, seems to have nothing to do with OSX and MingGW

all it does is removing the -MD flag

 IF(WIN32)
-  ADD_DEFINITIONS(-D_CRT_SECURE_NO_DEPRECATE -DWIN32_LEAN_AND_MEAN -MD)
+  ADD_DEFINITIONS(-D_CRT_SECURE_NO_DEPRECATE -DWIN32_LEAN_AND_MEAN)
 ELSEIF(${CMAKE_SYSTEM_NAME} STREQUAL "Darwin")
   ADD_DEFINITIONS(-DOSX)
 ENDIF()

so the name is a bit misleading .. ?

(and I wonder if this patch is needed at all ?)

PS:
this seems the way to handle that, but requires cmake 3.15 , what is actually ok, but paho is still on 2.8.??
https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 9, 2021

just a note:
the still used pack fix MingGW and OSX, seems to have nothing to do with OSX and MingGW

all it does is removing the -MD flag

 IF(WIN32)
-  ADD_DEFINITIONS(-D_CRT_SECURE_NO_DEPRECATE -DWIN32_LEAN_AND_MEAN -MD)
+  ADD_DEFINITIONS(-D_CRT_SECURE_NO_DEPRECATE -DWIN32_LEAN_AND_MEAN)
 ELSEIF(${CMAKE_SYSTEM_NAME} STREQUAL "Darwin")
   ADD_DEFINITIONS(-DOSX)
 ENDIF()

so the name is a bit misleading .. ?

(and I wonder if this patch is needed at all ?)

PS:
this seems the way to handle that, but requires cmake 3.15 , what is actually ok, but paho is still on 2.8.??
https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html

patch name is misleading, but I'm pretty sure this patch is required. MD shouldn't be forced.

@conan-center-bot conan-center-bot merged commit 30b6912 into conan-io:master Jan 9, 2021
@CroydonBot CroydonBot deleted the paho-mqtt-c-1-3-8 branch January 9, 2021 21:28
@a4z
Copy link
Contributor

a4z commented Jan 18, 2021

I wonder if anyone is successfully using this recipe in the version

I get strange errors,

I do not have them with the 1.3.7 version, that was successfully ignored here , #3834

difference is in the cmake files, here all the patches are missing ....

duplicate symbol '_SHA1_Init' in:
    /Users/a4z/elux/nsdk/.conan/data/paho-mqtt-c/1.3.8/_/_/package/c0e6b2a0b169ea50c3ed9c3ac7f2ea9f2263e4b5/lib/libpaho-mqtt3a.a(SHA1.c.o)
    /Users/a4z/elux/nsdk/.conan/data/openssl/1.1.1i/_/_/package/647afeb69d3b0a2d3d316e80b24d38c714cc6900/lib/libcrypto.a(sha1dgst.o)
duplicate symbol '_SHA1_Final' in:
    /Users/a4z/elux/nsdk/.conan/data/paho-mqtt-c/1.3.8/_/_/package/c0e6b2a0b169ea50c3ed9c3ac7f2ea9f2263e4b5/lib/libpaho-mqtt3a.a(SHA1.c.o)
    /Users/a4z/elux/nsdk/.conan/data/openssl/1.1.1i/_/_/package/647afeb69d3b0a2d3d316e80b24d38c714cc6900/lib/libcrypto.a(sha1dgst.o)
duplicate symbol '_SHA1_Update' in:
    /Users/a4z/elux/nsdk/.conan/data/paho-mqtt-c/1.3.8/_/_/package/c0e6b2a0b169ea50c3ed9c3ac7f2ea9f2263e4b5/lib/libpaho-mqtt3a.a(SHA1.c.o)
    /Users/a4z/elux/nsdk/.conan/data/openssl/1.1.1i/_/_/package/647afeb69d3b0a2d3d316e80b24d38c714cc6900/lib/libcrypto.a(sha1dgst.o)
ld: 3 duplicate symbols for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@a4z
Copy link
Contributor

a4z commented Jan 18, 2021

also that erorr, what does that mean

duplicate symbol '_MQTTClient_strerror' in:
    /Users/a4z/elux/nsdk/.conan/data/paho-mqtt-c/1.3.8/_/_/package/c0e6b2a0b169ea50c3ed9c3ac7f2ea9f2263e4b5/lib/libpaho-mqtt3c.a(MQTTClient.c.o)
    /Users/a4z/elux/nsdk/.conan/data/paho-mqtt-c/1.3.8/_/_/package/c0e6b2a0b169ea50c3ed9c3ac7f2ea9f2263e4b5/lib/libpaho-mqtt3cs.a(MQTTClient.c.o)

this , libpaho-mqtt3cs.a / libpaho-mqtt3c.a
is super strange ....

@Croydon , do you actually use that package in a project ?

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 18, 2021

libpaho-mqtt3c and libpaho-mqtt3cs shouldn't be packaged together, because we rely on tools.collect_libs(self). Happy ODR violation.

AFAIK, it was the purpose of fix-cmake-install.patch to filter build and installation of libs depending on shared, ssl and asynchronous options. I thought that the behaviour was modified upstream in 1.3.8 since the patch has been removed in this version :s

@a4z
Copy link
Contributor

a4z commented Jan 18, 2021

yes, just tested, I built 1.3.8 base on #3834 where I adopted the cmake patches so they work with the new files

I haven't tested all the profile I use, but that one I did work.

I have pretty strong negative feelings for this package, its wasting my time like no other package 😠
This cmake should be fixed upstream, it is a mess. But after the author behaved so bad against me, I will not do that.
But this needs to be fixed upstream , the patches are nearly not reproducible, when the upstream project again does changes with files, it will be hard to adopt the patches , did it once and do not want to do it again

@a4z a4z mentioned this pull request Jan 18, 2021
4 tasks
@a4z
Copy link
Contributor

a4z commented Jan 18, 2021

I committed a fix
#4293

@prince-chrismc
Copy link
Contributor

@a4z I know your pain #4096 took me a week to understand, the team responsible for those projects is not interested upgrading the CMake, there's little we can do 🤷

Based on Space's comments above, we need to use explicit libs to avoid the patches, I might follow up on this next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants