Replace autotools with CMake + fbcode_builder #449

mszabo-wikia · 2024-07-29T02:27:16Z

Part of the difficulty in building mcrouter as a third party stems from the fact that it uses an autotools-based build system. While other Meta OSS projects have adopted fbcode_builder to make it easier to build a project with all required dependencies, this tool was primarily written with CMake in mind and unfortunately wouldn't work with mcrouter's build system without making mcrouter-specific accommodations (e.g. explicitly passing in the Thrift compiler path and so on).

So, introduce a CMake-based build system for mcrouter and wire it up with fbcode_builder. This also allows for the tests (which had been nonfunctional in the autotools build for years) to be run in OSS CI via CMake/CTest, after some fixes for the GH Actions environment and disabling Meta-specific functionality in an OSS context. There are no changes to non-test/build files apart from the elimination of the generated config.h.

Use globbing extensively for generating source file lists in CMake files. While this may be a controversial approach, I think in this specific case the benefits outweigh the drawbacks, since this project doesn't use CMake internally so globbing can reduce the amount of times the OSS build needs to be fixed after the fact because a new source file wasn't added to it.

To assist reviewing, I've attempted to break this change into reasonable commits. The very first commit simply adds a vendored copy of fbcode_builder from a recent Folly checkout, hence the large diff size. I've not yet removed autotools paraphernalia to reduce the amount of changes in this PR—that can be done as a followup.

mszabo-wikia · 2024-07-29T02:28:32Z

Please let me know if you find this approach feasible—I'm not a CMake expert.

