proposal: keep public headers in common include/smf directory

[dotnwat]

Compiling the demo apps outside the smf tree, using smf as a git sub-module. This seems to work well, but the include paths are a little weird.

For example, the app wants to include histogram stuff:

include "histogram/histogram_seastar_utils.h"

but for this path to work we'd need to add the entire smf/src directory to the list of includes of the including project. alternatively, one can do:

smf/src/include/smf/{histogram, rpc, etc...}

and in the including project simply do include_directories(smf/src/include) then

include <smf/histogram_seastar_utils.h>

etc...

there are other schemes, too...

[crackcomm]

I would like to see distinct #include <seastar/net/tls.hh> also.

[dotnwat]

@crackcomm yeh, i think that'll happen eventually. <core/...> is far too general. i wonder if that has been brought up on the seastar mailing list...?

[emaxerrno]

@crackcomm - yeah, actually only until recently, seastar::* wasn't even an option, so it made it impossible to know where the code was coming from.

I think we need to figure out which ones are public headers and which ones are private.

Then the public ones go into it?

Do you have any examples of how other projects do it.

• Seastar does it the same as this project.
• facebook folly/wangle does it the same as well actually
• fmt is header only, so hard to judge.

Let me see what seems like a minimal and complete API we should expose.

[crackcomm]

I always find TensorFlow as well organized codebase, division between src and include is less accessible.

Seastar uses similar to current smf design and scylladb includes are <seastar/...>, same can be here.

[emaxerrno]

re: tensor flow - yeah, bazel makes this explicit and easy. I use it internally at work. we have cmake today and it's pretty decent for open source stuff.

I'm actually not sure how scylla does it... i think they actually build seastar in tree.

will look.

also, we welcome patches if you have expertise in this area.

[dotnwat]

been hacking on this a bit... @senior7515 what's your opinion on exposing a single smf library? currently the project builds and installs smf_{utils, hashing, tracing} of which utils is the largest at < 400 LOC. That has the potential to simplify any instructions on how to build and manage dependencies.

[emaxerrno]

LGTM. really a lot of this structure was needed because the project was growing in scope and in code. Currently the code is much more stable and not growing as fast... and much smaller in scope.

so this sounds good to me.

[emaxerrno]

I really like this idea. Also, i see some code that isn't used anymore - was used by the filesystem stuff.

Maybe after your PR, i can go around and make it even smaller.

[emaxerrno]

@noahdesu I was going to start hacking on this. Do you want to push a PR for this?

[emaxerrno]

so I think we stand to simply most things now, since there are no multiple projects in here.

