Skip to content
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.

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

Add GRPC Rust library target that is based on Tonic #160

Merged
merged 11 commits into from
Sep 1, 2022

Conversation

lukas-slezevicius
Copy link
Contributor

@lukas-slezevicius lukas-slezevicius commented Sep 1, 2022

What is the goal of this PR?

Add //grpc/rust:typedb-protocol target that is a rust_library that contains TypeDB Protocol's automatically generated protobuf code. The library used for this is tonic, which we are planning to use instead of grpc as it's more actively developed, popular, and mature.

What are the changes implemented in this PR?

  • Create filegroup targets for each directory that contains protobuf
  • Update vaticle_dependencies to include tonic and related crates
  • Add the targets required for creating the rust_library
  • Add lib.rs to include the generated code
  • Add build.rs to compile protobuf during Rust's compilation stage

@grabl
Copy link
Contributor

grabl commented Sep 1, 2022

PR Review Checklist

Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.


Code

  • Packages, classes, and methods have a single domain of responsibility.
  • Packages, classes, and methods are grouped into cohesive and consistent domain model.
  • The code is canonical and the minimum required to achieve the goal.
  • Modules, libraries, and APIs are easy to use, robust (foolproof and not errorprone), and tested.
  • Logic and naming has clear narrative that communicates the accurate intent and responsibility of each module (e.g. method, class, etc.).
  • The code is algorithmically efficient and scalable for the whole application.

Architecture

  • Any required refactoring is completed, and the architecture does not introduce technical debt incidentally.
  • Any required build and release automations are updated and/or implemented.
  • Any new components follows a consistent style with respect to the pre-existing codebase.
  • The architecture intuitively reflects the application domain, and is easy to understand.
  • The architecture has a well-defined hierarchy of encapsulated components.
  • The architecture is extensible and scalable.

"@com_google_protobuf//:protoc",
],
build_script_env = {
"PROTOC": "$(execpath @com_google_protobuf//:protoc)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uses protoc provided by bazel.

grpc/rust/BUILD Outdated
data = [
"//cluster:proto-files",
"//common:proto-files",
"//core:proto-files",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the paths of these targets through rootpath did not work, so I left it as it is.

cluster/BUILD Outdated
@@ -50,3 +50,8 @@ checkstyle_test(
license_type = "agpl-header",
size = "small",
)

filegroup(
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we would like to avoid having to create these filegroups as the proto files are already represented by the proto_library targets in these packages.

We have already a working build script using the grpc crate, grpc/rust/main.rs, that depends on the proto_library targets. Can we refactor grpc/rust:build_script to do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented the changes.

// along with this program. If not, see <https://www.gnu.org/licenses/>.
//

tonic::include_proto!("typedb.protocol");
Copy link
Member

Choose a reason for hiding this comment

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

Why do we use a one-line file here? Is there some way we can merge lib.rs and build.rs - or is there a specific reason why having both is useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rust_library requires at least a single source file that would contain the library's code. The one-line macro simply includes all the code generated through protoc. This way, you can just import the rust_library from your code directly.

build.rs is the build file that runs before compiling the rust_library.

You can't join the files, as the build script runs before compiling the library, and the library relies on the output of the build script.

grpc/rust/BUILD Outdated
)

rust_library(
name = "lib",
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename this to typedb-protocol, for consistency with Java, Python and Node.js.

This is in line with our other library repos, where the "entry point" - i.e the principal export of the repo - is named the same as the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@haikalpribadi haikalpribadi left a comment

Choose a reason for hiding this comment

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

1 change to make before merging.

grpc/rust/BUILD Outdated
@@ -85,3 +86,44 @@ checkstyle_test(
license_type = "agpl-header",
size = "small",
)

cargo_build_script(
name = "build_script",
Copy link
Member

Choose a reason for hiding this comment

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

Name this target properly, it should be typedb-protocol-src because that is what this target produces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants