Needed for backup and blob service
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
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?
Comment Actions
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.
Comment Actions
Looks good, appreciate the rebasing and reordering to appease the CI.
| services/lib/src/client-base-reactors/CMakeLists.txt | ||
|---|---|---|
| 2 ↗ | (On Diff #15289) | The minimum CMake versions we're specifying vary pretty significantly throughout the codebase. It looks like the highest in the codebase is 3.16. If we're not limited by some external factor (Android requirement/some dependency), could we try pushing these up? (Personally think it'd be good to aim for >=3.14 since that's the version that introduces FetchContent_MakeAvailable, etc.) |
| 40 ↗ | (On Diff #15289) | Not sure we really need this comment, but if it's consistent with the comments in the other CMakeLists.txt files it's probably fine |
| services/lib/src/client-base-reactors/CMakeLists.txt | ||
|---|---|---|
| 2 ↗ | (On Diff #15289) | I guess this isn't used by android, so that's fine. But I think we can bump it at a later time as needed. |
| 40 ↗ | (On Diff #15289) | Yea, I'm not really sure "what needs to be commented vs what doesn't" for cmake. So I think I just commented a lot early on, and have been copying around the files as templates. |
| services/lib/src/client-base-reactors/CMakeLists.txt | ||
|---|---|---|
| 2 ↗ | (On Diff #15289) | Can we get a task for this? (Any time somebody asks you to do something in a diff and you want to defer it to later, you should always create a task to track) |
| services/lib/src/client-base-reactors/CMakeLists.txt | ||
|---|---|---|
| 2 ↗ | (On Diff #15289) | Yes, but in this case client-base-reactors isn't consuming any external dependencies, so the use case for FetchContent_MakeAvailable isn't present in this diff. It would be more for the related diffs which use find_library(). I can still add a task though https://linear.app/comm/issue/ENG-1548/bump-minimum-cmake-version-in-cmake-projects Also, FetchContent_MakeAvailable is only really useful in a non-nix-dev workflow. The nix environment should be exposing everything needed. |