Skip to content

Builtin rdkafka library with cmake option STATIC_BUILD #5

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 24, 2019
Merged

Builtin rdkafka library with cmake option STATIC_BUILD #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f3e77e
Add librdkafka dependency as submodule
RunsFor May 15, 2019
5a2d440
Link rdkafka statically with BUNDLE_RDKAFKA option
RunsFor May 22, 2019
201f14f
Bypass BUILD_RDKAFKA from rockspec to cmake
RunsFor May 22, 2019
6261ca7
Add cmake build script for rdkafka
RunsFor May 22, 2019
cda420b
Update readme with new cmake option
RunsFor May 23, 2019
abe4ee9
cmake: Rename BUNDLE_RDKAFKA option to STATIC_BUILD
RunsFor May 23, 2019
1984c62
Update url to rockspec from README example
RunsFor May 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "librdkafka"]
path = librdkafka
url = https://github.com/edenhill/librdkafka.git
9 changes: 7 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ if(${tntver} VERSION_LESS 1.7.4.291)
message(FATAL_ERROR "Tarantool >= 1.7.4-291 is required")
endif()

set(RDKAFKA_FIND_REQUIRED ON)
find_package(RdKafka)
if(STATIC_BUILD)
Copy link

Choose a reason for hiding this comment

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

ENABLE_BUNDLED_FOO seems to be a standard name for such configuration options.

https://github.com/tarantool/tarantool/blob/d07e3ac2facfc5c5cec43ff138e2bdbd1fab1011/CMakeLists.txt#L304
https://github.com/tarantool/tarantool-c/blob/e9f17d649c2ee543cd505ceeff59e050763528a9/CMakeLists.txt#L38-L39

Copy link

Choose a reason for hiding this comment

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

I want a single option that will be the same for all rocks and mean roughly "link all your stuff statically, and don't use anything external". Hence STATIC_BUILD

Copy link

Choose a reason for hiding this comment

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

Nothing blocks us with that. We still can add separate options and the helper one. This is the right way, because packaging for distros usually requires linking with system libraries, but sometimes there are problems with some specific system libraries: e.g. lack of them in certain distros.

Hipster way packaging should not break existing agreements. Otherwise it would be the systemd way: a half of the world will hate us.

Copy link

@knazarov knazarov May 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not saying we should remove BUNDLED*, I'm just saying there should be a universal option that you can use to link all dependencies statically. It could be an alias of BUNDLED_* in case if there is only one dependency. But if there is more than one (which is true in case of kafka), it should statically link everything.

Copy link

Choose a reason for hiding this comment

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

As for systemd, there is only a small and vocal hater community, which doesn't even have strong arguments, except the fact that systemd is monolithic.

Copy link

Choose a reason for hiding this comment

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

I'm not against. Thanks for the clarification.

include(buildRdKafka)
buildrdkafka()
else()
set(RDKAFKA_FIND_REQUIRED ON)
find_package(RdKafka)
endif()

include_directories(${TARANTOOL_INCLUDE_DIRS})

Expand Down
10 changes: 9 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,15 @@ non critical errors as strings which allows you to decide how to handle it.

# Installation
```bash
tarantoolctl rocks install https://raw.githubusercontent.com/tarantool/tnt-kafka/master/rockspecs/kafka-scm-1.rockspec
tarantoolctl rocks install https://raw.githubusercontent.com/tarantool/kafka/master/rockspecs/kafka-scm-1.rockspec
```

## Build module with statically linked librdkafka

To install kafka module with builtin librdkafka dependency, use option `STATIC_BUILD`:

```bash
tarantoolctl rocks STATIC_BUILD=ON install https://raw.githubusercontent.com/tarantool/kafka/master/rockspecs/kafka-scm-1.rockspec
```

# Examples
Expand Down
6 changes: 6 additions & 0 deletions cmake/FindRdKafka.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,17 @@ find_path(RDKAFKA_INCLUDE_DIR
NAMES librdkafka/rdkafka.h
HINTS ${RDKAFKA_ROOT_DIR}/include
)

find_library(RDKAFKA_LIBRARY
NAMES ${CMAKE_SHARED_LIBRARY_PREFIX}rdkafka${CMAKE_SHARED_LIBRARY_SUFFIX} rdkafka
HINTS ${RDKAFKA_ROOT_DIR}/lib
)

find_library(RDKAFKA_STATIC
NAMES ${CMAKE_STATIC_LIBRARY_PREFIX}rdkafka${CMAKE_STATIC_LIBRARY_SUFFIX} rdkafka
HINTS ${RDKAFKA_ROOT_DIR}/lib
)

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(RDKAFKA DEFAULT_MSG
RDKAFKA_LIBRARY
Expand Down
10 changes: 10 additions & 0 deletions cmake/buildRdKafka.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
macro(buildrdkafka)
add_custom_target(
rdkafka
COMMAND ./configure
COMMAND make
COMMAND make install
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/librdkafka
VERBATIM
)
endmacro()
10 changes: 9 additions & 1 deletion kafka/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,16 @@ if (APPLE)
-undefined suppress -flat_namespace")
endif(APPLE)

target_link_libraries(tntkafka ${RDKAFKA_LIBRARY} pthread)
target_link_libraries(tntkafka pthread)

if (STATIC_BUILD)
add_dependencies(tntkafka rdkafka)
target_link_libraries(tntkafka ${CMAKE_SOURCE_DIR}/librdkafka/src/librdkafka.a)
else()
target_link_libraries(tntkafka ${RDKAFKA_LIBRARY})
endif()

set_target_properties(tntkafka PROPERTIES PREFIX "" OUTPUT_NAME "tntkafka")

install(TARGETS tntkafka LIBRARY DESTINATION ${TARANTOOL_INSTALL_LIBDIR}/kafka)
install(FILES init.lua DESTINATION ${TARANTOOL_INSTALL_LUADIR}/kafka)
1 change: 1 addition & 0 deletions librdkafka
Open in desktop
Submodule librdkafka added at 8695b9
6 changes: 2 additions & 4 deletions rockspecs/kafka-scm-1.rockspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ dependencies = {
external_dependencies = {
TARANTOOL = {
header = 'tarantool/module.h'
},
RDKAFKA = {
header = 'librdkafka/rdkafka.h'
}
}
build = {
Expand All @@ -26,6 +23,7 @@ build = {
CMAKE_BUILD_TYPE="RelWithDebInfo",
TARANTOOL_DIR="$(TARANTOOL_DIR)",
TARANTOOL_INSTALL_LIBDIR="$(LIBDIR)",
TARANTOOL_INSTALL_LUADIR="$(LUADIR)"
TARANTOOL_INSTALL_LUADIR="$(LUADIR)",
STATIC_BUILD="$(STATIC_BUILD)"
}
}