Fix -DHUNTER_ENABLED=0 builds #3

ringerc · 2018-06-05T08:13:35Z

Fixes #1

codecov · 2018-06-05T08:20:31Z

Codecov Report

Merging #3 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master       #3   +/-   ##
=======================================
  Coverage   79.54%   79.54%           
=======================================
  Files          34       34           
  Lines        3568     3568           
=======================================
  Hits         2838     2838           
  Misses        730      730

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8613dda...91b549d. Read the comment docs.

isaachier · 2018-06-05T15:07:54Z

.gitignore

@@ -1 +1,4 @@
 /build
+DartConfiguration.tcl
+/CMakeFiles
+/CMakeCache.txt


For these two directories, please remove the leading slash. This pattern only matches ${SRC_ROOT}/CMakeFiles but not ${SRC_ROOT}/build/CMakeFiles. Usually I use mkdir build && cd build && cmake .. to keep the build in its own directory, so it made sense to ignore the whole build directory in the source root. However, it seems like you are building in the source tree. Might as well just ignore all of the CMake generated files no matter where they are.

You can actually replace this entirely with this: https://github.com/github/gitignore/blob/master/CMake.gitignore.

Maybe add this and any generated files you can find: https://github.com/github/gitignore/blob/master/C.gitignore.

isaachier · 2018-06-05T15:08:39Z

README.md

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


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

codecov-io · 2018-06-07T04:20:32Z

Codecov Report

Merging #3 into master will increase coverage by 0.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
+ Coverage    79.2%   79.56%   +0.36%     
==========================================
  Files          31       34       +3     
  Lines        3635     3572      -63     
==========================================
- Hits         2879     2842      -37     
+ Misses        756      730      -26

Impacted Files	Coverage Δ
src/jaegertracingc/internal/random.h	`90.32% <ø> (ø)`
src/jaegertracingc/span.h	`66.33% <0%> (-33.67%)`	⬇️
src/jaegertracingc/sampling_strategy.h	`90.47% <0%> (-9.53%)`	⬇️
src/jaegertracingc/log_record.h	`96.42% <0%> (-3.58%)`	⬇️
src/jaegertracingc/sampler.c	`81.89% <0%> (-1.63%)`	⬇️
src/jaegertracingc/baggage.c	`0% <0%> (ø)`	⬆️
src/jaegertracingc/metrics.c	`100% <0%> (ø)`	⬆️
src/jaegertracingc/logging.c	`100% <0%> (ø)`	⬆️
src/jaegertracingc/alloc.c	`100% <0%> (ø)`	⬆️
src/jaegertracingc/common.c
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b89ecf7...ee3c238. Read the comment docs.

ringerc · 2018-06-07T04:21:49Z

@isaachier I've dropped the gitignore commit from the series, and I think otherwise it should be fine to merge.

ringerc · 2018-06-07T05:54:59Z

Actually hold on this, needs more work first

There were a number of case sensitivity issues in the CMake files relating to differing case between the Hunter files and the ones in cmake/ (isaachier#1) Also fixes finding of the protocol buffers support when protobuf 1.2 is present (isaachier#4)

Amends 49513a5 (re isaachier#2)

ringerc · 2018-06-11T08:30:03Z

I've removed the gitignore changes, added a fix to make it compile on protobuf-c 1.2, fixed some more issues with case in the CMake files, and rebased on top of 49513a5. It now configures fine here with cmake -DHUNTER_ENABLED=0 -DJAEGERTRACINGC_USE_PCG=0 and builds fine.

isaachier · 2018-06-12T10:43:18Z

.travis.yml

-        - clang-4.0
-    env:
-    - MATRIX_EVAL="CC=clang-4.0 && CXX=clang++-4.0"
+#DISABLED to reduce the matrix a bit


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.

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.

isaachier · 2018-06-12T10:44:09Z

CMakeLists.txt

@@ -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()


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

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.

isaachier · 2018-06-12T10:45:54Z

CMakeLists.txt

+  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)


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.

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.

isaachier · 2018-06-12T10:46:08Z

README.md

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


isaachier · 2018-06-12T10:46:30Z

README.md

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


:) excellent!

isaachier · 2018-06-12T10:47:10Z

README.md

+    git submodule update --init
+    mkdir -p build
+    cd build
+    cmake ..


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

isaachier · 2018-06-12T10:48:13Z

cmake/Findjansson.cmake

-  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


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

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?"

isaachier · 2018-06-12T10:49:36Z

scripts/build.sh

+
+    if [ "${USE_HUNTER:-}" == "off" ]; then
+        # We won't be using Hunter so we need to get opentracing's c library
+        # installed too.


How would you prefer I handle this dependency to support non-Hunter builds?

