Paths

Table of Contentst

[services] Tunnelbroker - Add Cargo manifest file
ClosedPublic
Actions

Authored by • max on Jul 14 2022, 7:17 AM.

Details

Reviewers

• karol
varun
• jon

Commits

rCOMMdec4d8d7c770: [services] Tunnelbroker - Add Cargo manifest file

Summary

This diff introduces a Rust Cargo manifest file for the notification library wrapper in Rust for using it from C++ using corrosion tool.

Linear task: ENG-1330

Test Plan

No tests, this is a manifest-only file. This file will be updated with the dependencies and built using cargo build in a follow-up diff of the stack.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Jul 14 2022, 7:17 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJul 14 2022, 7:17 AM

• max published this revision for review.Jul 14 2022, 7:22 AM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: • karol, varun, tomek.

Harbormaster completed remote builds in B10527: Diff 14488.Jul 14 2022, 7:30 AM

• max added a child revision: D4535: [services] Tunnelbroker - Add dependencies to Cargo.toml manifest and Cargo.lock files.Jul 14 2022, 7:38 AM

can we make the directory services/tunnelbroker/rust-notifications? the convention in rust is for the directory name to match the package name

services/tunnelbroker/rust/Cargo.toml
3 ↗	(On Diff #14488)	small nit
5 ↗	(On Diff #14488)	typically the last number is reserved for patches

This revision now requires changes to proceed.Jul 14 2022, 7:52 AM

• max added a child revision: D4537: [services] Tunnelbroker - Add Cargo manifest, headers and Rust linking to the CMakeLists.Jul 14 2022, 8:02 AM

• max added a child revision: D4540: [services] Tunnelbroker - Rust APNS push notifications A2 library wrapper.Jul 14 2022, 8:19 AM

In D4534#129014, @varun wrote:

can we make the directory services/tunnelbroker/rust-notifications? the convention in rust is for the directory name to match the package name

In the case of Rust to C++ integration, we need a "wrapper" for it, that's why our Rust notification library is a kind of adapter between the C and Rust packages functions. To use any other packages we need to include this adapter library. That's why even if we will add another package to use from Rust we should still import only one Rust adapter library to call Rust API from C++. That's why I think it's better to use something common like rust or rust-lib...

services/tunnelbroker/rust/Cargo.toml
3 ↗	(On Diff #14488)	small nit Thx!
5 ↗	(On Diff #14488)	typically the last number is reserved for patches As a pre-release (Alpha) I used 0 versions, but I don't mind we are switching to the first ;)

• max planned changes to this revision.Jul 14 2022, 10:51 AM

• max marked 2 inline comments as done.

In D4534#129137, @geekbrother wrote:

In D4534#129014, @varun wrote:

can we make the directory services/tunnelbroker/rust-notifications? the convention in rust is for the directory name to match the package name

In the case of Rust to C++ integration, we need a "wrapper" for it, that's why our Rust notification library is a kind of adapter between the C and Rust packages functions. To use any other packages we need to include this adapter library. That's why even if we will add another package to use from Rust we should still import only one Rust adapter library to call Rust API from C++. That's why I think it's better to use something common like rust or rust-lib...

ok, then all of the code pertaining to the rust-notifications package should be in a subdirectory services/tunnelbroker/rust/rust-notifications

Description and version number was changed.

back to your queue to address most recent comment

This revision now requires changes to proceed.Jul 14 2022, 10:56 AM

varun added inline comments.Jul 14 2022, 10:58 AM

services/tunnelbroker/rust/Cargo.toml
3 ↗	(On Diff #14488)	also i don't think this was fixed in the most recent revision

In D4534#129150, @varun wrote:

In D4534#129137, @geekbrother wrote:

In D4534#129014, @varun wrote:

can we make the directory services/tunnelbroker/rust-notifications? the convention in rust is for the directory name to match the package name

In the case of Rust to C++ integration, we need a "wrapper" for it, that's why our Rust notification library is a kind of adapter between the C and Rust packages functions. To use any other packages we need to include this adapter library. That's why even if we will add another package to use from Rust we should still import only one Rust adapter library to call Rust API from C++. That's why I think it's better to use something common like rust or rust-lib...

ok, then all of the code pertaining to the rust-notifications package should be in a subdirectory services/tunnelbroker/rust/rust-notifications

We would have only one adapter Rust library and will have an empty folder with a subfolder inside in this case. It doesn't look good to me.
services/tunnelbroker/rust-notifications is much better compared to an empty nesting...

I don't mind changing it to the services/tunnelbroker/rust-notifications if it looks much more organic way for you.

• max marked an inline comment as done.Jul 14 2022, 11:08 AM

• max added inline comments.

services/tunnelbroker/rust/Cargo.toml
3 ↗	(On Diff #14488)	also i don't think this was fixed in the most recent revision Seems we had a "race condition" )) It was fixed on a recent update. Thanks!

Harbormaster completed remote builds in B10537: Diff 14502.Jul 14 2022, 11:08 AM

• max planned changes to this revision.Jul 14 2022, 11:09 AM

• max marked an inline comment as done.

• jon accepted this revision.Jul 14 2022, 3:19 PM

The Rust directory was changed to services/tunnelbroker/rust-notifications from services/tunnelbroker/rust.
.gitignore file is updating on the updated Rust directory for the Tunnelbroker.

• max removed a reviewer: tomek. • max added 1 blocking reviewer(s): varun.Jul 15 2022, 8:39 AM

Harbormaster completed remote builds in B10555: Diff 14527.Jul 15 2022, 8:51 AM

varun accepted this revision.Jul 15 2022, 10:12 AM

This revision is now accepted and ready to land.Jul 15 2022, 10:12 AM

Closed by commit rCOMMdec4d8d7c770: [services] Tunnelbroker - Add Cargo manifest file (authored by • max). · Explain WhyJul 15 2022, 10:16 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMMdec4d8d7c770: [services] Tunnelbroker - Add Cargo manifest file.

ashoat added inline comments.Jul 17 2022, 7:28 AM

services/tunnelbroker/rust-notifications/Cargo.toml
6	Seems like we should use the same license here as do in the mainline repo

• max marked an inline comment as done.Jul 18 2022, 5:22 AM

• max added inline comments.

services/tunnelbroker/rust-notifications/Cargo.toml
6	Seems like we should use the same license here as do in the mainline repo Seems it is. I've put D4552 with the license changes in all `Cargo.toml` files in the project.