[native] use build configuration to set socket addr
ClosedPublic
Actions

Authored by varun on Dec 4 2023, 10:05 PM.

Details

Reviewers

bartek
michal

Commits

rCOMM6eb6d5a221b5: [native] use build configuration to set socket addr

Summary

Depends on D10186

Set the default socket address depending on the build configuration. We use staging for debug builds and prod for release builds. This mirrors the defaults on keyserver.

Test Plan

the newly added unit test passed

after building with and without a JSON file present, and with/without --release, i made sure the correct IDENTITY_SOCKET_ADDR value was present

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Dec 4 2023, 10:05 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 4 2023, 10:05 PM

the next diff will let users override the default socket addr with config from a json file. will test those changes on both android and iOS

Harbormaster completed remote builds in B24782: Diff 34245.Dec 4 2023, 10:33 PM

varun requested review of this revision.Dec 4 2023, 10:33 PM

varun added a child revision: D10201: b[native] configure socket address on native with json file.Dec 5 2023, 8:39 PM

bartek accepted this revision.Dec 6 2023, 2:34 AM

bartek added a reviewer: michal.

bartek added inline comments.

native/native_rust_library/src/constants.rs
9 ↗	(On Diff #34245)	I assume we're going to have more services here in the future

This revision is now accepted and ready to land.Dec 6 2023, 2:34 AM

varun added inline comments.Dec 6 2023, 8:00 AM

native/native_rust_library/src/constants.rs
9 ↗	(On Diff #34245)	good point

make socket address configurable with json file

varun added inline comments.Dec 6 2023, 8:24 PM

native/native_rust_library/build.rs
20 ↗	(On Diff #34361)	we probably shouldn't have `println` statements in `build.rs` since they're passed to `cargo` as instructions...

re-requesting review since this has changed so much

Harbormaster completed remote builds in B24839: Diff 34361.Dec 6 2023, 8:39 PM

I will be extending this to handle backup service in the near future, do you think we should have a separate configuration json file per service?

native/native_rust_library/build.rs
20 ↗	(On Diff #34361)	(if it's not starting with `cargo:` then that should be fine link)

bartek accepted this revision.Dec 7 2023, 4:01 AM

This revision is now accepted and ready to land.Dec 7 2023, 4:01 AM

In D10187#296729, @michal wrote:

I will be extending this to handle backup service in the near future, do you think we should have a separate configuration json file per service?

yeah i think we should have separate config files for each service. we can write all the consts to the same rust file using the pattern in build.rs, though