Skip to content

Conversation

Bill-hbrhbr
Copy link
Contributor

@Bill-hbrhbr Bill-hbrhbr commented Sep 29, 2025

Description

Following:

This PR renames the remaining CMake variables that referenced the old deps directory layout so they accurately reflect the new build/deps/cpp/cmake-settings/all-core.cmake structure.

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

  • Variable renaming only.

Summary by CodeRabbit

  • New Features

    • No user-facing changes in this release.
  • Refactor

    • Standardized internal build configuration variable names.
    • Aligned dependency search paths to use a unified convention.
  • Chores

    • Updated build scripts to improve consistency in dependency resolution.
    • Maintained policy handling with no impact on external behaviour.

@Bill-hbrhbr Bill-hbrhbr requested a review from a team as a code owner September 29, 2025 16:39
Copy link
Contributor

coderabbitai bot commented Sep 29, 2025

Walkthrough

Renames CMake variables from CLP_CORE_DEPS_DIR to CLP_DEPS_CPP_DIR and updates related include and conditional references in core build scripts and the FindLibLZMA module. Adjusts PKG_CONFIG_PATH construction accordingly. No changes to exported/public entities or external behaviour.

Changes

Cohort / File(s) Summary
CMake vars/path updates (core build)
components/core/CMakeLists.txt
Renamed CLP_CORE_DEPS_DIR → CLP_DEPS_CPP_DIR; updated include path; renamed RESULT_VARIABLE to CLP_DEPS_CPP_CORE_SETTINGS_FILE_PATH; updated conditional checks accordingly; CMP0144 handling continues using updated path var.
CMake find-module env path update
components/core/cmake/Modules/FindLibLZMA.cmake
Replaced CLP_CORE_DEPS_DIR with CLP_DEPS_CPP_DIR in PKG_CONFIG_PATH setup and restoration to influence pkg-config resolution during find.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarises the main change of renaming CMake dependency variables to match the updated directory layout, uses a conventional commit prefix, and is concise and specific enough for readers to understand the primary intent without extraneous detail.
✨ 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.

@Bill-hbrhbr Bill-hbrhbr changed the title chore(cmake): Rename variables related to core cpp dependencies to reflect their attributes more appropriately. chore(cmake): Rename cpp core dependency variables to match new directory structure. Sep 29, 2025
Copy link
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

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

Other all lgtm. The only question is: shouldn't CLP_CPP_DEPS_DIR make more sense? I understand CLP_DEPS_CORE_DIR would match the hierarchical structure of the actual directory, but it reads a bit weird though.

@Bill-hbrhbr
Copy link
Contributor Author

Other all lgtm. The only question is: shouldn't CLP_CPP_DEPS_DIR make more sense? I understand CLP_DEPS_CORE_DIR would match the hierarchical structure of the actual directory, but it reads a bit weird though.

I think I want to match how it's done in taskfiles, e.g.

G_DEPS_CPP_DIR: "{{.G_DEPS_DIR}}/cpp"

Before your PR I always wanted to change the original CLP_CORE_DEPS_DIR to CLP_DEPS_CORE_DIR but found that a bit unnecessary. Now that we've renamed the deps structure, I think it's worthwhile again.

In short, want to align CMake variable naming with Task.

Copy link
Member

@LinZhihao-723 LinZhihao-723 left a comment

Choose a reason for hiding this comment

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

lgtm. For the PR title, how about:

refactor(cmake): Rename CMake dependency variables to align with the latest dependency directory layout.

@Bill-hbrhbr Bill-hbrhbr changed the title chore(cmake): Rename cpp core dependency variables to match new directory structure. chore(cmake): Rename CMake dependency variables to align with the latest dependency directory layout. Oct 1, 2025
@Bill-hbrhbr Bill-hbrhbr changed the title chore(cmake): Rename CMake dependency variables to align with the latest dependency directory layout. refactor(cmake): Rename CMake dependency variables to align with the latest dependency directory layout. Oct 1, 2025
@Bill-hbrhbr Bill-hbrhbr merged commit d25f16b into y-scope:main Oct 1, 2025
31 of 32 checks passed
@Bill-hbrhbr Bill-hbrhbr deleted the rename-deps-core-var branch October 1, 2025 07:48
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.

3 participants