Skip to content
This repository has been archived by the owner on May 16, 2019. It is now read-only.

Fix -DHUNTER_ENABLED=0 builds #3

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix -DHUNTER_ENABLED=0 builds #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0cfd35e
Case of Find modules didn't match CMakeLists.txt usage
ringerc Jun 5, 2018
8132cfe
Mention packages for Fedora
ringerc Jun 5, 2018
f6e6a9e
Document cmake -DHUNTER_ENABLED=0 -DJAEGERTRACINGC_USE_PCG=0
ringerc Jun 11, 2018
875f0fc
Fix reference to a PCG header when PCG disabled
ringerc Jun 11, 2018
df918fe
Fixes for finding protobuf-c with !HUNTER_ENABLED
ringerc Jun 12, 2018
3e13da5
[travis] Quiet builds of protobuf and protobuf-c
ringerc Jun 12, 2018
91f0249
[travis] Support building with the locally installed CMake on test host
ringerc Jun 12, 2018
566f363
[travis] Only build protobuf and protobuf-c when not using Hunter
ringerc Jun 12, 2018
8b40b2a
[travis] When !HUNTER_ENABLED build and install opentracing in tests
ringerc Jun 12, 2018
81852fe
[travis] Perform test builds with HUNTER_ENABLED=0
ringerc Jun 12, 2018
c5b7aa5
Disable some test matrix entries to reduce build load
ringerc Jun 12, 2018
b11c2cd
Initialize system RNG when not using PCG
ringerc Jun 12, 2018
ee3c238
Expand README for local dependencies
ringerc Jun 12, 2018
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
149 changes: 85 additions & 64 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ addons:
apt:
update: true
packages: &1
- libhttp-parser-dev
- libjansson-dev
- pkg-config
- autoconf
- automake
Expand All @@ -22,7 +20,7 @@ matrix:
- os: linux
addons:
apt:
update: true
update: false
packages:
- *1
- g++-4.8
Expand All @@ -32,35 +30,54 @@ matrix:
addons:
apt:
update: true
sources:
- ubuntu-toolchain-r-test
packages:
- *1
- g++-4.9
env:
- MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9"
- os: linux
addons:
apt:
update: true
sources:
- ubuntu-toolchain-r-test
packages:
- *1
- g++-5
env:
- MATRIX_EVAL="CC=gcc-5 && CXX=g++-5"
- os: linux
addons:
apt:
update: true
sources:
- ubuntu-toolchain-r-test
packages:
- *1
- g++-6
- cmake
- g++-4.8
- libhttp-parser-dev
- libjansson-dev
#Can't use these, no suitable package source (see scripts/install-deps.sh
#and ppa:maarten-fonville/protobuf
#- libprotobuf-c-dev
#- protobuf-c-compiler
env:
- MATRIX_EVAL="CC=gcc-6 && CXX=g++-6"
- MATRIX_EVAL="CC=gcc-4.8 && CXX=g++-4.8"
- USE_HUNTER=off
- USE_LOCAL_CMAKE=on
#DISABLED to reduce the matrix a bit
# - os: linux
# addons:
# apt:
# update: true
# sources:
# - ubuntu-toolchain-r-test
# packages:
# - *1
# - g++-4.9
# env:
# - MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9"
# - os: linux
# addons:
# apt:
# update: true
# sources:
# - ubuntu-toolchain-r-test
# packages:
# - *1
# - g++-5
# env:
# - MATRIX_EVAL="CC=gcc-5 && CXX=g++-5"
# - os: linux
# addons:
# apt:
# update: true
# sources:
# - ubuntu-toolchain-r-test
# packages:
# - *1
# - g++-6
# env:
# - MATRIX_EVAL="CC=gcc-6 && CXX=g++-6"
- os: linux
addons:
apt:
Expand All @@ -84,41 +101,43 @@ matrix:
- clang-3.6
env:
- MATRIX_EVAL="CC=clang-3.6 && CXX=clang++-3.6"
- os: linux
addons:
apt:
update: true
sources:
- ubuntu-toolchain-r-test
- llvm-toolchain-precise-3.7
packages:
- *1
- clang-3.7
env:
- MATRIX_EVAL="CC=clang-3.7 && CXX=clang++-3.7"
- os: linux
addons:
apt:
update: true
sources:
- ubuntu-toolchain-r-test
- llvm-toolchain-precise-3.8
packages:
- *1
- clang-3.8
env:
- MATRIX_EVAL="CC=clang-3.8 && CXX=clang++-3.8"
- os: linux
addons:
apt:
update: true
sources:
- llvm-toolchain-trusty-4.0
packages:
- *1
- clang-4.0
env:
- MATRIX_EVAL="CC=clang-4.0 && CXX=clang++-4.0"
#DISABLED to reduce the matrix a bit
Copy link
Owner

