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

As observed in facebook#449, the OSS build currently produces various compiler warnings, mostly from OSS-specific stubs. Fix the following warnings: * Fix -Wunused-parameter in stub functions by commenting out the relevant parameters. * Fix -Wunused-local-typedef by removing unused typedefs. * Fix -Wreturn-local-addr in SRHost by returning a reference to a member variable instead. * Fix -Wredundant-move by removing a redundant std::move() call when returning a function parameter.

After 6c2142a, standalone mcrouter now deadlocks if router the configuration was incorrect. Spotted on facebook#449, where `test_unknown_named_handles` started failing due to the mcrouter process being tested never existing. GDB [indicates](https://gist.github.com/mszabo-wikia/47916c5655deffdb95332e972a52caf8) a deadlock between three threads: ``` (gdb) info threads Id Target Id Frame * 1 Thread 0x7f7b4cce1e00 (LWP 211878) "mcrouter" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 2 Thread 0x7f7b43fff6c0 (LWP 211882) "mcr-cpuaux-0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 3 Thread 0x7f7b49e0a6c0 (LWP 211879) "IOThreadPool0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 ``` Thread 1, the main thread, has triggered exit() and is waiting on the auxiliary thread pool to be destroyed. Thread 2, an auxiliary thread pool thread, is in the process of destroying the CarbonRouterInstance due to 6c2142a and is blocked on destroying the virtual EVBs by child proxies. These however are ultimately sourced from the IO thread pool which is also used by AsyncMcServer. Thread 3, an IO thread pool thread, is blocked by AsyncMcServer which is waiting to be started by later initialization code that never runs due to the config error, preventing the IO thread pool itself from being destroyed. Fix it by initializing the AsyncMcServer only after the router has been initialized.

Summary: After 6c2142a, standalone mcrouter now deadlocks if the router configuration was incorrect. Spotted on #449, where `test_unknown_named_handles` started failing due to the mcrouter process being tested never exiting. GDB [indicates](https://gist.github.com/mszabo-wikia/47916c5655deffdb95332e972a52caf8) a deadlock between three threads: ``` (gdb) info threads Id Target Id Frame * 1 Thread 0x7f7b4cce1e00 (LWP 211878) "mcrouter" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 2 Thread 0x7f7b43fff6c0 (LWP 211882) "mcr-cpuaux-0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 3 Thread 0x7f7b49e0a6c0 (LWP 211879) "IOThreadPool0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 ``` * Thread 1, the main thread, has triggered exit() and is waiting on the auxiliary thread pool to be destroyed. * Thread 2, an auxiliary thread pool thread, is in the process of destroying the CarbonRouterInstance due to 6c2142a and is blocked on destroying the virtual EVBs by child proxies. These however are ultimately sourced from the IO thread pool which is also used by AsyncMcServer. * Thread 3, an IO thread pool thread, is blocked by AsyncMcServer which is waiting to be started by later initialization code that never runs due to the config error, preventing the IO thread pool itself from being destroyed. Fix it by initializing the AsyncMcServer only after the router has been initialized. Pull Request resolved: #455 Reviewed By: lenar-f Differential Revision: D67069250 Pulled By: disylh fbshipit-source-id: d6edff40b9f40ee7925e94c4ee9cf985c10a98de

Summary: After 6c2142acd8e69edd40eed70a93ea17ee2909287d, standalone mcrouter now deadlocks if the router configuration was incorrect. Spotted on facebook/mcrouter#449, where `test_unknown_named_handles` started failing due to the mcrouter process being tested never exiting. GDB [indicates](https://gist.github.com/mszabo-wikia/47916c5655deffdb95332e972a52caf8) a deadlock between three threads: ``` (gdb) info threads Id Target Id Frame * 1 Thread 0x7f7b4cce1e00 (LWP 211878) "mcrouter" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 2 Thread 0x7f7b43fff6c0 (LWP 211882) "mcr-cpuaux-0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 3 Thread 0x7f7b49e0a6c0 (LWP 211879) "IOThreadPool0" syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38 ``` * Thread 1, the main thread, has triggered exit() and is waiting on the auxiliary thread pool to be destroyed. * Thread 2, an auxiliary thread pool thread, is in the process of destroying the CarbonRouterInstance due to 6c2142acd8e69edd40eed70a93ea17ee2909287d and is blocked on destroying the virtual EVBs by child proxies. These however are ultimately sourced from the IO thread pool which is also used by AsyncMcServer. * Thread 3, an IO thread pool thread, is blocked by AsyncMcServer which is waiting to be started by later initialization code that never runs due to the config error, preventing the IO thread pool itself from being destroyed. Fix it by initializing the AsyncMcServer only after the router has been initialized. X-link: facebook/mcrouter#455 Reviewed By: lenar-f Differential Revision: D67069250 Pulled By: disylh fbshipit-source-id: d6edff40b9f40ee7925e94c4ee9cf985c10a98de

