Skip to content

Conversation

davidlion
Copy link
Member

@davidlion davidlion commented Sep 25, 2025

Description

This PR changes the install scripts from include_guard(GLOBAL) to include_guard(). The GLOBAL flag unnecessarily applies the guard to an entire project calling find_package. Without the flag, ystdlib can be found and used in multiple independent scopes successfully.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Summary by CodeRabbit

  • Refactor
    • Standardized CMake include guards across library configuration packages by switching to the default scope for more consistent behaviour.
  • Chores
    • Cleaned up build configuration to improve consistency and predictability when integrating with downstream CMake projects.

…ackage calls from different scopes to succeed.
@davidlion davidlion requested a review from a team as a code owner September 25, 2025 19:22
Copy link

coderabbitai bot commented Sep 25, 2025

Walkthrough

All four CMake config templates in cmake/ystdlib/libs switched include_guard(GLOBAL) to include_guard(). Other includes remain unchanged.

Changes

Cohort / File(s) Summary
Include guard scope adjustment (CMake config templates)
cmake/ystdlib/libs/containers-config.cmake.in, cmake/ystdlib/libs/error_handling-config.cmake.in, cmake/ystdlib/libs/io_interface-config.cmake.in, cmake/ystdlib/libs/wrapped_facade_headers-config.cmake.in
Replace include_guard(GLOBAL) with include_guard(); retain existing include lines (…-target.cmake and, where applicable, wrapped_facade_headers-config.cmake).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title clearly and concisely summarizes the primary change of switching include_guard from global to current scope and explains the purpose of enabling find_package calls across different scopes while referencing the related issue.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@davidlion davidlion changed the title fix(cmake): Change include_guards from the global scope to the current scope, to enable find_package calls from different scopes to succeed (fixes #74). fix(cmake): Change include_guards from the global scope to the current scope, to enable find_package calls from different scopes to succeed (fixes #74). Sep 25, 2025
Copy link

@gibber9809 gibber9809 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I also previously tested that this change fixes things for the approach taken by this CLP PR.

@davidlion davidlion merged commit c03806a into y-scope:main Sep 25, 2025
13 of 14 checks passed
@davidlion davidlion deleted the inlucde-fix branch September 25, 2025 22:02
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.

2 participants