Page MenuHomePhabricator

[Services] Export shared services cpp as CMake Project
ClosedPublic

Authored by jon on Jul 8 2022, 10:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jun 26, 11:21 AM
Unknown Object (File)
Wed, Jun 26, 7:50 AM
Unknown Object (File)
Wed, Jun 26, 5:47 AM
Unknown Object (File)
Wed, Jun 26, 5:47 AM
Unknown Object (File)
Wed, Jun 26, 5:47 AM
Unknown Object (File)
Wed, Jun 26, 5:47 AM
Unknown Object (File)
Wed, Jun 26, 5:47 AM
Unknown Object (File)
Wed, Jun 26, 5:47 AM

Details

Summary

Needed for tunnelbroker, blob, backup

Decided to move DatabaseEntitiesTools.h into this directory, since backup, blob, and tunnelbroker all made use of it with only small differences in which headers were being imported. This file needed to be moved because using the shared library as a target caused conflicts as to what #include "DatabaseEntitiesTools.h" did for each project. However, moving this file causes all 3 services to "take notice". Tunnelbroker is just including the code as if it's own, but is fixed to do the idiomatic thing in D4757

Depends on D4488

Backup docker and cmake logic is updated to support new build paradigm in D4494
Blob docker and cmake logic is updated to support new build paradigm in D4493

Test Plan
nix develop
cd services/lib/src && mkdir build && cd build && cmake .. && make

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 8 2022, 10:27 PM
Harbormaster failed remote builds in B10430: Diff 14374!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 25 2022, 9:03 PM
Harbormaster failed remote builds in B10840: Diff 14903!

Import protobuf first, to avoid grpc not being able to find protobuf

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 29 2022, 9:52 AM
Harbormaster failed remote builds in B10978: Diff 15106!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 29 2022, 3:04 PM
Harbormaster failed remote builds in B10984: Diff 15118!

