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

Fix issue #3760 #3974

Closed
wants to merge 1 commit into from
Closed

Fix issue #3760 #3974

wants to merge 1 commit into from

Conversation

bowb
Copy link
Contributor

@bowb bowb commented Dec 22, 2020

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

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

@CLAassistant
Copy link

CLAassistant commented Dec 22, 2020

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

@ghost
Copy link

ghost commented Dec 22, 2020

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.

@ghost ghost mentioned this pull request Dec 22, 2020
4 tasks
@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

1 similar comment
@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

Comment on lines 93 to 94
self.cpp_info.names["cmake_find_package"] = "eclipse-paho-mqtt-c"
self.cpp_info.names["cmake_find_package_multi"] = "eclipse-paho-mqtt-c"
self.cpp_info.names["cmake_find_package"] = "PahoMqttC"
self.cpp_info.names["cmake_find_package_multi"] = "PahoMqttC"
Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is that paho-mqtt-c does not install cmake modules itself. So we don't know what the correct name is.
Granted, paho-mqtt-cpp uses PahoMqttC, but that's not official either.

There is no good solution ATM.
Conan should have a feature to rename these components for consumers.

Could you please add a comment/FIXME, saying that this name is used by paho-mqtt-cpp?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@Croydon has better eyesight then me apparently. Thanks!

Copy link
Contributor Author

@bowb bowb Dec 22, 2020

Choose a reason for hiding this comment

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

Looks like CMakeLists.txt from paho-mqtt-cpp is looking for PahoMqttC. I think this is where the error is coming from if set like this:

self.cpp_info.names["cmake_find_package"] = "eclipse-paho-mqtt-c"

https://github.com/eclipse/paho.mqtt.cpp/blob/a2e66455337a7e40277ca65ef2a3b045eb223c30/src/CMakeLists.txt#L26

Copy link
Contributor

Choose a reason for hiding this comment

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

It packs its own cmake module: https://github.com/eclipse/paho.mqtt.cpp/blob/a2e66455337a7e40277ca65ef2a3b045eb223c30/cmake/FindPahoMqttC.cmake

But that probably doesn't work with a static paho-mqtt-c library?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're seeing this problem: eclipse-paho/paho.mqtt.cpp#264

I think the correct solution is to fix their cmake script and use the exported targets of paho.mqtt.c.
If you write the patch on top of their current master, maybe they accept it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I'll have to take a look at the paho.mqtt.cpp issue. Thanks,


if(PAHO_MQTT_C_ASYNC)
add_executable(${PROJECT_NAME} test_package_async.c)
if(PAHO_MQTT_C_WITH_SSL)
if(PAHO_MQTT_C_SHARED)
target_link_libraries(${PROJECT_NAME} eclipse-paho-mqtt-c::paho-mqtt3as)
target_link_libraries(${PROJECT_NAME} PahoMqttC::paho-mqtt3as)
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested, but I think you can get away with using PahoMqttC::PahoMqttC.
That target will include all components (shared or static).
(I'm not 100% sure though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this? Seems a bit redundant but it works with different build options.

if(PAHO_MQTT_C_ASYNC)
    add_executable(${PROJECT_NAME} test_package_async.c)
    if(PAHO_MQTT_C_WITH_SSL)
        if(PAHO_MQTT_C_SHARED)
            target_link_libraries(${PROJECT_NAME} PahoMqttC::PahoMqttC)
        else()
            target_link_libraries(${PROJECT_NAME} PahoMqttC::PahoMqttC)
        endif()
    else()
        if(PAHO_MQTT_C_SHARED)
            target_link_libraries(${PROJECT_NAME}  PahoMqttC::PahoMqttC)
        else()
            target_link_libraries(${PROJECT_NAME} PahoMqttC::PahoMqttC)
        endif()
    endif()
else()
    add_executable(${PROJECT_NAME} test_package_client.c)
    if(PAHO_MQTT_C_WITH_SSL)
        if(PAHO_MQTT_C_SHARED)
            target_link_libraries(${PROJECT_NAME}  PahoMqttC::PahoMqttC)
        else()
            target_link_libraries(${PROJECT_NAME}  PahoMqttC::PahoMqttC)
        endif()
    else()
        if(PAHO_MQTT_C_SHARED)
            target_link_libraries(${PROJECT_NAME}  PahoMqttC::PahoMqttC)
        else()
            target_link_libraries(${PROJECT_NAME}  PahoMqttC::PahoMqttC)
        endif()
    endif()
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the conditionals anymore.
But maybe ignore my suggestion and keep it as it was.

The test_package is testing the static/shared targets.
Let's keep it since it's not broken.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

3 similar comments
@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.


IF (NOT WIN32)
- SET_TARGET_PROPERTIES(paho-mqtt3a-static PROPERTIES OUTPUT_NAME paho-mqtt3a)
+ SET_TARGET_PROPERTIES(paho-mqtt3a-static PROPERTIES OUTPUT_NAME paho-mqtt3a-static)
Copy link
Contributor

@madebr madebr Dec 23, 2020

Choose a reason for hiding this comment

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

Instead of a patch, it's also perfectly fine to only add the -static suffix in package_info when on Windows.
e.g.

        if not self.options.shared and self.settings.os == "Windows":
            target += "-static"

This is probably done to avoid mixing up a static library and an import library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this suppose to change the generated library name? There is already a _cmake_target defined but it doesn't appear to change the generated library name. I also don't know if -static needs to be added for a windows build. The _cmake_target hints that does need -static for windows.

self.cpp_info.components["_paho-mqtt-c"].names["cmake_find_package"] = self._cmake_target
self.cpp_info.components["_paho-mqtt-c"].names["cmake_find_package_multi"] = self._cmake_target

def _cmake_target(self):
       target = "paho-mqtt3"
       target += "a" if self.options.asynchronous else "c"
       if self.options.ssl:
           target += "s"
       if not self.options.shared:
           target += "-static"
       return target

Copy link
Contributor

Choose a reason for hiding this comment

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

OUTPUT_NAME sets the name of the output file.

You can do something as follows (untested):

def _cmake_target(self):
       target = "paho-mqtt3"
       target += "a" if self.options.asynchronous else "c"
       if self.options.ssl:
           target += "s"
       if self.settings.os != "Windows" and not self.options.shared:
           target += "-static"
       return target

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

@conan-center-bot
Copy link
Collaborator

config.yml syntax error in build 9:

Only one library can be changed in the same PR: [paho-mqtt-c/all, paho-mqtt-cpp/all, pthreads4w/all]|

@a4z
Copy link
Contributor

a4z commented Jan 2, 2021

I have closed #3834 since there is now 1.3.8 and this should be used, 1.3.6 has. known bugs and limitations.
possible you want to consider to change the recipe so it works with 1.3.8
however, when the C++ lib will be adopted, no one knows, the maintainer does not use the lib himself, so progress is very slow.
And I do not think it is possible to update both recipes in the same PR, so you might want to adopt that

@conan-center-bot
Copy link
Collaborator

config.yml syntax error in build 10:

Only one library can be changed in the same PR: [paho-mqtt-c/all, pthreads4w/all]|

@ghost ghost mentioned this pull request Jan 4, 2021
4 tasks
@bowb bowb closed this Jan 4, 2021
@bowb bowb deleted the paho-mqtt-c/1.3.5 branch January 4, 2021 17:50
@bowb
Copy link
Contributor Author

bowb commented Jan 4, 2021

I'm going to rework this. Closing.

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

Successfully merging this pull request may close these issues.

6 participants