I'm not sure yet, thinking about that. It depends a lot on whether you think apps would expect to link to opentracing-c then load jaeger-client-c at runtime, or if you see them linking to a jaeger-client-c that implements the opentracing-c APIs exposed in opentracing-c headers.

(The build doesn't produce a shared library yet. I'll deal with that at some later stage, far from urgent.)

You've pushed opentracing-c to hunter, so I guess you see it working more like the former? The app links to opentracing-c, and it loads a tracing implementation at runtime? I haven't looked closely at this part yet, as I've just been trying to get the build to work.

No strong opinions here anyway. I anticipate I'll be treating it as a unit of opentracing-c + jaeger-client-c anyway.

isaachier · 2018-06-12T10:49:49Z

scripts/install-deps.sh

-    make -j3
-    make check -j3
-    make install
+    ./configure --quiet --prefix="$prefix"


isaachier · 2018-06-12T10:50:48Z

src/jaegertracingc/random_test.c

+     * not much benefit when we're just generating request and span IDs, etc.
+     */
+    srand(time(NULL));
+#endif


isaachier suggested changes Jun 5, 2018

View reviewed changes

ringerc force-pushed the find-cmake-case branch from 91b549d to 8810c42 Compare June 7, 2018 04:20

ringerc force-pushed the find-cmake-case branch from 8810c42 to f8715ab Compare June 11, 2018 08:25

ringerc added 4 commits June 11, 2018 16:27

Mention packages for Fedora

8132cfe

Document cmake -DHUNTER_ENABLED=0 -DJAEGERTRACINGC_USE_PCG=0

f6e6a9e

Fix reference to a PCG header when PCG disabled

875f0fc

Amends 49513a5 (re isaachier#2)

ringerc force-pushed the find-cmake-case branch from f8715ab to 875f0fc Compare June 11, 2018 08:28

ringerc changed the title ~~Case of CMake Find modules did not match CMakeLists.txt~~ Fix -DHUNTER_ENABLED=0 builds Jun 11, 2018

ringerc added 7 commits June 12, 2018 11:09

Fixes for finding protobuf-c with !HUNTER_ENABLED

df918fe

[travis] Quiet builds of protobuf and protobuf-c

3e13da5

[travis] Support building with the locally installed CMake on test host

91f0249

[travis] Only build protobuf and protobuf-c when not using Hunter

566f363

[travis] When !HUNTER_ENABLED build and install opentracing in tests

8b40b2a

[travis] Perform test builds with HUNTER_ENABLED=0

81852fe

Disable some test matrix entries to reduce build load

c5b7aa5

ringerc force-pushed the find-cmake-case branch from da44af8 to c5b7aa5 Compare June 12, 2018 03:12

This was referenced Jun 12, 2018

Depends on protobuf-c 1.3 #4

Open

PCG lacks a module in cmake/ #2

Open

No Findjansson.cmake for -DHUNTER_ENABLED=0 #1

Open

ringerc added 2 commits June 12, 2018 11:56

Initialize system RNG when not using PCG

b11c2cd

Expand README for local dependencies

ee3c238

isaachier reviewed Jun 12, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix -DHUNTER_ENABLED=0 builds #3

Fix -DHUNTER_ENABLED=0 builds #3

ringerc commented Jun 5, 2018

codecov bot commented Jun 5, 2018 •

edited

Loading

isaachier Jun 5, 2018

isaachier Jun 5, 2018

isaachier Jun 5, 2018

isaachier Jun 5, 2018

codecov-io commented Jun 7, 2018 •

edited by codecov bot

Loading

ringerc commented Jun 7, 2018

ringerc commented Jun 7, 2018

ringerc commented Jun 11, 2018

isaachier Jun 12, 2018

ringerc Jun 12, 2018

isaachier Jun 12, 2018

ringerc Jun 12, 2018

isaachier Jun 12, 2018

ringerc Jun 12, 2018

isaachier Jun 12, 2018

isaachier Jun 12, 2018

isaachier Jun 12, 2018

isaachier Jun 12, 2018

ringerc Jun 12, 2018

isaachier Jun 12, 2018

ringerc Jun 12, 2018

isaachier Jun 12, 2018

isaachier Jun 12, 2018

Fix -DHUNTER_ENABLED=0 builds #3

Are you sure you want to change the base?

Fix -DHUNTER_ENABLED=0 builds #3

Conversation

ringerc commented Jun 5, 2018

codecov bot commented Jun 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 7, 2018 • edited by codecov bot Loading

Codecov Report

ringerc commented Jun 7, 2018

ringerc commented Jun 7, 2018

ringerc commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2018 •

edited

Loading

codecov-io commented Jun 7, 2018 •

edited by codecov bot

Loading