~~For actually applying these changes tofbcode_builder, I assume some internal work will need to be done on the Meta side.~~ (edit: I've submitted an appropriate Folly PR to do this)

mszabo-wikia · 2024-07-29T02:30:18Z

mcrouter/lib/carbon/example/gen/HelloGoodbyeService.thrift

 cpp_include "mcrouter/lib/carbon/example/gen/HelloGoodbyeMessages.h"

 namespace cpp2 hellogoodbye.thrift
 namespace py3 hellogoodbye.thrift

-service HelloGoodbye extends fb303.FacebookService {
+service HelloGoodbye {


Ugly, but installing fb303 via fbcode_builder still doesn't help the Thrift compiler find fb303 IDLs. Since this is a test/example service, perhaps this is okay.

mszabo-wikia · 2024-07-29T02:32:57Z

mcrouter/lib/network/test/TestClientServerUtil.cpp

@@ -87,6 +87,8 @@ void TestServerOnRequest::onRequest(
    processReply(std::move(ctx), Reply(carbon::Result::NOTFOUND));
  } else if (req.key_ref()->fullKey() == "shutdown") {
    shutdownLock_.post();
+    // TODO
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));


I've observed a race condition in several tests in AsyncMcClientTestSync, e.g. qosClass, where the test would hang on shutdown without this, with the test thread waiting on the ->shutdown call, the server thread waiting on ->join and event loop threads still selecting, so I added this as a workaround since I wasn't able to find the exact cause of the race.

Some things I've tried to attempt to resolve this:

reordering the shutdownLock_ post,

introducing another synchronization barrier between the call to shutdown and join on the mock server main thread,

changing the worker loop to use loopOnce over loop and check isShutdown on each loop iteration, like some of the other mock servers do.

mszabo-wikia · 2024-07-29T02:35:33Z

mcrouter/test/MCProcess.py

@@ -256,7 +255,7 @@ def ensure_connected(self):
            while True:
                try:
                    res = self.thrift_client.mcVersion()
-                    if res == Result.OK:
+                    if res == carbon.carbon_result.thrift_types.Result.OK:


While this could be generated from the Carbon IDLs, createThriftTestClient isn't implemented in the OSS version of mcrouter_config.py, so it seems better to "hide" the Meta-specific bits instead.

mcrouter/tools/mcpiper/test/CMakeLists.txt

https://github.com/facebook/mcrouter currently uses a legacy autotools-based build system, which forces third parties to rely on a collection of scripts hosted in the repository to build it with all required dependencies. This is brittle and has lead to many issue reports with build issues. In facebook/mcrouter#449, I've prepared a working CMake-based build system for mcrouter that could replace the legacy autotools setup. This patch adds the necessary manifests for mcrouter and its ragel dependency so that fbcode_builder itself will be setup for the repo without the PR having to do it.

mszabo-wikia · 2024-08-01T10:46:29Z

CMakeLists.txt

+include_directories(.)
+include_directories(${CMAKE_CURRENT_BINARY_DIR})


Not super sure about this, but this seems like the best way to support the current #include style.

Summary: X-link: facebookincubator/zstrong#944 https://github.com/facebook/mcrouter currently uses a legacy autotools-based build system, which forces third parties to rely on a collection of scripts hosted in the repository to build it with all required dependencies. This is brittle and has lead to many issue reports about build problems. In facebook/mcrouter#449, I've prepared a working CMake-based build system for mcrouter that could replace the legacy autotools setup. This patch adds the necessary manifests for mcrouter and its ragel dependency so that fbcode_builder itself will be setup for the repo without having to do it in the PR. X-link: facebook/folly#2268 Reviewed By: yfeldblum Differential Revision: D60537337 Pulled By: Orvid fbshipit-source-id: ed73693d4af93acc3b8e996a7c61d0090075949f

Summary: X-link: facebookincubator/zstrong#944 https://github.com/facebook/mcrouter currently uses a legacy autotools-based build system, which forces third parties to rely on a collection of scripts hosted in the repository to build it with all required dependencies. This is brittle and has lead to many issue reports about build problems. In facebook/mcrouter#449, I've prepared a working CMake-based build system for mcrouter that could replace the legacy autotools setup. This patch adds the necessary manifests for mcrouter and its ragel dependency so that fbcode_builder itself will be setup for the repo without having to do it in the PR. Pull Request resolved: #2268 Reviewed By: yfeldblum Differential Revision: D60537337 Pulled By: Orvid fbshipit-source-id: ed73693d4af93acc3b8e996a7c61d0090075949f

Summary: X-link: facebookincubator/zstrong#944 https://github.com/facebook/mcrouter currently uses a legacy autotools-based build system, which forces third parties to rely on a collection of scripts hosted in the repository to build it with all required dependencies. This is brittle and has lead to many issue reports about build problems. In facebook/mcrouter#449, I've prepared a working CMake-based build system for mcrouter that could replace the legacy autotools setup. This patch adds the necessary manifests for mcrouter and its ragel dependency so that fbcode_builder itself will be setup for the repo without having to do it in the PR. X-link: facebook/folly#2268 Reviewed By: yfeldblum Differential Revision: D60537337 Pulled By: Orvid fbshipit-source-id: ed73693d4af93acc3b8e996a7c61d0090075949f

facebook-github-bot added the CLA Signed label Jul 29, 2024

mszabo-wikia commented Jul 29, 2024

View reviewed changes

mcrouter/tools/mcpiper/test/CMakeLists.txt Outdated Show resolved Hide resolved

mszabo-wikia force-pushed the introduce-cmake branch 4 times, most recently from 90964da to 92cb3bd Compare July 31, 2024 14:13

mszabo-wikia mentioned this pull request Jul 31, 2024

Add fbcode_builder manifests for mcrouter and ragel facebook/folly#2268

Closed

mszabo-wikia force-pushed the introduce-cmake branch from 92cb3bd to 4d2a569 Compare July 31, 2024 15:56

mszabo-wikia commented Aug 1, 2024

View reviewed changes

mszabo-wikia mentioned this pull request Aug 4, 2024

Failed to build on Ubuntu 24.04 #447

Open

mszabo-wikia force-pushed the introduce-cmake branch from 1eab3f0 to 04963d0 Compare August 11, 2024 08:27

mszabo-wikia mentioned this pull request Aug 13, 2024

MCRouter installation on UBUNTU-22 using source code. #452

Open

mszabo-wikia force-pushed the introduce-cmake branch 2 times, most recently from aa59940 to 74f2de6 Compare August 20, 2024 11:53

mszabo-wikia force-pushed the introduce-cmake branch 8 times, most recently from a962acd to b56cb7c Compare August 29, 2024 21:40

mszabo-wikia force-pushed the introduce-cmake branch from b56cb7c to 11e1209 Compare September 3, 2024 22:44

mszabo-wikia force-pushed the introduce-cmake branch 3 times, most recently from 8515304 to f629f27 Compare September 16, 2024 15:26

mszabo-wikia force-pushed the introduce-cmake branch 2 times, most recently from ed66c01 to 816fd71 Compare October 2, 2024 10:33

mszabo-wikia added 14 commits October 4, 2024 12:56

Bootstrap fbcode_builder

a7858ba

Add CMake build system

78e036f

Add GitHub Action generated by getdeps.py

8b02a8f

Hack: Eliminate fb303 dependency from example service

176341b

Skip CompressionCodecManagerTest when compression is disabled

f6740b3

Skip ServiceRouter tests when SR is not linked

b343cd3

Update test certificates to work with newer OpenSSL

940dbaa

Run Python integration tests as well

7efb33d

Fix tests

aa46512

Use folly::Init for mock servers to avoid glog warnings

d0f6567

Set timeout for flaky tests in lib/network

fd322d7

Add basic CPack packaging

1121f67

Update the README

2160807

Disable client identity propagation in the OSS build

1dbcd9b

mszabo-wikia force-pushed the introduce-cmake branch from 816fd71 to 1dbcd9b Compare October 4, 2024 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace autotools with CMake + fbcode_builder #449

Replace autotools with CMake + fbcode_builder #449

mszabo-wikia commented Jul 29, 2024 •

edited

Loading

mszabo-wikia commented Jul 29, 2024 •

edited

Loading

mszabo-wikia Jul 29, 2024

mszabo-wikia Jul 29, 2024

mszabo-wikia Aug 29, 2024

mszabo-wikia Jul 29, 2024

mszabo-wikia Aug 1, 2024

		include_directories(.)
		include_directories(${CMAKE_CURRENT_BINARY_DIR})

Replace autotools with CMake + fbcode_builder #449

Are you sure you want to change the base?

Replace autotools with CMake + fbcode_builder #449

Conversation

mszabo-wikia commented Jul 29, 2024 • edited Loading

mszabo-wikia commented Jul 29, 2024 • edited Loading

mszabo-wikia Jul 29, 2024

Choose a reason for hiding this comment

mszabo-wikia Jul 29, 2024

Choose a reason for hiding this comment

mszabo-wikia Aug 29, 2024

Choose a reason for hiding this comment

mszabo-wikia Jul 29, 2024

Choose a reason for hiding this comment

mszabo-wikia Aug 1, 2024

Choose a reason for hiding this comment

mszabo-wikia commented Jul 29, 2024 •

edited

Loading

mszabo-wikia commented Jul 29, 2024 •

edited

Loading