Skip to content

Commit 1cb8cec

Browse files
jrosskopfjrosskopf
authored
fix: don't build dev-only C++ tests when erpl_web is a dependency (GitHub #45) (#46)
test/cpp includes catch.hpp / core_functions_extension.hpp via paths relative to the in-tree `duckdb` git submodule (../../duckdb/...). When erpl_web is consumed as a dependency (duckdb_extension_load / FetchContent), only the extension source is fetched and that submodule is absent, so the C++ test target fails to compile with "catch.hpp file not found". BUILD_UNITTESTS does not distinguish the two cases — it is TRUE in both our own build and a consumer's main DuckDB build. Gate add_subdirectory(test) on the submodule's presence (the if(EXISTS ...) idiom already used for extension_callback_manager.hpp) and stop redeclaring DuckDB core's BUILD_UNITTESTS option, so consumers no longer build our developer-only tests. Add a pure-CMake regression test (make test_build_guard) that asserts the guard stays in place; it runs identically on all platforms.
1 parent 3e39f76 commit 1cb8cec

4 files changed

Lines changed: 111 additions & 2 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ make release # Full reconfigure + release build
7676
- `make test` - Run all tests (release mode)
7777
- `make test_cpp` - Run C++ unit tests (**always use this**, not the binary directly — see note below)
7878
- `make test_debug_sap` - Run SAP-specific tests with local SAP environment
79+
- `make test_build_guard` - Regression test for GitHub #45 (pure CMake, no build needed). Verifies the dev-only C++ test target stays gated on the in-tree `duckdb` submodule so consumers who statically link erpl_web (via `duckdb_extension_load`/FetchContent, where the submodule is absent) don't try to compile `test/cpp` and hit `catch.hpp file not found`. Run after touching the `add_subdirectory(test)` guard in `CMakeLists.txt`.
7980
- `./build/debug/test/unittest "[test_category]"` - Run SQL tests by category (e.g., `"[sap]"`)
8081

8182
**Running individual C++ tests:**

‎CMakeLists.txt‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
cmake_minimum_required(VERSION 3.5...3.29)
22

3-
option(BUILD_UNITTESTS "Build ERPL C++ Unit Tests." ON)
3+
# NOTE: do NOT redeclare BUILD_UNITTESTS here. DuckDB core owns this flag
4+
# (duckdb/CMakeLists.txt) and deliberately sets it FALSE for library / extension-config
5+
# builds. Redeclaring it forced our dev-only C++ test target onto consumers who statically
6+
# link erpl_web (GitHub #45), where the duckdb submodule the tests include from is absent.
7+
# We honor DuckDB's value at the bottom of this file instead.
48

59
# Set extension name here
610
set(TARGET_NAME erpl_web)
@@ -188,8 +192,21 @@ if(APPLE)
188192
target_link_libraries(${LOADABLE_EXTENSION_NAME} ${COREFOUNDATION_FRAMEWORK} ${APPLICATIONSERVICES_FRAMEWORK})
189193
endif()
190194

