Details

Reviewers

• jon
varun
marcin
tomek

Commits

rCOMM7c08edf8507f: [services] Tunnelbroker - Changes to build the Tunnelbroker using `cargo` in a…

Summary

This diff introduces changes to the Tunnelbroker's Dockerfile to change the build system from cmake to use cargo instead.
Also, deprecated cmake-related commands were removed from the Dockerfile.

This diff will also fix the CI (Docker) workflow fails for Tunnelbroker which are related to the Tunnelbroker's transition to Rust.

Related Linear task: ENG-2063

CI Notice:
The CI will fail on this diff on the Tunnelbroker Unit Tests (Nix) until the D5461 where all the changes to CI build commands are made.
This stack will be landed all in one to prevent CI from failing on diffs out of this stack.

Test Plan

The Tunnelbroker Docker CI build should pass successfully.

To test it manually:

Go to the Tunnelbroker service directory services/tunnelbroker and run the Docker compose build command docker compose build tunnelbroker-server.
The expected result is that the Docker image with the Tunnelbroker service is successfully built.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Oct 21 2022, 11:51 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 21 2022, 11:51 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

Harbormaster failed remote builds in B12976: Diff 17825!Oct 21 2022, 11:51 AM

• max edited the summary of this revision. (Show Details)Oct 21 2022, 11:55 AM

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

• max added reviewers: varun, marcin. • max added 1 blocking reviewer(s): • jon.

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

• max added inline comments.Oct 21 2022, 12:01 PM

services/docker-compose.yml
23 ↗	(On Diff #17825)	We are not using `tunnelbroker-dev.ini` anymore, this is a small fix for the config name to use `tunnelbroker-sandbox.ini` instead.
services/tunnelbroker/Dockerfile
5 ↗	(On Diff #17825)	Fixing the folders nesting to make it nested identically to the repo: `services/tunnelbroker`. This is necessary for future proto files compilation and including.
37–39 ↗	(On Diff #17825)	We are running `cargo build` and `cargo run` or `cargo test` now instead of cmake build commands.

• max published this revision for review.Oct 21 2022, 12:01 PM

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

• max added a parent revision: D5436: [services] Tunnelbroker - Creating Cargo config file for Tunnelbroker as a Rust app.

• max added a child revision: D5461: [services] Tunnelbroker - Changing Buildkite commands for Tunnelbroker to run `cargo`.Oct 21 2022, 3:12 PM

• max planned changes to this revision.Oct 24 2022, 3:41 AM

Reducing the file size by squashing commands.
Adding of annotations.
Adding of the building and running of the gTest for the legacy C++ codebase.

• max edited the summary of this revision. (Show Details)Oct 24 2022, 9:38 AM

Harbormaster failed remote builds in B13010: Diff 17869!Oct 24 2022, 9:39 AM

• max edited the summary of this revision. (Show Details)Oct 24 2022, 9:44 AM

• max mentioned this in D5428: [services] Tunnelbroker - Moving directories according to the Rust app structure.Oct 24 2022, 9:52 AM

• jon requested changes to this revision.Oct 26 2022, 2:06 PM

• jon added inline comments.

services/tunnelbroker/Dockerfile
14–23 ↗	(On Diff #17869)	it will be pretty common to have changes in the tunnelbroker directory, I would prefer to have it occur later so that we can cache the installation of the dependencies better

This revision now requires changes to proceed.Oct 26 2022, 2:06 PM

Changing the SDK install and sources copying commands order.

services/tunnelbroker/Dockerfile
14–23 ↗	(On Diff #17869)	it will be pretty common to have changes in the tunnelbroker directory, I would prefer to have it occur later so that we can cache the installation of the dependencies better Such command execution optimization makes sense to me. Thanks, @jon, I've changed the execution order.

Harbormaster failed remote builds in B13129: Diff 18023!Nov 2 2022, 9:09 AM

• max edited the summary of this revision. (Show Details)Nov 2 2022, 9:09 AM

• jon accepted this revision.Nov 2 2022, 11:48 AM

This revision is now accepted and ready to land.Nov 2 2022, 11:48 AM

• max added a reviewer: tomek.Nov 2 2022, 12:47 PM

This revision now requires review to proceed.Nov 2 2022, 12:47 PM

tomek accepted this revision.Nov 3 2022, 2:38 AM

tomek added inline comments.

services/tunnelbroker/Dockerfile
37 ↗	(On Diff #18023)	Shouldn't this be run conditionally based on `COMM_TEST_SERVICES` flag?
43 ↗	(On Diff #18023)	Shouldn't we prefer double brackets?

This revision is now accepted and ready to land.Nov 3 2022, 2:38 AM

ashoat added inline comments.Nov 3 2022, 7:29 AM

services/tunnelbroker/Dockerfile
43 ↗	(On Diff #18023)	Double brackets don't work here I think, as `CMD` isn't being run in `bash` for this image

• max marked 3 inline comments as done.Nov 3 2022, 7:46 AM

• max added inline comments.

services/tunnelbroker/Dockerfile
37 ↗	(On Diff #18023)	Shouldn't this be run conditionally based on `COMM_TEST_SERVICES` flag? I think that makes sense not to build the test app every time. I'll try to add a condition here. Thanks!
43 ↗	(On Diff #18023)	Double brackets don't work here I think, as `CMD` isn't being run in `bash` for this image Yes, that's correct!