It will either be in the src/* or in the include/smf/*

smf is the RPC, so no need for src/rpc/*

[emaxerrno]

Writing down more thoughts:

We should have a CMake option to just build the smf_gen after this issue is fixed, so users from other languages (Go from @crackcomm) can just ask the build system to build the generator which doesn't need to build seastar.

[crackcomm]

@senior7515 this is a good idea :+1: thanks

[dotnwat]

tldr; i don't have a meaning pr ready for this, so definitely take a stab at it.

here are some thoughts i had while working on this issue, but ultimately deleted the first attempt...

1. i think the goal of having public headers in src/include/smf/* is a good pattern. users can include <smf/*>, and all the cmake header install directives can be consolidated in src/include.

2. agree that src/rpc and all of the rpc_ prefixed names are redundant. even naming it libsmf makes sense.

3. +1 to pulling smf_gen up a level.

Those three items were really quick to tackle, and made smf's use in a separate cmake project pretty nice. The issues that need to be addressed:

1. it would appear that rpc_generated.h needs to be in include/smf/. that's no big deal, it's just a matter of making sure the dependencies are output paths are setup correctly.

2. it looks like flatbuffers library is statically linked into smf_gen so no install needed, but flatbuffers/* also needs to be public. this is a non-issue for users of smf that use smf as a sub-module: the installable output from those projects all have statically linked dependencies from smf's third party bits, and the transitive fb header dependencies can be setup correctly through cmake magic.

3. as for smf installs, it doesn't appear that smf's users will ever need to link against flatbuffers library, so it is possible that an smf install could simply copy flatbuffers/* to include/smf/flatbuffers. are your flatbuffers upstream changes required for the smf users, or are those flatbuffers changes only internal to smf library and/or smf_gen? that's probably the first question to ask.

that's all i can think of right now.

[emaxerrno]

in addition, i'm renaming smf_gen -> smfc as the smf compiler basically. to keep in line with libsmf and smfc . similar mental model to libprotobuf and protoc for the compiler.

[emaxerrno]

I have the smfc working now. Will fix libsmf tomorrow.

cmake .. -DENABLE_TESTS=OFF -DSMF_BUILD_COMPILER=ON -DSMF_BUILD_LIBSMF=OFF

[emaxerrno]

hi @noahdesu - need some cmake help.. hehe. https://github.com/senior7515/smf/blob/4136b03cab475bab2a249d4cffa67a74b3573581/src/CMakeLists.txt

Not sure if you can see something obviously wrong w/ this, but somehow, third_party is not being included (fmtlib). :(

[dotnwat]

Hmm lots of build configurations w all the options :) what’s a cmake command you’re using that I can run in that branch? On Sat, Mar 3, 2018 at 17:13 Alexander Gallego notifications@github.com wrote:

hi @noahdesu https://github.com/noahdesu - need some cmake help.. hehe.

https://github.com/senior7515/smf/blob/4136b03cab475bab2a249d4cffa67a74b3573581/src/CMakeLists.txt

Not sure if you can see something obviously wrong w/ this, but somehow, third_party is not being included (fmtlib). :(

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/senior7515/smf/issues/206#issuecomment-370193492, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOy8R0p68WlfH9vr6kHaQj_4NJJSMKmks5taz-xgaJpZM4SSdvV .

[emaxerrno]

I figured that one out. almost ready for something reviewable...

cmake -DSEASTAR_DIR=/home/agallego/workspace/seastar  -DSMF_ENABLE_TESTS=OFF ..

[dotnwat]

sweet, i'll test it out as soon as its ready

[emaxerrno]

I think is ready to be tested :)

[emaxerrno]

@noahdesu i also fixed the smf compiler function

https://github.com/senior7515/smf/blob/master/CMake/smf.cmake#L3

and the usage is here

https://github.com/senior7515/smf/blob/master/demo_apps/CMakeLists.txt#L1-L5

once we get @crackcomm 's Go support merged we can do:

smfc_gen(
  GOLANG
  TARGET_NAME demo_fbs
  OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
  SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/demo_service.fbs)

and the structure is already there too.

[emaxerrno]

this is done https://travis-ci.org/senior7515/smf/jobs/348936234

and it works, you guys can see it in the logs.

one annoying this is that

-- Installing: /tmp/tmp.wYpcrcbCnr/lib/libhdr_histogram_static.a
-- Installing: /tmp/tmp.wYpcrcbCnr/include/hdr/hdr_histogram.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/hdr/hdr_histogram_log.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/hdr/hdr_time.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/hdr/hdr_writer_reader_phaser.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/hdr/hdr_interval_recorder.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/hdr/hdr_thread.h
-- Installing: /tmp/tmp.wYpcrcbCnr/share/pkgconfig/libzstd.pc
-- Installing: /tmp/tmp.wYpcrcbCnr/include/zstd.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/zbuff.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/zdict.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/zstd_errors.h
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/libzstd.a
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/libbenchmark.a
-- Installing: /tmp/tmp.wYpcrcbCnr/include/benchmark
-- Installing: /tmp/tmp.wYpcrcbCnr/include/benchmark/benchmark.h
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/cmake/benchmark/benchmarkConfig.cmake
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/cmake/benchmark/benchmarkConfigVersion.cmake
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/pkgconfig/benchmark.pc
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/cmake/benchmark/benchmarkTargets.cmake
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/cmake/benchmark/benchmarkTargets-release.cmake
-- Installing: /tmp/tmp.wYpcrcbCnr/bin/smfc
-- Installing: /tmp/tmp.wYpcrcbCnr/lib/libsmf.a
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/random.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/load_generator.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/histogram_seastar_utils.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_server_stats.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/load_generator_args.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_service.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/stdx.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_typed_envelope.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/log.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_client.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/macros.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/histogram.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_handle_router.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/time_utils.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_connection_limits.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_letter.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_server.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_recv_context.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_header_utils.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_filter.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_server_connection.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/native_type_utils.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_recv_typed_context.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/unique_histogram_adder.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/fbs_typed_buf.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/compression.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/human_bytes.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/futurize_utils.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_envelope.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_connection.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/load_generator_duration.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/lz4_filter.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/zstd_filter.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/load_channel.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/flatbuffers_concepts.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc.smf.fb.h
-- Installing: /tmp/tmp.wYpcrcbCnr/include/smf/rpc_generated.h
-- Installing: /tmp/tmp.wYpcrcbCnr/bin/smf_histograms_integration_test
-- Installing: /tmp/tmp.wYpcrcbCnr/bin/smf_rpc_integration_test
-- Installing: /tmp/tmp.wYpcrcbCnr/bin/smf_rpc_recv_timeout_integration_test
-- Installing: /tmp/tmp.wYpcrcbCnr/bin/demo_server

it also install's all the transitive cmake files that have install targets too :(

[dotnwat]

this is the patch i made while testing, with adding the include PUBLIC ${PROJECT_SOURCE_DIR}/src being the only required change that i had to make. i'm using master now in my play 3rd party project here https://github.com/noahdesu/verve

[nwatkins@bourbon smf]$ git diff
diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
index f549374..6974642 100644
--- a/src/core/CMakeLists.txt
+++ b/src/core/CMakeLists.txt
@@ -7,7 +7,7 @@ find_package(PkgConfig REQUIRED)
 smfc_gen(
   CPP
   TARGET_NAME raw_rpc_gen
-  OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}/src/smf
+  OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/src/smf
   SOURCES ${PROJECT_SOURCE_DIR}/src/core/rpc.fbs)

 # seastar dep
@@ -56,7 +56,8 @@ target_compile_options(smf
   PUBLIC ${SEASTAR_CFLAGS})

 target_include_directories(smf
-  PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}
+  PUBLIC ${PROJECT_SOURCE_DIR}/src
+  PUBLIC ${PROJECT_BINARY_DIR}/src
   PUBLIC ${CMAKE_CURRENT_BINARY_DIR}
   )
 target_include_directories(smf

as for the install issue you saw, that is sort of like the histogram issue from last week. external project is a solution, or what ends up being easiest is to fork and/or push changes upstream.

[emaxerrno]

@noahdesu i made a PR #221 To address all your comments! thank you.

However, say you are a user that doesn't use cmake and just want to make install away, this will not have the file in the installed directories, will it ?

[dotnwat]

you mean rpc_generated.h ? make install should install the generated file. for example, just edit smfc_gen and add an install() directive.

[dotnwat]

something like install(SOURCES ${PROJECT_BUILD_DIR}/src/smf/rpc_generated.h include)

[emaxerrno]

ok, fixed all of these comments as well. thanks @noahdesu !!!