Prune duplicated DatabaseEntitiesTools.h

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 1 2022, 1:02 PM
Harbormaster failed remote builds in B11028: Diff 15169!
Harbormaster returned this revision to the author for changes because remote builds failed.Aug 1 2022, 1:23 PM
Harbormaster failed remote builds in B11032: Diff 15173!
jon added inline comments.
services/lib/src/DatabaseEntitiesTools.h
6 ↗(On Diff #15173)

This is a fix for GCC. Shouldn't have any effect on clang.

Just use variable for constructing path

jon added a reviewer: atul. jon added 2 blocking reviewer(s): karol, max.Aug 3 2022, 8:31 AM

Any time you're asking somebody to review a diff despite CI failing, you should explain why

atul requested changes to this revision.Aug 3 2022, 10:42 AM

I'm running into the following when I try to run nix develop after patching in this diff:

atul@atuls-MacBook-Pro comm % nix develop
error: builder for '/nix/store/mi2jwqcaqlpcd2sw2ljvw3sk4h7hjy1m-arcanist-20220517.drv' failed with exit code 2;
       last 10 log lines:
       > configuring
       > no configure script, doing nothing
       > building
       > no Makefile, doing nothing
       > installing
       > patching script interpreter paths in /nix/store/rms7xx6z22qys43qhg4pvsvlj2l3fsnn-arcanist-20220517/libexec/arcanist/bin/arc
       > /nix/store/rms7xx6z22qys43qhg4pvsvlj2l3fsnn-arcanist-20220517/libexec/arcanist/bin/arc: interpreter directive changed from "#!/usr/bin/env php" to "/nix/store/p3my8h8q02429pfgh54b2knwckymxcph-php-with-extensions-8.0.21/bin/php"
       > /nix/store/rms7xx6z22qys43qhg4pvsvlj2l3fsnn-arcanist-20220517/libexec/arcanist/bin/arc: line 2: ?php: No such file or directory
       > /nix/store/rms7xx6z22qys43qhg4pvsvlj2l3fsnn-arcanist-20220517/libexec/arcanist/bin/arc: line 4: syntax error near unexpected token `'pcntl_async_signals''
       > /nix/store/rms7xx6z22qys43qhg4pvsvlj2l3fsnn-arcanist-20220517/libexec/arcanist/bin/arc: line 4: `if (function_exists('pcntl_async_signals')) {'
       For full logs, run 'nix log /nix/store/mi2jwqcaqlpcd2sw2ljvw3sk4h7hjy1m-arcanist-20220517.drv'.
error: 1 dependencies of derivation '/nix/store/3xpdf6mxb2bi8mxqd47j9c55yp0zm43a-nix-shell-env.drv' failed to build

Note that nix develop works fine on master

This revision now requires changes to proceed.Aug 3 2022, 10:42 AM

Any time you're asking somebody to review a diff despite CI failing, you should explain why

Yea, currently have a chicken and an egg problem between docker and cmake being happy across 3 projects. Bunching all of the related changes into a single PR would have been a massive diff.

But I'll refactor to make CI more happy.

Looks like I would need to separate additional header inclusions to fix the builds for all diffs. I can do that in the morning. Or we can keep the existing per-project changes.

atul requested changes to this revision.EditedAug 4 2022, 11:25 AM

Getting the following when I follow the Test Plan locally.

atuls-MacBook-Pro:comm atul$ cd services/lib/src && mkdir build && cd build && cmake .. && make
-- The C compiler identification is AppleClang 13.1.6.13160021
-- The CXX compiler identification is AppleClang 13.1.6.13160021
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found AWS SDK for C++, Version: 1.9.238, Install Root:, Platform Prefix:, Platform Dependent Libraries: pthread;curl
-- Components specified for AWSSDK: core;dynamodb, application will be depending on libs: aws-cpp-sdk-dynamodb;aws-cpp-sdk-core
-- Try finding aws-cpp-sdk-core
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Found aws-cpp-sdk-core
-- Try finding aws-cpp-sdk-dynamodb
-- Found aws-cpp-sdk-dynamodb
CMake Warning (dev) at /nix/store/cg3f9hs6zdag48yi8zdpg3kwq9ffcpbj-cmake-3.23.2/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:438 (message):
  The package name passed to `find_package_handle_standard_args` (Protobuf)
  does not match the name of the calling package (protobuf).  This can lead
  to problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  /nix/store/cg3f9hs6zdag48yi8zdpg3kwq9ffcpbj-cmake-3.23.2/share/cmake-3.23/Modules/Findprotobuf.cmake:650 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:24 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found Protobuf: /nix/store/r5nv2l4hiraggzawpjhxhbqgnc93p0a9-protobuf-3.15.8/lib/libprotobuf.dylib (found version "3.15.8") 
-- Found ZLIB: /nix/store/lkgrb3lfingdcxr9fzc847c5wihk1w73-zlib-1.2.12/lib/libz.dylib (found version "1.2.12") 
-- Found OpenSSL: /nix/store/1yhkn4rmhxpxdpvf10m10x5z60sxwhkr-openssl-1.1.1q/lib/libcrypto.dylib (found version "1.1.1q")  
-- Found c-ares: /Users/atul/.local/lib/cmake/c-ares/c-ares-config.cmake (found version "1.14.0") 
-- Found RE2 via CMake.
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/atul/comm/services/lib/src/build
[ 25%] Building CXX object CMakeFiles/comm-services-common.dir/DatabaseManagerBase.cpp.o
In file included from /Users/atul/comm/services/lib/src/DatabaseManagerBase.cpp:1:
In file included from /Users/atul/comm/services/lib/src/DatabaseManagerBase.h:3:
In file included from /Users/atul/comm/services/lib/src/DatabaseEntitiesTools.h:3:
/Users/atul/comm/services/lib/src/Item.h:22:22: error: no member named 'make_unique' in namespace 'std'
        sortKey(std::make_unique<std::string>(sortKey)) {
                ~~~~~^
/Users/atul/comm/services/lib/src/Item.h:22:45: error: expected '(' for function-style cast or type construction
        sortKey(std::make_unique<std::string>(sortKey)) {
                                 ~~~~~~~~~~~^
/Users/atul/comm/services/lib/src/DatabaseManagerBase.cpp:9:10: fatal error: 'glog/logging.h' file not found
#include <glog/logging.h>
         ^~~~~~~~~~~~~~~~
3 errors generated.
make[2]: *** [CMakeFiles/comm-services-common.dir/DatabaseManagerBase.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-services-common.dir/all] Error 2
make: *** [all] Error 2

In D4489#136244, @jon wrote:

Looks like I would need to separate additional header inclusions to fix the builds for all diffs. I can do that in the morning. Or we can keep the existing per-project changes.

Would this be quick, or would it be time consuming?

In an ideal world every diff would pass CI, but there have been cases in the past where we decided the effort wouldn't be worth it. As long as all the changes are landed atomically it should be fine.

Feel free to re-request review if you think this diff should still be reviewed despite CI not passing. (Looks like D4494 is passing CI, but D4493 still isn't... I'd be comfortable accepting this if both of those were all green)

This revision now requires changes to proceed.Aug 4 2022, 11:25 AM

As an aside could we add all of these build folders (eg services/lib/src/build/) to .gitignore?

We can probably do this for all build folders in one go in a separate diff at the end of this stack?

As an aside could we add all of these build folders (eg services/lib/src/build/) to .gitignore?

I did an initial pass in https://phab.comm.dev/D4487, but could be updated

We can probably do this for all build folders in one go in a separate diff at the end of this stack?

Sounds good

In D4489#136169, @jon wrote:

Any time you're asking somebody to review a diff despite CI failing, you should explain why

Yea, currently have a chicken and an egg problem between docker and cmake being happy across 3 projects. Bunching all of the related changes into a single PR would have been a massive diff.

But I'll refactor to make CI more happy.

The feedback here isn't that you shouldn't be submitting diffs that fail CI. Of course that's generally a good idea, but the feedback here is that if and when you find yourself submitting a diff that is failing CI, and you ask somebody to review it anyways, you should explain on the diff (thoroughly) why you are asking somebody to review despite CI failing.

Fix glog and boost not being found. Also set CXX_STANDARD to 14 for clang.

@atul I added Clang/MacOS support for this project in https://phab.comm.dev/D4761 and rebased this diff on top of the changes. Please try again.

Worked!

[100%] Built target comm-services-common
This revision is now accepted and ready to land.Aug 8 2022, 11:29 AM
This revision was landed with ongoing or failed builds.Aug 8 2022, 1:45 PM
This revision was automatically updated to reflect the committed changes.