Split scheme to server and client ones #1550

l4l · 2018-07-09T20:43:55Z

Description of the Change

Fixed build of standalone shared_model build with tests
schema is now for the client and the daemon
Split block.proto to block.proto and transaction.proto

Benefits

Weaker coupling of shared model component

Possible Drawbacks

Building grpc protobuf services now isn't straightforward and require SCHEMA_PATH variable to be set

Signed-off-by: Kitsu <mail@kitsu.me>

nickaleks · 2018-07-10T08:14:09Z

schema/CMakeLists.txt

+# SPDX-License-Identifier: Apache-2.0
+
+set(SCHEMA_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../shared_model/schema/)
+compile_proto_only_grpc_to_cpp(endpoint.proto "-I${CMAKE_CURRENT_SOURCE_DIR}/../shared_model/schema/")


-I${SCHEMA_PATH} ?

nickaleks · 2018-07-10T08:17:37Z

shared_model/clean.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
-rm ../schema/*.{cc,h}
+rm schema/*.pb.{cc,h}


Now that all generated files are inside build directory do we still need this?

nickaleks · 2018-07-10T08:18:27Z

shared_model/clean.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
-rm ../schema/*.{cc,h}
+rm schema/*.pb.{cc,h}


Now that all generated files are inside build directory do we still need this?

nickaleks · 2018-07-10T08:19:47Z

shared_model/schema/CMakeLists.txt

+target_link_libraries(schema
+    protobuf
+    )
+set_target_properties(schema PROPERTIES


Maybe use target_include_directories? It is cleaner and more idiomatic.

Also, I think we need PUBLIC for include directories, since they are used by generated files themselves (e. g. block includes transaction)

nickaleks · 2018-07-10T08:22:11Z

test/module/shared_model/backend_proto/CMakeLists.txt

-    shared_model_stateless_validation
-    shared_model_cryptography
-    )
+if (IROHA_ROOT_PROJECT)


What is the reason for this conditional?

Fixed build of standalone shared_model build with tests

Just try to build shared_model without any fixes

Signed-off-by: Kitsu <mail@kitsu.me>

nickaleks · 2018-07-10T10:51:31Z

schema/CMakeLists.txt

+compile_proto_to_grpc_cpp(mst.proto "-I${SM_SCHEMA_PATH}")
+
+add_library(endpoint
+    # ${SCHEMA_OUT_DIR}/endpoint.pb.cc


Remove comment?

nickaleks · 2018-07-10T10:54:39Z

schema/CMakeLists.txt

+
+set(SM_SCHEMA_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../shared_model/schema/)
+set(SCHEMA_PATH ${SM_SCHEMA_PATH})
+compile_proto_only_grpc_to_cpp(endpoint.proto "-I${SM_SCHEMA_PATH}")


Logically it should be SCHEMA_PATH?

Not really, the model proto should be included for the building grpc services

lebdron · 2018-07-10T11:04:37Z

example/java/prepare.sh

@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
 CURDIR="$(cd "$(dirname "$0")"; pwd)"
 IROHA_HOME="$(dirname $(dirname "${CURDIR}"))"
-cmake -H$IROHA_HOME -Bbuild -DSWIG_JAVA=ON -DSWIG_JAVA_PKG="jp.co.soramitsu.iroha";
+cmake -H$IROHA_HOME -Bbuild -DSWIG_JAVA=ON -DSWIG_JAVA_PKG="jp.co.soramitsu.iroha" -GNinja


I don't think that every script user has Ninja installed, so it seems better to leave the default value.

lebdron · 2018-07-10T11:11:01Z

shared_model/schema/CMakeLists.txt

+compile_proto_to_cpp(proposal.proto)
+compile_proto_to_cpp(qry_responses.proto)
+compile_proto_to_cpp(queries.proto)
+compile_proto_to_cpp(endpoint.proto "")


"" looks reduntant, please remove.

Signed-off-by: Kitsu <mail@kitsu.me>

sorabot · 2018-07-10T15:20:04Z

SonarQube analysis reported 5 issues

query_response_template.hpp#L204: syntax error
pb_common.cpp#L75: The function 'serializeAsset' is never used.
pb_common.cpp#L93: The function 'serializeDomain' is never used.
pb_query_response_factory.hpp#L54: Function parameter 'pb_response' should be passed by reference.
transport_builder_test.cpp#L155: The function 'createInvalidProposal' is never used.

vdrobnyi

What was the reason to rename responses to qry_responses?

l4l · 2018-07-11T21:40:51Z

There's none actually. The first impl considered splitting endpoint to torii_responses, so qry_responses looks more accurate and meaningful naming

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added 3 commits July 9, 2018 23:38

Wrap internal tests

e28db02

Signed-off-by: Kitsu <mail@kitsu.me>

Extract SM-related protobuf and move it

acb8db8

Signed-off-by: Kitsu <mail@kitsu.me>

Add copyrights

3b94512

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added the needs-review pr awaits review from maintainers label Jul 9, 2018

l4l requested review from vdrobnyi and nickaleks July 9, 2018 20:43

Move Transaction to separate proto

5362c65

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the fix/move_proto branch from 99397b3 to 5362c65 Compare July 9, 2018 20:48

l4l changed the title ~~Fix/move proto~~ Split scheme to server and client ones Jul 9, 2018

Fix bindings

01a74d0

Signed-off-by: Kitsu <mail@kitsu.me>

nickaleks suggested changes Jul 10, 2018

View reviewed changes

Cleanup schema cmakes and clean.sh

30425b2

Signed-off-by: Kitsu <mail@kitsu.me>

nickaleks approved these changes Jul 10, 2018

View reviewed changes

lebdron reviewed Jul 10, 2018

View reviewed changes

l4l added 3 commits July 10, 2018 16:45

Fix typos in schema cmakes

d0704bc

Signed-off-by: Kitsu <mail@kitsu.me>

Revert setting ninja generator in java example

85e7eeb

Signed-off-by: Kitsu <mail@kitsu.me>

Remove redundant deps in schema cmake

8909165

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the fix/move_proto branch from c344d35 to 8909165 Compare July 10, 2018 14:34

vdrobnyi approved these changes Jul 11, 2018

View reviewed changes

l4l merged commit 4a83590 into develop Jul 11, 2018

l4l deleted the fix/move_proto branch July 11, 2018 22:26

l4l mentioned this pull request Jul 19, 2018

Out of source proto building #1223

Closed

neewy mentioned this pull request Jul 25, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l added a commit that referenced this pull request Jul 25, 2018

Split scheme to server and client ones (#1550)

923f216

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added a commit that referenced this pull request Jul 25, 2018

Split scheme to server and client ones (#1550)

501373e

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added a commit that referenced this pull request Jul 25, 2018

Split scheme to server and client ones (#1550)

d625e10

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added a commit that referenced this pull request Jul 25, 2018

Split scheme to server and client ones (#1550)

dc907e4

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added a commit that referenced this pull request Jul 25, 2018

Split scheme to server and client ones (#1550)

b697963

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added shared model and removed needs-review pr awaits review from maintainers labels Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split scheme to server and client ones #1550

Split scheme to server and client ones #1550

l4l commented Jul 9, 2018 •

edited

Loading

nickaleks Jul 10, 2018

nickaleks Jul 10, 2018

nickaleks Jul 10, 2018

nickaleks Jul 10, 2018

nickaleks Jul 10, 2018

l4l Jul 10, 2018

nickaleks Jul 10, 2018

nickaleks Jul 10, 2018

l4l Jul 10, 2018

lebdron Jul 10, 2018

lebdron Jul 10, 2018

sorabot commented Jul 10, 2018

vdrobnyi left a comment

l4l commented Jul 11, 2018

Split scheme to server and client ones #1550

Split scheme to server and client ones #1550

Conversation

l4l commented Jul 9, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

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

sorabot commented Jul 10, 2018

vdrobnyi left a comment

Choose a reason for hiding this comment

l4l commented Jul 11, 2018

l4l commented Jul 9, 2018 •

edited

Loading