Page MenuHomePhabricator

[Services] Export server-base-reactors as CMake Project
ClosedPublic

Authored by jon on Jul 8 2022, 10:51 PM.
Tags
None
Referenced Files
F3521432: D4492.id15368.diff
Mon, Dec 23, 3:31 AM
F3521431: D4492.id15172.diff
Mon, Dec 23, 3:31 AM
F3518653: D4492.id15433.diff
Sun, Dec 22, 8:29 PM
Unknown Object (File)
Thu, Dec 19, 1:56 AM
Unknown Object (File)
Tue, Dec 17, 7:02 AM
Unknown Object (File)
Mon, Dec 16, 10:06 AM
Unknown Object (File)
Mon, Dec 16, 12:18 AM
Unknown Object (File)
Mon, Dec 16, 12:12 AM

Details

Summary

Necessary for backup service

Depends on D4491

Test Plan

Run test plan from https://phab.comm.dev/D4494

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 8 2022, 10:53 PM
Harbormaster failed remote builds in B10433: Diff 14377!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 25 2022, 9:05 PM
Harbormaster failed remote builds in B10842: Diff 14905!

Update with fixes for docker environment

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 29 2022, 9:52 AM
Harbormaster failed remote builds in B10977: Diff 15105!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 29 2022, 3:06 PM
Harbormaster failed remote builds in B10987: Diff 15121!
Harbormaster returned this revision to the author for changes because remote builds failed.Aug 1 2022, 1:17 PM
Harbormaster failed remote builds in B11031: Diff 15172!
jon added a reviewer: atul.

Copied from D4491:


At a high level this looks good but

A. It looks like some of the CI workflows are failing, could you provide some context if that's to be expected?
B. There isn't a Test Plan so it's hard to verify that this works as expected. It says "used by later diff," could you specify which diff that is so I can follow the Test Plan for that?

atul requested changes to this revision.Aug 3 2022, 5:11 PM
This revision now requires changes to proceed.Aug 3 2022, 5:11 PM

Yes, it's meant to fail right now, I could have rebased to be before the tunnelbroker changes, but it's hard to order this work in some way. I chose to do batch them logically "as needed by later changes".

I updated the test plan.

I eventually fix the gates again, but I had to change both backup and blob to do so. I tried to make the individual diffs as small as possible.

Looks good, appreciate the rebasing and reordering to appease the CI.

services/lib/src/server-base-reactors/CMakeLists.txt
2 ↗(On Diff #15290)

We got a different minimum version here as well, can we try to push that up as high as we can without running into issues (with Android NDK or whatever)?

38 ↗(On Diff #15290)

Again, not sure this comment is necessary... but feel free to leave it in if it's consistent with all the other CMakeLists.txt files.

This revision is now accepted and ready to land.Aug 4 2022, 11:51 AM

Remove installation comment, set cmake_minimum to 3.10

jon added inline comments.
services/lib/src/server-base-reactors/CMakeLists.txt
2 ↗(On Diff #15290)

Having a higher minimum than necessary is also an anti-pattern. Unless we have something which requires it being bumped (e.g. fetchContent_MakeAvailable), then we shouldn't really be preemptively bumping a minimum required.