Choose a reason for hiding this comment

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

I'm assuming this is for your own testing. In practice, we can add an additional matrix entry (probably one will suffice) that will test HUNTER_ENABLED=0.

Copy link
Author

Choose a reason for hiding this comment

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

It is, but actually I don't see the benefit of the build matrix being so dense for all builds. Unless you're chasing a very specific compiler bug of regression it seems oddly broad. Much more useful to add an osx target at some point instead.

# - os: linux
# addons:
# apt:
# update: true
# sources:
# - ubuntu-toolchain-r-test
# - llvm-toolchain-precise-3.7
# packages:
# - *1
# - clang-3.7
# env:
# - MATRIX_EVAL="CC=clang-3.7 && CXX=clang++-3.7"
# - os: linux
# addons:
# apt:
# update: true
# sources:
# - ubuntu-toolchain-r-test
# - llvm-toolchain-precise-3.8
# packages:
# - *1
# - clang-3.8
# env:
# - MATRIX_EVAL="CC=clang-3.8 && CXX=clang++-3.8"
#DISABLED to reduce the matrix a bit
# - os: linux
# addons:
# apt:
# update: true
# sources:
# - llvm-toolchain-trusty-4.0
# packages:
# - *1
# - clang-4.0
# env:
# - MATRIX_EVAL="CC=clang-4.0 && CXX=clang++-4.0"
- os: linux
addons:
apt:
Expand All @@ -144,6 +163,8 @@ script:
- ./scripts/build.sh "$HOME/install"
after_success:
- ./scripts/upload-coverage.sh
after_failure:
- cat CMakeCache.txt
env:
global:
- LANG="en_US.UTF-8"
Expand Down
40 changes: 23 additions & 17 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,19 +113,24 @@ if(HUNTER_ENABLED)
hunter_add_package(Protobuf)
find_package(Protobuf ${hunter_config} REQUIRED)
list(APPEND package_deps Protobuf)
set(protoc_exe protobuf::protoc)
else()
Copy link
Owner

Choose a reason for hiding this comment

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

So how do we generate the messages? How do we connect to backend at all?

Copy link
Author

Choose a reason for hiding this comment

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

You will have seen the answer to that further down, guessing this is outdated. It's just a change in how the IDL is compiled by cmake.

find_program(protoc_exe protoc)
if(NOT protoc_exe)
message(FATAL_ERROR
"Cannot find protoc executable, please install Protobuf")
endif()
endif()

hunter_add_package(protobuf-c)
find_package(protobuf-c ${hunter_config} REQUIRED)
list(APPEND package_deps protobuf-c)

if(TARGET protobuf-c::protoc-gen-c AND TARGET protobuf::protoc)
message(STATUS "using protoc and protoc-gen-c")
set(protoc_exe $<TARGET_FILE:protobuf::protoc>)
set(protoc_plugin_arg --plugin=$<TARGET_FILE:protobuf-c::protoc-gen-c>)
elseif(TARGET protobuf-c::protoc-c)
Copy link
Owner

Choose a reason for hiding this comment

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

I can fix this issue by using protoc-c directly. I found that I had issues doing this with the Hunter version of protoc-c, so I used an invocation of protoc that loaded the protoc-gen-c plugin. If Hunter is disabled, I can try using protoc-c directly instead.

