Skip to content

Commit afdb05a

Browse files
John Huggmeta-codesync[bot]
John Hugg
authored and
meta-codesync[bot]
committed
Fix the OSS Autotools build
Summary: D113608290 broke the OSS GitHub build and D114285182 did not fix it. There were several distinct problems, not one. Everything below was built and run in a real Ubuntu 24.04 chroot rather than reasoned about. **1. Automake rejects literal `-Wl,` tokens in `LDADD`.** That was the original D113608290 failure. D114285182 hid them behind `$(FBTHRIFT_LINKER_DRIVER_FLAG)`, which got past `autoreconf` but made things worse: Automake only expands a word when the whole word is a `$(VAR)` reference, so it copied `$(FBTHRIFT_LINKER_DRIVER_FLAG),--start-group` verbatim into `mock_mc_server_DEPENDENCIES`, giving `No rule to make target '-Wl,--start-group'`. Fixed by defining `FBTHRIFT_STATIC_LIBS` via `AC_SUBST`, which is opaque to both the flag check and the dependency computation. **2. libtool defeats the linker group anyway.** This is the part that makes the whole D113608290 approach unworkable, independent of Automake syntax. libtool hoists every `-Wl,` flag to the front of the command and de-duplicates repeated `-l` flags: ``` automake passes: ... libmcrouter.a -Wl,--start-group -lthriftcpp2 ... -lcommon -Wl,--end-group -lfmt ... libtool execs: ... -Wl,--start-group -Wl,--end-group -L... libmcrouter.a -lthriftcpp2 ... -lcommon ... ``` The group collapses to an empty pair. Because libtool also de-duplicates (keeping the last occurrence), repeating libraries cannot resolve the cycle either. Fixed with a `_LINK` override on the two affected programs so they are linked by `$(CXXLD)` directly; the fbthrift archives are genuinely cyclic, so the group is required. **3. `-lthrift_path` was missing** from the closure. It defines `apache::thrift::dynamic::detail::typeDisplayName`, used by `libthrift_dynamic_value.a`. **4. `Common_constants.cpp` and `Memcache_constants.cpp` are generated but were never compiled.** They define the `_fbthrift_schema_*_includes()` symbols that `*_data.cpp` references. **5. `Server.cpp` was never compiled**, leaving `mcrouter::detail::startServerShutdown` undefined in the `mcrouter` binary. **6. `CarbonTest.thrift` was never compiled at all.** `lib/network/test/gen` had no `Makefile.am` and no `AC_CONFIG_FILES` entry, so `gen-cpp2/CarbonTest_types.h` never existed and every `mcrouter_network_test` source failed to compile. Added a `Makefile.am` for that directory following the `lib/network/gen` pattern, wired it into `SUBDIRS` and `AC_CONFIG_FILES`, and added the generated sources to the test. Also corrected `$(top_builddir)/lib/network/libtest_util.a` to `libtest_util.a` — that archive is built in `lib/network/test`, not `lib/network`. The top-level `mcrouter_LDADD` also carried its own stale copy of the fbthrift list; it now shares `FBTHRIFT_STATIC_LIBS`. **`make check` still does not work, and this diff does not claim to fix it.** Item 6 fixes the compile step, but the OSS test wiring has independent rot: `libtestmain.la`'s only source, `lib/TestMain.cpp`, does not exist anywhere in the repo (7 test directories depend on it), and `MockMcServer.cpp` is listed in `mcrouter_network_test_SOURCES` even though it defines its own `main()`, so the test binary runs a mock server instead of gtest. Resurrecting `make check` is a separate piece of work. CI runs `make`, not `make check`. Reviewed By: anidev Differential Revision: D114393734 fbshipit-source-id: e3f7c269c929e40995ba3b07a75244ef4a20ff0d
1 parent 1f1e9f6 commit afdb05a

6 files changed

Lines changed: 73 additions & 43 deletions

File tree

‎mcrouter/Makefile.am‎

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ libmcroutercore_a_SOURCES = \
235235

236236
mcrouter_SOURCES = \
237237
main.cpp \
238+
Server.cpp \
238239
Server-inl.h \
239240
Server.h \
240241
ServerOnRequest.h \
@@ -250,31 +251,18 @@ mcrouter_SOURCES = \
250251
libmcroutercore_a_CPPFLAGS = -I..
251252

