Page MenuHomePhabricator
Differential D4535

[services] Tunnelbroker - Add dependencies to Cargo.toml manifest and Cargo.lock files
ClosedPublic
Actions

Authored by • max on Jul 14 2022, 7:27 AM.
Tags
None
Referenced Files
F2902288: D4535.diff
Sat, Oct 5, 4:44 PM
Unknown Object (File)
Sun, Sep 29, 11:43 PM
Unknown Object (File)
Sun, Sep 29, 11:43 PM
Unknown Object (File)
Sun, Sep 29, 11:43 PM
Unknown Object (File)
Sun, Sep 29, 11:43 PM
Unknown Object (File)
Thu, Sep 26, 6:43 PM
Unknown Object (File)
Sep 2 2024, 12:59 PM
Unknown Object (File)
Sep 1 2024, 11:35 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• jon
• karol
tomek

Details

Reviewers
• karol
varun
• jon
Commits
rCOMM6737a9974f12: [services] Tunnelbroker - Add dependencies to Cargo.toml manifest and Cargo.
Summary

This diff adds dependencies to the Rust Cargo manifest file for the notifications library wrapper.

Dependencies list:
tracing - Logging and tracing inside the functions.
tokio - Asynchronous calls.
lazy_static - Lazy static initialization that will be used for the Tokio runtime initialization.
a2 - APNS client library.

An empty services/tunnelbroker/rust-notifications/src/lib.rs file was added to prevent the build failures. This file will contain the Rust library code in follow-up diffs.

The Cargo.lock file is added to lock versions.

Linear task: ENG-1330

Test Plan

Run cargo build from the services/tunnelbroker/rust-notifications directory.

Diff Detail

Repository
rCOMM Comm
Branch
add-cargo-depenencies
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Jul 14 2022, 7:27 AM
• max held this revision as a draft.
Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJul 14 2022, 7:27 AM
• max edited the summary of this revision. (Show Details)Jul 14 2022, 7:30 AM
• max added reviewers: • karol, varun, tomek.
• max edited the summary of this revision. (Show Details)
• max updated this revision to Diff 14492.Jul 14 2022, 7:37 AM

Cargo.lock file was added.

• max added a parent revision: D4534: [services] Tunnelbroker - Add Cargo manifest file.Jul 14 2022, 7:38 AM
Harbormaster completed remote builds in B10528: Diff 14489.Jul 14 2022, 7:39 AM
Harbormaster completed remote builds in B10530: Diff 14492.Jul 14 2022, 7:49 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 published this revision for review.Jul 14 2022, 8:25 AM
• max updated this revision to Diff 14501.Jul 14 2022, 10:43 AM

Description and version number was changed.

varun requested changes to this revision.Jul 14 2022, 10:47 AM
varun added inline comments.
services/tunnelbroker/rust/Cargo.toml
10

I prefer tracing to log for async code

https://github.com/tokio-rs/tracing#in-libraries
https://github.com/tokio-rs/tracing#in-asynchronous-code

This revision now requires changes to proceed.Jul 14 2022, 10:47 AM
• max planned changes to this revision.Jul 14 2022, 10:51 AM
Harbormaster completed remote builds in B10536: Diff 14501.Jul 14 2022, 10:56 AM
• max added inline comments.Jul 14 2022, 11:01 AM
services/tunnelbroker/rust/Cargo.toml
10

I prefer tracing to log for async code

https://github.com/tokio-rs/tracing#in-libraries
https://github.com/tokio-rs/tracing#in-asynchronous-code

I should check it today, thanks!