Copy link
Author

Choose a reason for hiding this comment

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

That's what I did in an earlier revision, but there turns out to be no point. protobuf-c 1.3 adds protobuf-gen-c and is also the oldest version that supports "proto3". So in practice there's a hard requirement on 1.3 and you can just rely on doing it this way.

It's silly that Hunter doesn't expose a protobuf-c::protoc-c target, but in this case it wouldn't help us.

# 1.2-style protoc-c
# the CMake files don't report the version properly
message(FATAL_ERROR "could only find protoc-c not protoc-gen-c; you must have protobuf 1.2 or older, which is not supported")
else()
message(FATAL_ERROR "neither protoc and protoc-gen-c, nor the standalone protoc-c, were found")
endif()

option(JAEGERTRACINGC_USE_PCG "Use pcg random number generation" ON)
if(JAEGERTRACINGC_USE_PCG)
hunter_add_package(pcg)
Expand Down Expand Up @@ -206,16 +211,17 @@ function(generate_protobuf_c gen_src)
set(protoc_out_files
"${generated_src_dir}/protoc-gen/${component}.pb-c.h"
"${generated_src_dir}/protoc-gen/${component}.pb-c.c")
add_custom_command(
OUTPUT ${protoc_out_files}
COMMAND ${protoc_exe}
ARGS --plugin=$<TARGET_FILE:protobuf-c::protoc-gen-c>
--c_out "${generated_src_dir}/protoc-gen"
-I. "${component}.proto"
WORKING_DIRECTORY ${dir}
MAIN_DEPENDENCY ${proto_src}
VERBATIM
USES_TERMINAL)

add_custom_command(
OUTPUT ${protoc_out_files}
COMMAND ${protoc_exe}
ARGS ${protoc_plugin_arg}
--c_out "${generated_src_dir}/protoc-gen"
-I. "${component}.proto"
WORKING_DIRECTORY ${dir}
MAIN_DEPENDENCY ${proto_src}
VERBATIM
USES_TERMINAL)
list(APPEND out_files ${protoc_out_files})
endforeach()
set(${gen_src} ${out_files} PARENT_SCOPE)
Expand Down
74 changes: 67 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,72 @@

## Install

Install the project library dependencies. On Debian, the following command
should work.
By default the client build will fetch all dependencies from the Hunter
build dependency system. Only cmake must be installed.
Copy link
Owner

Choose a reason for hiding this comment

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

👍


```
sudo apt install libhttp-parser-dev libjansson-dev protobuf-c-compiler \
libprotobuf-c-dev
```
This builds a static library which must be linked into the application
along with all its dependencies. The simplest way to do that is to use
Hunter to find it in your own application's CMakeLists.txt.

Run `git submodule update --init` to fetch the submodules.
git submodule update --init
mkdir -p build
cd build
cmake ..
Copy link
Owner

Choose a reason for hiding this comment

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

Just minor suggestion: cmake -DCMAKE_BUILD_TYPE=Release .. or Debug.

make -s
make -s test
make -s install

## Install (manual dependencies)

If you don't want to use Hunter or you can't you'll want to use local
dependencies. This will be the case if you need offline builds, are building
for packages that must be reproducible, or aren't using cmake in your own
application's builds.

Hunter may be disabled with

cmake -DHUNTER_ENABLED=0 -DJAEGERTRACINGC_USE_PCG=0

in which case local library dependencies must be installed:

* protobuf
https://developers.google.com/protocol-buffers/
* protobuf-c >= 1.3.0
https://github.com/protobuf-c/protobuf-c
* libhttp-parser
https://github.com/nodejs/http-parser
* libjansson-dev
https://github.com/akheron/jansson
* OpenTracing C API
https://github.com/opentracing/opentracing-c
(in `third_party/opentracing-c/`)

Run `git submodule update --init` to fetch submodules for IDL and testing.

Most systems do not package protobuf-c 1.3.0, so a source install or backported
packages will be required.