252253
# Link order matters.
254+
mcrouter_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) $(AM_LDFLAGS) $(LDFLAGS) -o $@
253255
mcrouter_LDADD = \
254256
libmcroutercore.a \
255257
lib/libmcrouter.a \
256-
-lthriftcpp2 \
257-
-lserverdbginfo \
258-
-ltransport \
259-
-lthriftanyrep \
260-
-lthrifttype \
261-
-lthrifttyperep \
262-
-lthriftprotocol \
263-
-lrpcmetadata \
264-
-lthriftannotation \
265-
-lthriftmetadata \
266-
-lasync \
267-
-lconcurrency \
268-
-lruntime \
269-
-lthrift-core \
270-
-lcommon
258+
$(FBTHRIFT_STATIC_LIBS)
271259

272260
mcrouter_CPPFLAGS = -I..
273261

274262
clean-local:
275-
-rm -rf lib/network/gen-cpp2/ \
263+
-rm -rf lib/network/gen/gen-cpp2/ \
276264
lib/carbon/gen-cpp2/
277265

278266
distclean-local:
279-
-rm -rf lib/network/gen-cpp2/ \
267+
-rm -rf lib/network/gen/gen-cpp2/ \
280268
lib/carbon/gen-cpp2/

‎mcrouter/configure.ac‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,14 @@ AC_SUBST([FBTHRIFT])
2525

2626
AC_SUBST([INSTALL_DIR], [$INSTALL_DIR])
2727

28+
# fbthrift's static libraries are mutually recursive, so they have to be linked
29+
# inside a --start-group/--end-group pair. This lives in configure.ac rather
30+
# than Makefile.am because Automake inspects LDADD contents it can see: it
31+
# rejects literal -Wl tokens there, and turns anything it cannot classify into a
32+
# make prerequisite. A configure substitution is opaque to both checks.
33+
AC_SUBST([FBTHRIFT_STATIC_LIBS],
34+
["-Wl,--start-group -lthriftcpp2 -lserverdbginfo -ltransport -lthriftanyrep -lthriftannotation -lthrifttype -lthrifttyperep -lthrift_dynamic_value -lthrift_path -lrpcmetadata -lthriftprotocol -lthriftmetadata -lasync -lconcurrency -lruntime -lthrift-core -lcommon -Wl,--end-group"])
35+
2836
AC_DEFINE_UNQUOTED([MCROUTER_PACKAGE_STRING], ["AC_PACKAGE_VERSION AC_PACKAGE_NAME"], [Full name and version of this package, with version coming first for libmemcached compatibility])
2937

