Skip to content

Kryczkal/cmake modularization 03 - #169

Merged
kryczkal merged 30 commits into
devfrom
kryczkal/cmake-modularization-03
Aug 10, 2025
Merged

kryczkal merged 30 commits into
devfrom
kryczkal/cmake-modularization-03

Conversation

@kryczkal

@kryczkal kryczkal commented Aug 5, 2025

Copy link
Copy Markdown
Member

CMake Refactoring Summary

This update modernizes the CMake build system by replacing PARENT_SCOPE variable passing with INTERFACE library targets, which act as central hubs for properties and dependencies.

Key Improvements:

  • Centralized Configuration:

    • The alkos.kernel.config INTERFACE library is now a central data bus.
    • Architecture-specific files set their properties directly on this target.
    • The top-level CMakeLists.txt queries this target, eliminating complex variable management.
    • The alkos.kernel.deps Intreface library was added as a public target to for others to link to. This was supposed to be a fasade to hide the custom linking order logic from other modules. (The .deps lib would just be linked in the proper place by kernel). This is currently USELESS. It was important when I tried to move towards a target_link_libraries approach, but I failed. This will be important again if I try again. For now it doesn't hurt so let it stay. I may retry some time in the future to avoid the hardcoded crti.
  • Simplified Linker Logic:

    • Removed the fragile override of CMAKE_CXX_LINK_EXECUTABLE.
    • crti and crtn are now OBJECT libraries, and their paths are queried dynamically.
    • The kernel executable links these objects in the correct order using the standard target_link_libraries.
  • Streamlined Dependencies:

    • The alkos.kernel.thirdparty INTERFACE library aggregates all third-party dependencies like uACPI.
    • The main kernel links against this single target to pull in all necessary third-party code.
  • Enhanced Helper Functions:

    • Added alkos_ensure_property_defined for robust property validation.
    • Improved alkos_ensure_defined to handle both undefined and empty variables.
  • Fixed Bug in CI/CD Pipeline

  • Pipeline configured alkos before installing toolchain. This somehow worked. But after my changes to cmake, this doesn't work, so I reorderd it to fix it.

Failures

I tried to refactor the crti crtn logic. I failed. I was able to get to a point where the crti objects are automatically found by cmake (no hardcoded paths), and I was able to link it using target_link_libraries. But i have no idea if the ordering was ensured. Kernel compiled but no tests were detected. This is too much work for too little gain. I left the hardcoded paths to be there and simply upgraded their propagation to the .config library.

#159 Previous part

@kryczkal kryczkal added the improvement Improvement to existing code label Aug 5, 2025
@kryczkal kryczkal linked an issue Aug 6, 2025 that may be closed by this pull request
@kryczkal
kryczkal changed the base branch from dev to kryczkal/cmake-modularization-02 August 6, 2025 00:16
@kryczkal
kryczkal marked this pull request as ready for review August 6, 2025 00:25
@kryczkal

kryczkal commented Aug 6, 2025

Copy link
Copy Markdown
Member Author

Tests failed, working on it

@kryczkal

kryczkal commented Aug 6, 2025

Copy link
Copy Markdown
Member Author

Introduced a bug where no tests are detected. Working on it

@kryczkal
kryczkal requested a review from Copilot August 6, 2025 00:38

This comment was marked as outdated.

@kryczkal
kryczkal requested a review from Copilot August 6, 2025 14:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modernizes the CMake build system by replacing variable propagation through PARENT_SCOPE with INTERFACE library targets that serve as central configuration hubs. The refactoring centralizes build configuration through the alkos.kernel.config target and consolidates third-party dependencies under alkos.kernel.thirdparty.

  • Replaced PARENT_SCOPE variable passing with property-based configuration through INTERFACE targets
  • Consolidated third-party dependencies into a single alkos.kernel.thirdparty target
  • Modernized CRT object file handling while maintaining the existing linking order

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
alkos/kernel/CMakeLists.txt Main kernel configuration refactored to use properties and simplified CRT linking
alkos/kernel/arch/x86_64/CMakeLists.txt Architecture variables converted to target properties instead of parent scope
alkos/kernel/arch/x86_64/kernel/CMakeLists.txt CRT object creation split into separate targets and paths set as properties
alkos/kernel/thirdparty/CMakeLists.txt Third-party library aggregation through interface target
alkos/cmake/ValidationHelpers.cmake Added property validation function and improved variable checking
alkos/CMakeLists.txt Top-level configuration retrieval from properties instead of variables
.github/actions/prepare_env/action.yaml Fixed CI pipeline ordering to install toolchain before configuration
scripts/env/toolchain_versions.txt Updated toolchain versions
Comments suppressed due to low confidence (2)

