Skip to content

Fix #143: SystemLink Cmake module is unnecessary complexity#145

Merged
lefticus merged 2 commits into
cpp-best-practices:mainfrom
leftibot:fix/issue-143-systemlink-cmake-module-is-unnecessary-c
Apr 12, 2026
Merged

Fix #143: SystemLink Cmake module is unnecessary complexity#145
lefticus merged 2 commits into
cpp-best-practices:mainfrom
leftibot:fix/issue-143-systemlink-cmake-module-is-unnecessary-c

Conversation

@leftibot
Copy link
Copy Markdown
Contributor

Automated fix by @leftibot.

What changed

Fix #143: Remove SystemLink.cmake, use CPM SYSTEM option instead
The SystemLink.cmake module performed complex operations at link-site to
mark imported dependency headers as system includes. This is unnecessary
since CPM supports a SYSTEM option that passes SYSTEM to add_subdirectory,
causing CMake (>= 3.25) to treat all targets from that subdirectory as
system includes natively. All CPMAddPackage calls now use SYSTEM YES,
the target_link_system_libraries call is replaced with plain
target_link_libraries, and SystemLink.cmake is removed entirely.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

Files

 Dependencies.cmake                  | 52 ++++++++++++++++++++---
 ProjectOptions.cmake                |  1 -
 cmake/SystemLink.cmake              | 83 -------------------------------------
 src/ftxui_sample/CMakeLists.txt     |  6 +--
 test/CMakeLists.txt                 | 14 +++++++
 test/check_system_includes.cmake.in | 13 ++++++
 6 files changed, 75 insertions(+), 94 deletions(-)

Closes #143

Triggered by @lefticus.

…ption instead

The SystemLink.cmake module performed complex operations at link-site to
mark imported dependency headers as system includes. This is unnecessary
since CPM supports a SYSTEM option that passes SYSTEM to add_subdirectory,
causing CMake (>= 3.25) to treat all targets from that subdirectory as
system includes natively. All CPMAddPackage calls now use SYSTEM YES,
the target_link_system_libraries call is replaced with plain
target_link_libraries, and SystemLink.cmake is removed entirely.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@lefticus
Copy link
Copy Markdown
Member

@leftibot remove CMake test scripts, that will be confusing to end user, and don't include lefticus tools as a system include

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 20.50%. Comparing base (12e3e37) to head (c5426d7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #145   +/-   ##
=======================================
  Coverage   20.50%   20.50%           
=======================================
  Files           5        5           
  Lines         239      239           
  Branches      108      110    +2     
=======================================
  Hits           49       49           
+ Misses        186      181    -5     
- Partials        4        9    +5     
Flag Coverage Δ
Linux 18.36% <ø> (ø)
Windows 21.02% <ø> (ø)
macOS 18.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…cus tools

Remove check_system_includes.cmake.in and associated test from
test/CMakeLists.txt — these would confuse end users. Remove SYSTEM YES
from the lefticus/tools CPMAddPackage call so its headers are not
treated as system includes.

Requested by @lefticus in PR cpp-best-practices#145 review.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@leftibot
Copy link
Copy Markdown
Contributor Author

Addressed review feedback from @lefticus. New commits:

c5426d7 Address review: remove CMake test scripts, don't SYSTEM-include lefticus tools

@leftibot
Copy link
Copy Markdown
Contributor Author

Hi @lefticus — I tried to address the review feedback but couldn't produce a working change. May need human attention.

@leftibot
Copy link
Copy Markdown
Contributor Author

Hi @lefticus — I tried to address the review feedback but couldn't produce a working change. May need human attention.

@lefticus lefticus merged commit ea9a592 into cpp-best-practices:main Apr 12, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SystemLink Cmake module is unnecessary complexity

2 participants