Skip to content

Kryczkal/cmake modularization 03#169

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

Kryczkal/cmake modularization 03#169
kryczkal merged 30 commits intodevfrom
kryczkal/cmake-modularization-03

Conversation

@kryczkal
Copy link
Copy Markdown
Member

@kryczkal kryczkal commented Aug 5, 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

@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
Copy link
Copy Markdown
Member Author

kryczkal commented Aug 6, 2025

Tests failed, working on it

@kryczkal
Copy link
Copy Markdown
Member Author

kryczkal commented Aug 6, 2025

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
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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

@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
Copy link
Copy Markdown
Collaborator

@Jlisowskyy Jlisowskyy left a comment

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
Collaborator

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
Collaborator

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