• jon added a subscriber: • jon.Jul 14 2022, 3:25 PM
• jon added inline comments.
services/tunnelbroker/rust/Cargo.toml
10 ↗(On Diff #14501)

I would prefer to have flexible patch versions, so that bug fixes can be fetched with cargo update. Pinning should be done with a corresponding Cargo.lock

If a package does release regressions between patch versions, then we would probably want to re-evaluate if it's something that we want to include in the long-run.

• max retitled this revision from [services] Tunnelbroker - Add dependencies to Cargo manifest file to [services] Tunnelbroker - Add dependencies to Cargo manifest file and Cargo.lock file.Jul 15 2022, 10:28 AM
• max edited the summary of this revision. (Show Details)
• max edited the test plan for this revision. (Show Details)
• max updated this revision to Diff 14529.Jul 15 2022, 10:30 AM
• max retitled this revision from [services] Tunnelbroker - Add dependencies to Cargo manifest file and Cargo.lock file to [services] Tunnelbroker - Add dependencies to Cargo manifest file.
• max edited the summary of this revision. (Show Details)
• max edited the test plan for this revision. (Show Details)

Use tracing and changed the version numbers to allow patching.

• max edited reviewers, added: • jon; removed: tomek. • max added 1 blocking reviewer(s): varun.Jul 15 2022, 10:30 AM
• max marked 2 inline comments as done.
• max added inline comments.
services/tunnelbroker/rust/Cargo.toml
10 ↗(On Diff #14501)

I would prefer to have flexible patch versions, so that bug fixes can be fetched with cargo update. Pinning should be done with a corresponding Cargo.lock

If a package does release regressions between patch versions, then we would probably want to re-evaluate if it's something that we want to include in the long-run.

That makes sense. I've changed the numbers.

10

I prefer tracing to log for async code

https://github.com/tokio-rs/tracing#in-libraries
https://github.com/tokio-rs/tracing#in-asynchronous-code

I've switched to using tracing instead of log. Thanks for the suggestion!

• max edited the summary of this revision. (Show Details)Jul 15 2022, 10:33 AM
• max edited the test plan for this revision. (Show Details)
• max retitled this revision from [services] Tunnelbroker - Add dependencies to Cargo manifest file to [services] Tunnelbroker - Add dependencies to Cargo.toml manifest and Cargo.lock files.
• max edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B10556: Diff 14529.Jul 15 2022, 10:42 AM
• jon accepted this revision.Jul 15 2022, 11:12 AM

lgtm

varun accepted this revision.Jul 15 2022, 1:11 PM
This revision is now accepted and ready to land.Jul 15 2022, 1:11 PM
atul added inline comments.Jul 15 2022, 1:33 PM
services/tunnelbroker/rust-notifications/Cargo.toml
11 ↗(On Diff #14529)

Could we specify features in a more "fine-tuned" way here to avoid pulling in features we don't end up using?

• max updated this revision to Diff 14580.Jul 18 2022, 11:24 AM
• max marked an inline comment as done.

Changing the Tokio features list from 'full' to 'rt-multi-thread' only.

Harbormaster failed remote builds in B10589: Diff 14580!Jul 18 2022, 11:26 AM
• max marked an inline comment as done.Jul 18 2022, 11:26 AM
• max added inline comments.
services/tunnelbroker/rust-notifications/Cargo.toml
11 ↗(On Diff #14529)

Could we specify features in a more "fine-tuned" way here to avoid pulling in features we don't end up using?

Nice catch!
With the current code, we can use rt-multi-thread instead of the full. I've changed to it.

Harbormaster completed remote builds in B10589: Diff 14580.Jul 18 2022, 2:01 PM
atul added a comment.Jul 18 2022, 2:02 PM

(ESLint & Flow & Jest workflow failed because of some intermittent network error. Restarted the workflow and it looks good to go.)

Closed by commit rCOMM6737a9974f12: [services] Tunnelbroker - Add dependencies to Cargo.toml manifest and Cargo. (authored by • max). · Explain WhyJul 20 2022, 2:58 AM
This revision was automatically updated to reflect the committed changes.
• max marked an inline comment as done.
• max added a commit: rCOMM6737a9974f12: [services] Tunnelbroker - Add dependencies to Cargo.toml manifest and Cargo..
• 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/
4 lines

Diff 14492

View Options

services/tunnelbroker/rust/Cargo.lock

Loading...
View Options

services/tunnelbroker/rust/Cargo.toml

Loading...