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

paho-mqtt-c: fix cmake names #2729

Merged
merged 1 commit into from Sep 24, 2020

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Aug 31, 2020

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

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

targets are exported since version 1.3.2
See https://github.com/eclipse/paho.mqtt.c/blob/3148fe2d5f4b87e16266dfe559c0764e16ca0546/src/CMakeLists.txt#L308 and check targets added to eclipse-paho-mqtt-cTargets EXPORT

if self.options.ssl:
target += "s"
if not self.options.shared:
target += "-static"
Copy link
Contributor

Choose a reason for hiding this comment

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

poh, this is a mess 💃

Copy link
Contributor

Choose a reason for hiding this comment

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

cant we just change this upstream?

what do you think? @fpagliughi
could we harmonize the paho-mqtt-c and paho-mqtt-cpp to have sane targets and find cmake names

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose PahoMqttC and PahoMqttCpp for find cmake names and paho::mqtt-c and paho::mqtt-cpp
we could also replace PAHO with ECLIPSE...

Copy link
Contributor

Choose a reason for hiding this comment

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

and remove all the madness with static, shared, ssl etc. changes to the name

I mean we could also change the name based on zlib, xy, brotli etc. or use another crypto lib like mbedtls and also add this to the name -> becomes unmaintainable

Copy link
Contributor

Choose a reason for hiding this comment

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

this is okay to provide different target names for different flavours of libraries.
e.g. in conan, we usually don't build static and shared in the same package. but, not everyone uses conan...
while without conan, it's pretty common practice to allow to build shared and static simultaneously (or another flavors, e.g. SSL on + SSL off, debug + release, 32 + 64, etc).
in this case, consumer has to somehow distinguish such flavours, and the best way is that he can refer to these libraries by different CMake targets - that's the most straightforward and obvious way.
so IMO there is nothing to be fixed upstream - just follow the upstream's naming in the recipe, doesn't matter how complicated it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SSE4 interesting that you support this :D

I guess you know the needed effort to support conan recipes across multiple years if everyone starts creating special options, names etc.

Choose a reason for hiding this comment

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

Thing is he's right that not everyone uses conan and there are plenty of cases where builds generate multiple flavors and need different names. Unfortunate but true.

@SpaceIm SpaceIm closed this Sep 2, 2020
@SpaceIm SpaceIm reopened this Sep 2, 2020
@conan-center-bot
Copy link
Collaborator

All green in build 3 (8320ae72363aa4854845a3175040018159cef337)! 😊

@conan-center-bot conan-center-bot merged commit 2ff0517 into conan-io:master Sep 24, 2020
@SpaceIm SpaceIm deleted the fix/paho-mqtt-c-cmake branch September 24, 2020 20:37
@keysight-daryl
Copy link

I think it would be good idea to add the upstream paho-mqtt-c author @icraggs on all PR reviews for this package since he is now active on here developing this.

@keysight-daryl
Copy link

upstream is in the process of releasing the next version of this package right now and will need to reconcile with all these changes shortly.

@uilianries
Copy link
Member

Great news! Thanks @keysight-daryl !

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.

None yet

8 participants