Skip to content

Conversation

@accelerated
Copy link
Contributor

Description

  • Removed dependency from rdkafka inside the cppkafka.pc since rdkafka has its own pkg_config file and it would cause double linking which is unnecessary.
  • Also added bitness detection for path installation and library search for the cppkafka.pc file.
  • Moved dependent libs from Requires to Requires.private and added comma separation between libs.

CMakeLists.txt Outdated
option(CPPKAFKA_RDKAFKA_STATIC_LIB "Link with Rdkafka static library." OFF)

# Determine if this is a 32 or 64 bit build
string(FIND ${CMAKE_CXX_FLAGS} "-m64" BITNESS)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any guarantees there's actually going to be a -m64 in CXX_FLAGS? I've used something like this in the past to detect 32/64 bit architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think your proposal is more platform independent. How about something like this which is simpler? from the 2nd voted answer

math(EXPR BITS "8*${CMAKE_SIZEOF_VOID_P}")
set(BOOST_LIBRARY "/boost/win${BITS}/lib")

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that would also work

@accelerated
Copy link
Contributor Author

accelerated commented May 21, 2019

Done. Btw, for the versioning, perhaps you can bump to 1.0.0 since the sonames are not really matching. Also any plans to finish the admin API?

@mfontanini mfontanini merged commit c733e0b into mfontanini:master May 21, 2019
@mfontanini
Copy link
Owner

Thanks! What do you mean the sonames aren't matching?

I have a separate branch on the admin API stuff but I honestly don't have the time (or the willingness) to finish it now. It's actually mostly done I think. I was just missing something to encapsulate the message responses. Maybe I'll push myself to finish it some day soon.

@accelerated
Copy link
Contributor Author

Thanks! What do you mean the sonames aren't matching?

Sorry i meant the .so versions. Since your version was already tagged in github at a higher number (0.3.x) but your cmake was still building with 0.2

@accelerated
Copy link
Contributor Author

I might give it a go to complete the admin API since we will most likely need it in the near future.

@mfontanini
Copy link
Owner

I already went back and fixed the sonames in 0.3.0 and 0.3.1, that was very broken.

I might give it a go to complete the admin API since we will most likely need it in the near future.

Sounds good! Please let me know (comment on #119) when you do so we don't overlap in case I actually start working on it.

@accelerated
Copy link
Contributor Author

sure

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.

3 participants