3038
AC_CONFIG_SRCDIR([CarbonRouterInstance.h])
@@ -218,6 +226,7 @@ AC_CONFIG_FILES([Makefile
218226
lib/fbi/cpp/test/Makefile
219227
lib/network/gen/Makefile
220228
lib/network/test/Makefile
229+
lib/network/test/gen/Makefile
221230
routes/Makefile
222231
routes/test/Makefile
223232
test/Makefile

‎mcrouter/lib/Makefile.am‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,8 @@ libmcrouter_a_SOURCES = \
208208
network/gen/gen-cpp2/Common_types_custom_protocol.h \
209209
network/gen/gen-cpp2/Common_data.cpp \
210210
network/gen/gen-cpp2/Common_data.h \
211+
network/gen/gen-cpp2/Common_constants.cpp \
212+
network/gen/gen-cpp2/Common_constants.h \
211213
network/gen/gen-cpp2/MemcacheAsyncClient.cpp \
212214
network/gen/gen-cpp2/MemcacheAsyncClient.h \
213215
network/gen/gen-cpp2/MemcacheService_constants.cpp \
@@ -228,6 +230,8 @@ libmcrouter_a_SOURCES = \
228230
network/gen/gen-cpp2/Memcache_types_custom_protocol.h \
229231
network/gen/gen-cpp2/Memcache_data.cpp \
230232
network/gen/gen-cpp2/Memcache_data.h \
233+
network/gen/gen-cpp2/Memcache_constants.cpp \
234+
network/gen/gen-cpp2/Memcache_constants.h \
231235
network/FizzContextProvider.cpp \
232236
network/FizzContextProvider.h \
233237
network/McAsciiParser-gen.cpp \

‎mcrouter/lib/network/gen/Makefile.am‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ BUILT_SOURCES = \
1212
gen-cpp2/Common_types_custom_protocol.h \
1313
gen-cpp2/Common_data.cpp \
1414
gen-cpp2/Common_data.h \
15+
gen-cpp2/Common_constants.cpp \
16+
gen-cpp2/Common_constants.h \
1517
gen-cpp2/Memcache_types.cpp \
1618
gen-cpp2/Memcache_types_binary.cpp \
1719
gen-cpp2/Memcache_types_compact.cpp \
@@ -20,6 +22,8 @@ BUILT_SOURCES = \
2022
gen-cpp2/Memcache_types_custom_protocol.h \
2123
gen-cpp2/Memcache_data.cpp \
2224
gen-cpp2/Memcache_data.h \
25+
gen-cpp2/Memcache_constants.cpp \
26+
gen-cpp2/Memcache_constants.h \
2327
gen-cpp2/MemcacheAsyncClient.cpp \
2428
gen-cpp2/MemcacheAsyncClient.h \
2529
gen-cpp2/MemcacheService_constants.cpp \
@@ -38,7 +42,9 @@ gen-cpp2/Common_types_binary.cpp: gen-cpp2/Common_types.h
3842
gen-cpp2/Common_types_compact.cpp: gen-cpp2/Common_types.h
3943
gen-cpp2/Common_types_serialization.cpp: gen-cpp2/Common_types.h
4044
gen-cpp2/Common_types.h: gen-cpp2/Common_types_custom_protocol.h
41-
gen-cpp2/Common_types_custom_protocol.h: gen-cpp2/Common_data.cpp
45+
gen-cpp2/Common_constants.cpp: gen-cpp2/Common_constants.h
46+
gen-cpp2/Common_constants.h: gen-cpp2/Common_data.cpp
47+
gen-cpp2/Common_types_custom_protocol.h: gen-cpp2/Common_constants.cpp
4248
gen-cpp2/Common_data.cpp: gen-cpp2/Common_data.h
4349
gen-cpp2/Common_data.h: Common.thrift
4450
@FBTHRIFT@ -gen mstch_cpp2:stack_arguments,sync_methods_return_try,deprecated_terse_writes,include_prefix=mcrouter/lib/network/gen -I $(top_srcdir)/.. -I $(INSTALL_DIR)/include/ Common.thrift
@@ -48,7 +54,9 @@ gen-cpp2/Memcache_types_binary.cpp: gen-cpp2/Memcache_types.h
4854
gen-cpp2/Memcache_types_compact.cpp: gen-cpp2/Memcache_types.h
4955
gen-cpp2/Memcache_types_serialization.cpp: gen-cpp2/Memcache_types.h
5056
gen-cpp2/Memcache_types.h: gen-cpp2/Memcache_types_custom_protocol.h
51-
gen-cpp2/Memcache_types_custom_protocol.h: gen-cpp2/Memcache_data.cpp
57+
gen-cpp2/Memcache_constants.cpp: gen-cpp2/Memcache_constants.h
58+
gen-cpp2/Memcache_constants.h: gen-cpp2/Memcache_data.cpp
59+
gen-cpp2/Memcache_types_custom_protocol.h: gen-cpp2/Memcache_constants.cpp
5260
gen-cpp2/Memcache_data.cpp: gen-cpp2/Memcache_data.h
5361
gen-cpp2/Memcache_data.h: Memcache.thrift
5462
@FBTHRIFT@ -gen mstch_cpp2:stack_arguments,sync_methods_return_try,deprecated_terse_writes,include_prefix=mcrouter/lib/network/gen -I $(top_srcdir)/.. -I $(INSTALL_DIR)/include/ Memcache.thrift

‎mcrouter/lib/network/test/Makefile.am‎

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,40 +3,22 @@
33
# This source code is licensed under the MIT license found in the
44
# LICENSE file in the root directory of this source tree.
55

6+
SUBDIRS = gen
7+
68
noinst_PROGRAMS = mock_mc_server
79
noinst_LIBRARIES = libtest_util.a
810
check_PROGRAMS = mcrouter_network_test
911

10-
# Keep the group delimiters in LDADD so they surround the static libraries.
11-
# Splitting the driver prefix prevents Automake from misclassifying them as
12-
# target LDFLAGS, which would be emitted before LDADD and break the grouping.
13-
FBTHRIFT_LINKER_DRIVER_FLAG = -Wl
14-
FBTHRIFT_STATIC_LIBS = \
15-
$(FBTHRIFT_LINKER_DRIVER_FLAG),--start-group \
16-
-lthriftcpp2 \
17-
-lserverdbginfo \
18-
-ltransport \
19-
-lthriftanyrep \
20-
-lthriftannotation \
21-
-lthrifttype \
22-
-lthrifttyperep \
23-
-lthrift_dynamic_value \
24-
-lrpcmetadata \
25-
-lthriftprotocol \
26-
-lthriftmetadata \
27-
-lasync \
28-
-lconcurrency \
29-
-lruntime \
30-
-lthrift-core \
31-
-lcommon \
32-
$(FBTHRIFT_LINKER_DRIVER_FLAG),--end-group
12+
# FBTHRIFT_STATIC_LIBS comes from configure so that Automake treats it as an
13+
# opaque substitution; see the comment on AC_SUBST in configure.ac.
3314

3415
mock_mc_server_SOURCES = \
3516
MockMc.cpp \
3617
MockMc.h \
3718
MockMcOnRequest.h \
3819
MockMcServer.cpp
3920
mock_mc_server_CPPFLAGS = -I$(top_srcdir)/..
21+
mock_mc_server_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) $(AM_LDFLAGS) $(LDFLAGS) -o $@
4022
# Link order matters.
4123
mock_mc_server_LDADD = \
4224
$(top_builddir)/lib/libmcrouter.a \
@@ -90,6 +72,13 @@ mcrouter_network_test_SOURCES = \
9072
CarbonMockMcTest.cpp \
9173
CarbonQueueAppenderTest.cpp \
9274
gen/CarbonTestMessages.cpp \
75+
gen/CarbonTestMessagesThrift.cpp \
76+
gen/gen-cpp2/CarbonTest_types.cpp \
77+
gen/gen-cpp2/CarbonTest_types_binary.cpp \
78+
gen/gen-cpp2/CarbonTest_types_compact.cpp \
79+
gen/gen-cpp2/CarbonTest_types_serialization.cpp \
80+
gen/gen-cpp2/CarbonTest_data.cpp \
81+
gen/gen-cpp2/CarbonTest_constants.cpp \
9382
McAsciiParserTest.cpp \
9483
McParserTest.cpp \
9584
McServerAsciiParserTest.cpp \
@@ -110,5 +99,5 @@ mcrouter_network_test_CPPFLAGS = \
11099
mcrouter_network_test_LDADD = \
111100
$(top_builddir)/lib/libmcrouter.a \
112101
$(top_builddir)/lib/libtestmain.la \
113-
$(top_builddir)/lib/network/libtest_util.a \
102+
libtest_util.a \
114103
$(FBTHRIFT_STATIC_LIBS)

‎mcrouter/lib/network/test/gen/Makefile.am‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# Copyright (c) Facebook, Inc. and its affiliates.
2+
#
3+
# This source code is licensed under the MIT license found in the
4+
# LICENSE file in the root directory of this source tree.
5+
6+
BUILT_SOURCES = \
7+
gen-cpp2/CarbonTest_types.cpp \
8+
gen-cpp2/CarbonTest_types_binary.cpp \
9+
gen-cpp2/CarbonTest_types_compact.cpp \
10+
gen-cpp2/CarbonTest_types_serialization.cpp \
11+
gen-cpp2/CarbonTest_types.h \
12+
gen-cpp2/CarbonTest_types_custom_protocol.h \
13+
gen-cpp2/CarbonTest_data.cpp \
14+
gen-cpp2/CarbonTest_data.h \
15+
gen-cpp2/CarbonTest_constants.cpp \
16+
gen-cpp2/CarbonTest_constants.h
17+
18+
gen-cpp2/CarbonTest_types.cpp: gen-cpp2/CarbonTest_types.h
19+
gen-cpp2/CarbonTest_types_binary.cpp: gen-cpp2/CarbonTest_types.h
20+
gen-cpp2/CarbonTest_types_compact.cpp: gen-cpp2/CarbonTest_types.h
21+
gen-cpp2/CarbonTest_types_serialization.cpp: gen-cpp2/CarbonTest_types.h
22+
gen-cpp2/CarbonTest_types.h: gen-cpp2/CarbonTest_types_custom_protocol.h
23+
gen-cpp2/CarbonTest_constants.cpp: gen-cpp2/CarbonTest_constants.h
24+
gen-cpp2/CarbonTest_constants.h: gen-cpp2/CarbonTest_data.cpp
25+
gen-cpp2/CarbonTest_types_custom_protocol.h: gen-cpp2/CarbonTest_constants.cpp
26+
gen-cpp2/CarbonTest_data.cpp: gen-cpp2/CarbonTest_data.h
27+
gen-cpp2/CarbonTest_data.h: CarbonTest.thrift
28+
@FBTHRIFT@ -gen mstch_cpp2:stack_arguments,sync_methods_return_try,deprecated_terse_writes,include_prefix=mcrouter/lib/network/test/gen -I $(top_srcdir)/.. -I $(INSTALL_DIR)/include/ $(srcdir)/CarbonTest.thrift
29+
30+
# thrift1 emits more files than BUILT_SOURCES lists, so remove the whole tree.
31+
clean-local:
32+
-rm -rf gen-cpp2/

0 commit comments

Comments
 (0)