Page MenuHomePhabricator
Differential D4534

[services] Tunnelbroker - Add Cargo manifest file
ClosedPublic
Actions

Authored by • max on Jul 14 2022, 7:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 1:58 AM
Unknown Object (File)
Thu, Dec 19, 1:58 AM
Unknown Object (File)
Sat, Dec 7, 4:25 PM
Unknown Object (File)
Nov 8 2024, 7:59 PM
Unknown Object (File)
Nov 2 2024, 6:20 PM
Unknown Object (File)
Oct 7 2024, 11:04 AM
Unknown Object (File)
Oct 7 2024, 11:04 AM
Unknown Object (File)
Oct 7 2024, 11:04 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• karol
tomek

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
Branch
add-tunnelbroker-cargo
Lint
No Lint Coverage
Unit
No Test Coverage

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
varun requested changes to this revision.Jul 14 2022, 7:52 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
• max marked 2 inline comments as done.Jul 14 2022, 10:50 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.
varun added a comment.Jul 14 2022, 10:56 AM
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

• max updated this revision to Diff 14502.Jul 14 2022, 10:56 AM

Description and version number was changed.

varun requested changes to this revision.Jul 14 2022, 10:56 AM

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

• max added a comment.Jul 14 2022, 11:06 AM
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
• max updated this revision to Diff 14527.Jul 15 2022, 8:38 AM

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 ↗(On Diff #14528)

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 ↗(On Diff #14528)

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.

• max removed a child revision: D4540: [services] Tunnelbroker - Rust APNS push notifications A2 library wrapper.Jul 20 2022, 3:31 AM

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
rust/
12 lines

Diff 14502

View Options

services/tunnelbroker/rust/Cargo.toml

Loading...