Summary: The OSS autotools build has been nonfunctional for about a year. While #449 would be a better long-term approach to fixing the build since it would also bring it in sync with other Meta OSS projects and enable testing in CI, fixing the existing build until there's movement on that front would at least provide a signal on whether the OSS version is buildable at all. So: * Introduce new Ubuntu 24.04 build files and use them in CI. I'll remove build files, Dockerfiles etc. for ancient Ubuntu and CentOS versions in a followup PR. * Pin fmtlib to 11.0.2, like fbcode_builder does, and skip building its test suite to not waste time. * Use Ninja to generate and build CMake-based dependencies. * Use googletest, gmock and zstd as packaged by Ubuntu rather than building their older versions from source. * Introduce and use a new define `MCROUTER_OSS_BUILD` to mark OSS-specific conditional compilation blocks. This could probably replace the existing `LIBMC_FBTRACE_DISABLE` and `DISABLE_COMPRESSION` defines too, as long as those aren't used outside of the OSS build. * Fix OSS build failures that have gone undetected due to CI being broken: * Add a shim for the SR-specific method `HostInfoLocation::hostUniqueKey` (used since 85facb9). * Remove redundant CarbonRouterInstance deletion logging that was using a Meta-specific include (used since 6c2142a). * Disable client identity propagation in the OSS build since it relies on Meta-specific code (used since 2f32271). Fixes #453, fixes #444, fixes #447 etc. Pull Request resolved: #457 Reviewed By: lenar-f Differential Revision: D67428119 Pulled By: disylh fbshipit-source-id: a22e7616d9b82875611e45bf1df40a771db569d9

Summary: The OSS autotools build has been nonfunctional for about a year. While facebook/mcrouter#449 would be a better long-term approach to fixing the build since it would also bring it in sync with other Meta OSS projects and enable testing in CI, fixing the existing build until there's movement on that front would at least provide a signal on whether the OSS version is buildable at all. So: * Introduce new Ubuntu 24.04 build files and use them in CI. I'll remove build files, Dockerfiles etc. for ancient Ubuntu and CentOS versions in a followup PR. * Pin fmtlib to 11.0.2, like fbcode_builder does, and skip building its test suite to not waste time. * Use Ninja to generate and build CMake-based dependencies. * Use googletest, gmock and zstd as packaged by Ubuntu rather than building their older versions from source. * Introduce and use a new define `MCROUTER_OSS_BUILD` to mark OSS-specific conditional compilation blocks. This could probably replace the existing `LIBMC_FBTRACE_DISABLE` and `DISABLE_COMPRESSION` defines too, as long as those aren't used outside of the OSS build. * Fix OSS build failures that have gone undetected due to CI being broken: * Add a shim for the SR-specific method `HostInfoLocation::hostUniqueKey` (used since 85facb9200dea75e661447ed11e05806f42de65f). * Remove redundant CarbonRouterInstance deletion logging that was using a Meta-specific include (used since 6c2142acd8e69edd40eed70a93ea17ee2909287d). * Disable client identity propagation in the OSS build since it relies on Meta-specific code (used since 2f32271533fdf54ce71cfb65f6fda3c621f076a4). Fixes facebook/mcrouter#453, fixes facebook/mcrouter#444, fixes facebook/mcrouter#447 etc. X-link: facebook/mcrouter#457 Reviewed By: lenar-f Differential Revision: D67428119 Pulled By: disylh fbshipit-source-id: a22e7616d9b82875611e45bf1df40a771db569d9

As observed in facebook#449, the OSS build currently produces various compiler warnings, mostly from OSS-specific stubs. Fix the following warnings: * Fix -Wunused-parameter in stub functions by commenting out the relevant parameters. * Fix -Wunused-local-typedef by removing unused typedefs. * Fix -Wreturn-local-addr in SRHost by returning a reference to a member variable instead. * Fix -Wredundant-move by removing a redundant std::move() call when returning a function parameter.

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

Closed

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 from f8b5bec to 9bb6fa2 Compare December 11, 2024 11:15

mszabo-wikia mentioned this pull request Dec 11, 2024

Fix autotools build #457

Closed

mszabo-wikia force-pushed the introduce-cmake branch from 9bb6fa2 to af7eab2 Compare December 11, 2024 20:28

mszabo-wikia force-pushed the introduce-cmake branch from 168ac69 to d182405 Compare December 13, 2024 09:48

mszabo-wikia added 17 commits December 19, 2024 12:40

Bootstrap fbcode_builder

f40fc69

Add CMake build system

4b4c613

Add GitHub Action generated by getdeps.py

f741e7d

Hack: Eliminate fb303 dependency from example service

c4a13a3

Skip CompressionCodecManagerTest when compression is disabled

cadaf43

Skip ServiceRouter tests when SR is not linked

a0e46b2

Update test certificates to work with newer OpenSSL

a042334

Run Python integration tests as well

0805ea8

Fix tests

3604ad5

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

631f285

Set timeout for flaky tests in lib/network

5436663

Add basic CPack packaging

d96fd27

Update the README

e55da44

Fix refill lease-get test for OSS

c318691

Lower timeout for integration tests

f96d70f

Fix OSS compiler warnings

f443398

Fix flaky CarbonRouterClient.basicUsageSameThreadClient test

3a0ef5c

mszabo-wikia force-pushed the introduce-cmake branch from d182405 to 3a0ef5c Compare December 19, 2024 12:18

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