build: add reusable CMake library target for downstream projects#236
Conversation
Add conditional CMake configuration for macOS (APPLE): - Skip -m32 and Linux-specific linker flags - Define HAVE_STRLCPY/HAVE_STRLCAT (macOS provides these natively) - Exclude n_str.c (strlcpy/strlcat bundled implementations) - Suppress AppleClang-specific warnings in upstream code - Use -flat_namespace for dylib and test executables to enable FFF test fake symbol interposition All changes are guarded by if(APPLE)/if(NOT APPLE) so Linux CI behavior is completely unchanged.
Use check_symbol_exists() to detect system-provided strlcpy/strlcat (macOS, *BSD) and conditionally exclude n_str.c and define HAVE_STRLCPY/ HAVE_STRLCAT guards. This avoids collisions with the platform's fortified string macros that prevented compilation on macOS. Guard the 32-bit x86 compile/link flags behind a Linux x86 check instead of if(NOT APPLE), making the build portable to any non-x86 platform. Suppress two AppleClang warnings-as-errors in upstream code (-Wstrict-prototypes, -Wunused-but-set-variable). Add -Wl,-flat_namespace on macOS for both the library and test executables so FFF test fakes can interpose dylib symbols.
- n_ua.c: add void to NoteUserAgent() prototype to fix -Wstrict-prototypes (empty parens means unspecified args in C) - n_helpers.c: remove unused variables j and lastLengthCount that triggered -Wunused-but-set-variable With these fixes the -Wno-* suppressions added in the previous commit are no longer needed and are removed from CMakeLists.txt.
Adds a `run_macos_unit_tests` job that configures and builds note-c with tests on macos-latest. Tests are not run via ctest because FFF symbol interposition does not fully work on macOS — even with -flat_namespace, intra-library calls are resolved at link time and cannot be faked.
- Add `strlcpy` and `strlcat` to the libc dependency whitelist so the `check_libc_dependencies` job passes when `n_str.c` is excluded on systems that provide these functions natively (glibc 2.38+). - Only add `n_str.c` to the lcov coverage exclude list when the file is actually compiled, avoiding an lcov error on unused patterns.
Add a clean note_c_lib static library target that downstream projects (e.g. note-posix) can consume via add_subdirectory + target_link_libraries without pulling in the unit test harness, 32-bit cross-compile flags, or FFF interposition settings. - Bump cmake_minimum_required to 3.21 for `PROJECT_IS_TOP_LEVEL` - Default `NOTE_C_BUILD_TESTS` to ON only when note-c is the top-level project; OFF when included as a subdirectory - Move the existing note_c shared library (with -Werror, 32-bit flags, NOTE_C_TEST, etc.) inside the NOTE_C_BUILD_TESTS guard - Add test/integration/ with a standalone CMake project that verifies `note_c_lib` links correctly via `add_subdirectory`
a2f10d9 to
6dedcda
Compare
|
Pretty cool! 🌟 I'm tending a dumpster fire at the moment 🔥, but I am excited to check this one out! 😎 Cheers! |
Any updates? |
|
Suggested cleanup around The diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2eaa1a7..xxxxxxx 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -21,11 +21,18 @@ option(NOTE_C_LOW_MEM "Build the library tailored for low memory usage." OFF)
option(NOTE_C_MEM_CHECK "Run tests with Valgrind." OFF)
option(NOTE_NODEBUG "Build the library without debug information." OFF)
-option(NOTE_C_NO_LIBC "Build the library without linking against libc, generating errors for any undefined symbols." OFF)
+option(NOTE_C_NO_LIBC "Audit libc dependencies by linking the test shared library without libc." OFF)
option(NOTE_C_SHOW_MALLOC "Build the library with flags required to log memory usage." OFF)
option(NOTE_C_SINGLE_PRECISION "Use single precision for JSON floating point numbers." OFF)
option(NOTE_C_HEARTBEAT_CALLBACK "Enable heartbeat callback support." OFF)
+if(NOTE_C_NO_LIBC AND NOT NOTE_C_BUILD_TESTS)
+ message(FATAL_ERROR
+ "NOTE_C_NO_LIBC is only meaningful for the note_c shared test/audit "
+ "target. Enable NOTE_C_BUILD_TESTS, or leave NOTE_C_NO_LIBC disabled "
+ "when consuming note_c_lib."
+ )
+endif()
+
set(NOTE_C_SRC_DIR ${CMAKE_CURRENT_SOURCE_DIR})
@@ -97,15 +104,6 @@ else()
)
endif()
-if(NOTE_C_NO_LIBC)
- target_link_options(
- note_c_lib
- PRIVATE
- -nostdlib
- -nodefaultlibs
- LINKER:--no-undefined
- )
-endif()
-
if(NOTE_NODEBUG)
target_compile_definitions(
note_c_libThe existing |
|
I would also wire the new Suggested workflow addition: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index c845f41..xxxxxxx 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -198,6 +198,22 @@ jobs:
run: |
docker run --rm --volume $(pwd):/note-c/ --workdir /note-c/ --entrypoint ./scripts/run_cppcheck.sh ghcr.io/blues/note_c_ci:latest
+ run_integration_tests:
+ runs-on: ubuntu-latest
+
+ steps:
+ - name: Checkout code
+ uses: actions/checkout@v4
+
+ - name: Configure add_subdirectory integration test
+ run: cmake -S test/integration -B build-integration
+
+ - name: Build add_subdirectory integration test
+ run: cmake --build build-integration --parallel $(nproc)
+
+ - name: Run add_subdirectory integration test
+ run: ctest --test-dir build-integration --output-on-failure
+
run_macos_unit_tests:
runs-on: macos-latest
This catches regressions in the exact downstream-consumer workflow introduced by this PR without pulling in the unit-test harness, 32-bit test flags, or FFF-specific settings. |
- Remove the dead `NOTE_C_NO_LIBC` block from the static note_c_lib target. - Add a `FATAL_ERROR` guard so enabling `NOTE_C_NO_LIBC` without `NOTE_C_BUILD_TESTS` fails loudly instead of appearing to audit when the audit target isn't built, and clarify the option's help text. - Wire test/integration into CI (run_integration_tests job) so the add_subdirectory + target_link_libraries(... note_c_lib) downstream contract is exercised on every PR.
|
Added the first commit to address your valid feedback, and a second commit as some cleanup to DRY up some definitions. |
zfields
left a comment
There was a problem hiding this comment.
Thanks Mat! This was long overdue. 🙏
You're welcome. I have an open issue from March (#238) - keen to hear your thoughts on that perhaps with Ray's input? It's not specifically a note-c issue, more a notecard change, but note-c was the closest available place where I could raise the issue. |
Adds a reusable CMake library target, so that downstream targets don't have to deal with note-c internals, and can opt-in to build the tests.
This builds on PR #225, since those fixes are needed to get a clean build on macOS.
Summary
note_c_libstatic library target that downstream projects (e.g. note-posix) can consume viaadd_subdirectoryandtarget_link_libraries, without pulling in the unit test harness, 32-bit cross-compile flags, or FFF interposition settings, which are specific to the tests.NOTE_C_BUILD_TESTSto ON only when note-c is the top-level project (usingPROJECT_IS_TOP_LEVEL), so subdirectory consumers skip tests automatically.cmake_minimum_requiredfrom 3.20 to 3.21 forPROJECT_IS_TOP_LEVELsupport.Usage
The
note_c_libtarget carries the public include path, platform compile definitions (HAVE_STRLCPY, etc.), and all build options (NOTE_C_LOW_MEM,NOTE_C_SINGLE_PRECISION, etc.) — but none of the test-specific settings.Test plan
https://github.com/zakoverflow/note-posix— pristine clone builds and passes all 13 tests on macOS usingnote_c_lib