Page MenuHomePhabricator
Differential D4624

[blob] Use `FetchContent_Declare` to pull in `glog`
AbandonedPublic
Actions

Authored by atul on Jul 25 2022, 11:21 AM.
Tags
None
Referenced Files
F3529770: D4624.diff
Tue, Dec 24, 9:13 PM
Unknown Object (File)
Mon, Dec 16, 12:44 AM
Unknown Object (File)
Sun, Dec 15, 4:51 PM
Unknown Object (File)
Sun, Dec 15, 4:51 PM
Unknown Object (File)
Sun, Dec 15, 4:51 PM
Unknown Object (File)
Sun, Dec 15, 4:00 PM
Unknown Object (File)
Sun, Dec 15, 4:00 PM
Unknown Object (File)
Sun, Dec 15, 3:58 PM
View All Files
Subscribers
• adrian
• jon

Details

Reviewers
• abosh
varun
ashoat
tomek
• karol
• jon
Summary

Right now we handle dependencies for services in a very weird way that depends on various shell scripts and assumes a Docker environment built atop the base-image.

This diff uses the CMake FetchContent_Declare functionality to pull in the correct version of glog (as described in services/base-image/docker/install_glog.sh) without depending on a shell script/base image.

In my opinion it makes way more sense for each service to handle required dependencies rather than assuming there's some "base image" that magically takes care of things.

The coupling of services builds with assumption of Docker environment leads to CMake projects that aren't portable... making it impossible to get things to work directly on dev machines. IMO the right level of abstraction for handling dependencies is the build system (in this case CMake).

Test Plan

Blob CI continues to work.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Jul 25 2022, 11:21 AM
Herald added a subscriber: • adrian. · View Herald TranscriptJul 25 2022, 11:21 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 25 2022, 11:23 AM
Harbormaster failed remote builds in B10819: Diff 14882!
atul updated this revision to Diff 14883.Jul 25 2022, 11:24 AM

remove extraneous include

Harbormaster completed remote builds in B10820: Diff 14883.Jul 25 2022, 11:36 AM
atul requested review of this revision.Jul 25 2022, 11:36 AM
atul added a child revision: D4625: [blob] Use `FetchContent_Declare` to pull in `gRPC`.Jul 25 2022, 11:42 AM
ashoat added a reviewer: • jon. ashoat added 1 blocking reviewer(s): • karol.Jul 25 2022, 11:50 AM
ashoat added a subscriber: • jon.

@jon and @karol should definitely take a look. @jon should be reviewing all CMake diffs and @karol wrote the blob service

• jon added a comment.Jul 25 2022, 9:07 PM

This also overlaps with: https://phab.comm.dev/D4490

services/blob/CMakeLists.txt
24

I would much rather attempt to find the package through find_package then fall back to this logic

• karol requested changes to this revision.Jul 26 2022, 4:05 AM

How about tunnelbroker ad backup? I think we should have a consistent way of installing this in every service.
Separately, if glog's used in all of them, why don't we install it in the base image and just use the installed version in the child images instead force the children to install/pull it (which essentially leads to wasting time when building child images for the first time)?

This revision now requires changes to proceed.Jul 26 2022, 4:05 AM
atul added a comment.Jul 26 2022, 7:55 AM
In D4624#132971, @karol wrote:

How about tunnelbroker ad backup? I think we should have a consistent way of installing this in every service.

I agree, was going to just start with blob service to narrow the scope initially and get one service to be "portable." Hopefully after getting it to work for one service the rest would be trivial.

I could update this diff to use the new approach for all services, and then remove the install_glog.sh script altogether?

Separately, if glog's used in all of them, why don't we install it in the base image and just use the installed version in the child images instead force the children to install/pull it (which essentially leads to wasting time when building child images for the first time)?

We can do both! We can install it in the base image so we can benefit from caching there and avoid having to fetch/build dependencies for every child image for the Docker use case.

We can also conditionally FetchContent so we can get the child services to run in non-Docker contexts (eg macOS/other distros/etc)

atul abandoned this revision.Jul 26 2022, 2:08 PM

Revision Contents
Changeset List

PathSize
services/
blob/
11 lines

Diff 14883

View Options

services/blob/CMakeLists.txt

Loading...