Skip to content

build: use PROJECT_IS_TOP_LEVEL for the pkgconfig top-level guard - #348

Closed
bobluppes wants to merge 1 commit into
mainfrom
cmake-project-is-top-level
Closed

bobluppes wants to merge 1 commit into
mainfrom
cmake-project-is-top-level

Conversation

@bobluppes

Copy link
Copy Markdown
Owner

Why

#312 fixed the FetchContent crash from #310 by skipping the pkgconfig install block unless Graaf is the top-level project, via CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR. CMake 3.21+ exposes exactly this as the built-in PROJECT_IS_TOP_LEVEL variable, which is clearer and self-documenting -- but this project's floor is CMake 3.11, so it isn't guaranteed to be defined.

What Changed

Defines PROJECT_IS_TOP_LEVEL ourselves (matching CMake's own semantics) when it isn't already set by CMake, and uses it for the pkgconfig guard instead of the raw path comparison. No behavior change; same guard, clearer name, and reusable if more top-level-only logic gets added later.

Verification

Built and installed locally in both scenarios:

#312 introduced a CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR check to
skip installing pkgconfig files when Graaf is consumed via FetchContent.
CMake 3.21+ exposes this as PROJECT_IS_TOP_LEVEL directly; since this
project's floor is 3.11, define it ourselves when CMake doesn't. Same
behavior, clearer intent, and reusable for future top-level-only logic.
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.58%. Comparing base (4b0ad5a) to head (8fd7055).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #348   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          59       59           
  Lines        2622     2622           
  Branches      136      136           
=======================================
  Hits         2611     2611           
  Misses         11       11           

☔ View full report in Codecov by Harness.
📢 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.

@bobluppes

Copy link
Copy Markdown
Owner Author

Closing this — on reflection there's a real gap: my verification only ran on CMake 4.4.3, so the CMake<3.21 fallback branch (guarded by if(NOT DEFINED PROJECT_IS_TOP_LEVEL)) never actually executed, since modern CMake already defines the variable before that check runs. Worse, that guard has a real bug: plain CMake variables are inherited into subdirectories, so if a consuming project already sets PROJECT_IS_TOP_LEVEL for itself, that value would leak into Graaf's scope and the fallback would skip recomputing it for Graaf specifically. Not worth pursuing further -- the CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR check from #312 stays as the correct, version-portable approach.

@bobluppes bobluppes closed this Sep 1, 2026
@bobluppes
bobluppes deleted the cmake-project-is-top-level branch September 1, 2026 19:03
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.

1 participant