scripts/env/toolchain_versions.txt:1

  • binutils version 2.44 does not exist. The latest stable release is 2.43.1. Consider using an existing version.
binutils=2.44

scripts/env/toolchain_versions.txt:3

  • GCC version 15.1.0 does not exist. The latest stable release is 14.2.0. Consider using an existing version.
gcc=15.1.0

Comment thread alkos/cmake/ValidationHelpers.cmake
Comment thread alkos/kernel/CMakeLists.txt Outdated
@kryczkal
kryczkal requested a review from Jlisowskyy August 6, 2025 14:07
kryczkal and others added 3 commits August 6, 2025 16:12
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@kryczkal
kryczkal deleted the branch dev August 7, 2025 14:29
@kryczkal kryczkal closed this Aug 7, 2025
@kryczkal kryczkal reopened this Aug 7, 2025
@kryczkal
kryczkal changed the base branch from kryczkal/cmake-modularization-02 to dev August 7, 2025 14:33
@kryczkal kryczkal linked an issue Aug 9, 2025 that may be closed by this pull request

@Jlisowskyy Jlisowskyy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Comment on lines +76 to +78
# TODO: Immediately here, error checking should happen
# But the error checking itself fails!!! and for some reason
# alkos_ensure_property_defined() says the property is not defined

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

create issue?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems important

@kryczkal
kryczkal merged commit cd17da4 into dev Aug 10, 2025
4 checks passed
@kryczkal
kryczkal deleted the kryczkal/cmake-modularization-03 branch August 10, 2025 20:45
@kryczkal
kryczkal restored the kryczkal/cmake-modularization-03 branch August 10, 2025 20:45
@Jlisowskyy
Jlisowskyy deleted the kryczkal/cmake-modularization-03 branch August 13, 2025 22:04
kryczkal added a commit that referenced this pull request Oct 22, 2025
### CMake Refactoring Summary

This update modernizes the CMake build system by replacing
`PARENT_SCOPE` variable passing with `INTERFACE` library targets, which
act as central hubs for properties and dependencies.

**Key Improvements:**

*   **Centralized Configuration:**
* The `alkos.kernel.config` `INTERFACE` library is now a central data
bus.
* Architecture-specific files set their properties directly on this
target.
* The top-level `CMakeLists.txt` queries this target, eliminating
complex variable management.
* The `alkos.kernel.deps` Intreface library was added as a public target
to for others to link to. This was supposed to be a fasade to hide the
custom linking order logic from other modules. (The .deps lib would just
be linked in the proper place by kernel). This is currently USELESS. It
was important when I tried to move towards a target_link_libraries
approach, but I failed. This will be important again if I try again. For
now it doesn't hurt so let it stay. I may retry some time in the future
to avoid the hardcoded crti.

*   **Simplified Linker Logic:**
    *   Removed the fragile override of `CMAKE_CXX_LINK_EXECUTABLE`.
* `crti` and `crtn` are now `OBJECT` libraries, and their paths are
queried dynamically.
* The kernel executable links these objects in the correct order using
the standard `target_link_libraries`.

*   **Streamlined Dependencies:**
* The `alkos.kernel.thirdparty` `INTERFACE` library aggregates all
third-party dependencies like `uACPI`.
* The main kernel links against this single target to pull in all
necessary third-party code.

*   **Enhanced Helper Functions:**
* Added `alkos_ensure_property_defined` for robust property validation.
* Improved `alkos_ensure_defined` to handle both undefined and empty
variables.

* **Fixed Bug in CI/CD Pipeline**
* Pipeline configured alkos before installing toolchain. This somehow
worked. But after my changes to cmake, this doesn't work, so I reorderd
it to fix it.
 
 # Failures
I tried to refactor the crti crtn logic. I failed. I was able to get to
a point where the crti objects are automatically found by cmake (no
hardcoded paths), and I was able to link it using target_link_libraries.
But i have no idea if the ordering was ensured. Kernel compiled but no
tests were detected. This is too much work for too little gain. I left
the hardcoded paths to be there and simply upgraded their propagation to
the .config library.

#159 Previous part

---------
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement to existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Manage kernel modules and boot executables via target properties Manage PARENT_SCOPE with interface libraries

3 participants