### Dependencies (Debian/Ubuntu)

Install avilable dependencies with:

sudo apt install libhttp-parser-dev libjansson-dev

# For protocol buffers 3.3, Ubuntu only:
sudo add-apt-repository ppa:maarten-fonville/protobuf
sudo apt-get update
sudo apt-get install libprotobuf-dev protobuf-compiler

The packages `protobuf-c-compiler` and `libprotobuf-c-dev`, if available, will
be too old and must be installed from source or a backports repository.

### Dependencies (Fedora)

On Fedora 28:

sudo dnf install jansson-devel http-parser-devel \
protobuf-c-devel protobuf-c-compiler

Copy link
Owner

Choose a reason for hiding this comment

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

Nice! I haven't tested HUNTER_DISABLED=ON in a while, so I should double-check this still works.

On older Fedora releases, RHEL, and CentOS, protobuf-c-devel
protobuf-c-compiler are too old and must be installed from source, custom
packages, or a backport repository instead.
Copy link
Owner

Choose a reason for hiding this comment

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

:) excellent!

8 changes: 4 additions & 4 deletions cmake/Findhttp-parser.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#
# and the following imported targets::
#
# http-parser::http-parser - The http-parser library
# http-parser::http_parser - The http-parser library
#
# Based on https://cmake.org/cmake/help/v3.10/manual/cmake-developer.7.html#a-sample-find-module

Expand Down Expand Up @@ -55,9 +55,9 @@ if(HTTP_PARSER_FOUND)
set(HTTP_PARSER_DEFINITIONS ${PC_HTTP_PARSER_CFLAGS_OTHER})
endif()

if(HTTP_PARSER_FOUND AND NOT TARGET http-parser::http-parser)
add_library(http-parser::http-parser UNKNOWN IMPORTED)
set_target_properties(http-parser::http-parser PROPERTIES
if(HTTP_PARSER_FOUND AND NOT TARGET http-parser::http_parser)
add_library(http-parser::http_parser UNKNOWN IMPORTED)
set_target_properties(http-parser::http_parser PROPERTIES
IMPORTED_LOCATION "${HTTP_PARSER_LIBRARY}"
INTERFACE_COMPILE_OPTIONS "${PC_HTTP_PARSER_CFLAGS_OTHER}"
INTERFACE_INCLUDE_DIRECTORIES "${HTTP_PARSER_INCLUDE_DIR}")
Expand Down
8 changes: 4 additions & 4 deletions cmake/FindJansson.cmake → cmake/Findjansson.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#
# and the following imported targets::
#
# Jansson::Jansson - The Jansson library
# jansson::jansson - The Jansson library
#
# Based on https://cmake.org/cmake/help/v3.10/manual/cmake-developer.7.html#a-sample-find-module

Expand Down Expand Up @@ -55,9 +55,9 @@ if(JANSSON_FOUND)
set(JANSSON_DEFINITIONS ${PC_JANSSON_CFLAGS_OTHER})
endif()

if(JANSSON_FOUND AND NOT TARGET Jansson::Jansson)
add_library(Jansson::Jansson UNKNOWN IMPORTED)
set_target_properties(Jansson::Jansson PROPERTIES
if(JANSSON_FOUND AND NOT TARGET jansson::jansson)
add_library(jansson::jansson UNKNOWN IMPORTED)
set_target_properties(jansson::jansson PROPERTIES
Copy link
Owner

Choose a reason for hiding this comment

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

Fantastic! The casing can be annoying, but that seems to be the correct fix.

Copy link
Author

Choose a reason for hiding this comment

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

Very. CMake could at least offer a "hint" message of some kind. "Target Jansson::Jansson not found (but there's a jansson::jansson, did you mean that?"

IMPORTED_LOCATION "${JANSSON_LIBRARY}"
INTERFACE_COMPILE_OPTIONS "${PC_JANSSON_CFLAGS_OTHER}"
INTERFACE_INCLUDE_DIRECTORIES "${JANSSON_INCLUDE_DIR}")
Expand Down
Loading