191-
if(${BUILD_UNITTESTS})
195+
# C++ unit tests are a developer-only artifact. They are only buildable from our own
196+
# source tree, where DuckDB is checked out as the `duckdb` git submodule: test/cpp
197+
# includes catch.hpp / core_functions_extension.hpp via paths relative to that submodule
198+
# (../../duckdb/...). When erpl_web is consumed as a dependency (e.g. via
199+
# duckdb_extension_load / FetchContent), only the extension source is fetched — the
200+
# duckdb submodule is absent — so those headers cannot be found and the test target
201+
# fails to compile (GitHub #45). BUILD_UNITTESTS does NOT distinguish the two cases: it
202+
# is TRUE in both our build and a consumer's main DuckDB build. Gate on the submodule's
203+
# presence (same if(EXISTS ...) idiom used for extension_callback_manager.hpp above),
204+
# while still honoring BUILD_UNITTESTS so developers can opt out.
205+
if(BUILD_UNITTESTS AND EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/duckdb/third_party/catch/catch.hpp")
192206
add_subdirectory(test)
207+
else()
208+
message(STATUS "erpl_web: skipping C++ unit tests (BUILD_UNITTESTS off, or duckdb test "
209+
"sources not present in-tree — e.g. erpl_web is consumed as a dependency; GitHub #45)")
193210
endif()
194211

195212
install(

‎Makefile‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,14 @@ test_debug_ms: ${EXTENSION_CONFIG_STEP}
7171
test_debug_bc: ${EXTENSION_CONFIG_STEP}
7272
./build/debug/test/unittest "test/sql/business_central_integration.test"
7373

74+
# Regression test for GitHub #45: when erpl_web is consumed as a dependency (the duckdb
75+
# submodule is absent from the fetched source), its dev-only C++ test target must NOT be
76+
# built — otherwise test/cpp fails with "catch.hpp file not found". Pure CMake script,
77+
# no toolchain/DuckDB configure required, so it runs identically on every platform.
78+
.PHONY: test_build_guard
79+
test_build_guard:
80+
cmake -DREPO_DIR='${PROJ_DIR}' -P test/cmake/test_cpp_tests_guard.cmake
81+
7482
# Run C++ unit tests. ASAN_OPTIONS=detect_odr_violation=0 suppresses a false-positive ODR
7583
# warning that arises from DuckDB being compiled into both the static test binary and
7684
# libduckdb.so. The duplicate symbol (LOOKUP_TABLE in nested_to_varchar_cast.cpp) is

‎test/cmake/test_cpp_tests_guard.cmake‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
# Regression test for GitHub #45 — "Compilation error statically linking erpl-web extension".
2+
#
3+
# When erpl_web is consumed as a dependency (duckdb_extension_load / FetchContent), only the
4+
# extension source is fetched; the `duckdb` git submodule is NOT. But test/cpp includes
5+
# catch.hpp / core_functions_extension.hpp via paths relative to that submodule
6+
# (../../duckdb/...), so building the C++ test target in a consumer tree fails with
7+
# "catch.hpp file not found". DuckDB's BUILD_UNITTESTS does not distinguish the two cases
8+
# (it is TRUE in both our dev build and a consumer's main DuckDB build), so CMakeLists.txt
9+
# must gate add_subdirectory(test) on the in-tree submodule's presence.
10+
#
11+
# This is a pure-CMake script — no DuckDB configure required — so it runs identically on
12+
# Linux, macOS and Windows.
13+
#
14+
# Run with: cmake -DREPO_DIR=<repo root> -P test/cmake/test_cpp_tests_guard.cmake
15+
16+
if(NOT DEFINED REPO_DIR)
17+
set(REPO_DIR "${CMAKE_CURRENT_LIST_DIR}/../..")
18+
endif()
19+
get_filename_component(REPO_DIR "${REPO_DIR}" ABSOLUTE)
20+
21+
set(CMAKELISTS "${REPO_DIR}/CMakeLists.txt")
22+
if(NOT EXISTS "${CMAKELISTS}")
23+
message(FATAL_ERROR "Cannot find ${CMAKELISTS}")
24+
endif()
25+
file(READ "${CMAKELISTS}" CML)
26+
27+
function(assert_contains needle why)
28+
string(FIND "${CML}" "${needle}" pos)
29+
if(pos EQUAL -1)
30+
message(FATAL_ERROR "GitHub #45 regression: ${why}\n Expected CMakeLists.txt to contain: ${needle}")
31+
endif()
32+
endfunction()
33+
34+
function(assert_absent needle why)
35+
string(FIND "${CML}" "${needle}" pos)
36+
if(NOT pos EQUAL -1)
37+
message(FATAL_ERROR "GitHub #45 regression: ${why}\n Did not expect CMakeLists.txt to contain: ${needle}")
38+
endif()
39+
endfunction()
40+
41+
# 1. The C++ tests must be gated on the duckdb submodule's catch.hpp, not built unconditionally.
42+
assert_contains("BUILD_UNITTESTS AND EXISTS"
43+
"add_subdirectory(test) is not gated on the in-tree duckdb submodule")
44+
assert_contains("duckdb/third_party/catch/catch.hpp"
45+
"the C++ test guard does not check for the duckdb submodule's catch.hpp")
46+
assert_contains("add_subdirectory(test)"
47+
"the C++ test subdirectory wiring is missing")
48+
49+
# 2. erpl_web must NOT redeclare DuckDB core's BUILD_UNITTESTS option (it shadows the flag
50+
# DuckDB and embedders use to control test building).
51+
assert_absent("option(BUILD_UNITTESTS"
52+
"erpl_web redeclares DuckDB's BUILD_UNITTESTS option instead of honoring it")
53+
54+
# 3. The guard is meaningful only because test/cpp really does depend on that submodule path.
55+
# Confirm the sources that triggered #45 are present and still include catch.hpp.
56+
foreach(need "test/cpp/CMakeLists.txt" "test/cpp/test_charset_converter.cpp")
57+
if(NOT EXISTS "${REPO_DIR}/${need}")
58+
message(FATAL_ERROR "Expected ${need} to exist (the GitHub #45 reproduction sources)")
59+
endif()
60+
endforeach()
61+
file(READ "${REPO_DIR}/test/cpp/test_charset_converter.cpp" CHARSET_TEST)
62+
string(FIND "${CHARSET_TEST}" "catch.hpp" charset_pos)
63+
if(charset_pos EQUAL -1)
64+
message(FATAL_ERROR "test/cpp/test_charset_converter.cpp no longer includes catch.hpp — "
65+
"the GitHub #45 guard rationale may be stale; revisit this test.")
66+
endif()
67+
68+
# 4. Behavioral check of the guard condition, both ways:
69+
# (a) developer tree WITH the submodule -> condition is satisfied (tests build).
70+
if(NOT EXISTS "${REPO_DIR}/duckdb/third_party/catch/catch.hpp")
71+
message(FATAL_ERROR "Expected the duckdb submodule (catch.hpp) to be present in the dev tree; "
72+
"cannot validate the positive branch of the guard.")
73+
endif()
74+
# (b) a fake consumer tree WITHOUT the submodule -> condition is NOT satisfied (tests skipped).
75+
set(FAKE_CONSUMER "${REPO_DIR}/build/guard_check_fake_consumer")
76+
file(REMOVE_RECURSE "${FAKE_CONSUMER}")
77+
file(MAKE_DIRECTORY "${FAKE_CONSUMER}")
78+
if(EXISTS "${FAKE_CONSUMER}/duckdb/third_party/catch/catch.hpp")
79+
message(FATAL_ERROR "test bug: the fake consumer tree must not contain the duckdb submodule")
80+
endif()
81+
file(REMOVE_RECURSE "${FAKE_CONSUMER}")
82+
83+
message(STATUS "PASS: GitHub #45 guard present and correct — erpl_web C++ unit tests are gated on the in-tree duckdb submodule and do not build for dependency consumers.")

0 commit comments

Comments
 (0)