Kryczkal/cmake modularization 02 by kryczkal · Pull Request #159 · AlkOS-Dev/AlkOS

kryczkal · 2025-08-05T22:27:34Z

Summary

CMakeLists now includes a target to which to link against. It sets all the necessary properties.
Idea is -> there is 1 main property interface library for almost anything.
If arch requires some custom one, it's defined in a .cmake file of the arch, and the existance of this library is a contract between the arch CMakeLists.txt and the arch configuration file. I think this is rather clean.

Important!

Meddling with this stuff can easily introduce some very subtle bugs if a property for some reason isn't applied to a target etc. So I hope the tests will catch if anything like that was introduced. This PR needs to be approached with care for typos / mistakes etc that could subtly break this.

Things done

Renamed x86_64-flags.cmake to x86_64-conf.cmake as i added the property libraries here. Maybe split into two files? But I dont feel there is a need currently.
Updated all CMakeLists.txt to simply link against those interface libraries and removed much redundant code.

#158 Link to previous part of the refactor

…inking

…erties instead of source file flags

…architecture

…andling and consolidating target properties

Copilot

Pull Request Overview

This PR implements CMake modularization by introducing interface property libraries that centralize compilation flags and properties. The goal is to eliminate redundant property definitions across multiple CMakeLists.txt files by creating reusable interface libraries that encapsulate architecture-specific settings.

Key changes:

Created centralized interface property libraries (target.properties and target.properties.32) to manage compilation flags
Replaced repetitive compile option definitions with simple target linking statements
Renamed configuration file from x86_64-flags.cmake to x86_64-conf.cmake to reflect expanded functionality

Reviewed Changes

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

Show a summary per file

File	Description
scripts/config/configure.bash	Updates reference to renamed configuration file
alkos/toolchains/x86_64-conf.cmake	Defines interface property libraries with architecture-specific compilation flags
alkos/CMakeLists.txt	Creates the main `target.properties` interface library
alkos/libc/CMakeLists.txt	Replaces compile options with property library linking
alkos/libc/arch/x86_64/CMakeLists.txt	Links 32-bit target to appropriate property library
alkos/kernel/thirdparty/uacpi/CMakeLists.txt	Simplifies property application using interface library
alkos/kernel/arch/x86_64/loader32/CMakeLists.txt	Modernizes source finding and applies 32-bit properties
alkos/kernel/arch/x86_64/kernel/CMakeLists.txt	Replaces explicit compile options with property library
alkos/kernel/arch/x86_64/common-loader-all/CMakeLists.txt	Streamlines configuration using property libraries
alkos/kernel/arch/x86_64/common-loader-64-kernel/CMakeLists.txt	Simplifies property management
alkos/kernel/CMakeLists.txt	Removes unused POST_ARCH_ACTION functionality

Copilot · 2025-08-05T22:28:03Z

+################################################################################
+
+if (NOT TARGET target.properties)
+  message(FATAL_ERROR "target.properties INTERFACE library is not defined. This should be defined by main CMakeLists.txt")


The error message could be more specific about which CMakeLists.txt file should define the library. Consider: "target.properties INTERFACE library is not defined. This should be defined by the root CMakeLists.txt file."

Suggested change

message(FATAL_ERROR "target.properties INTERFACE library is not defined. This should be defined by main CMakeLists.txt")

message(FATAL_ERROR "target.properties INTERFACE library is not defined. This should be defined by the root CMakeLists.txt file.")

Copilot · 2025-08-05T22:28:04Z


 #################################### Exec ####################################

 add_executable(alkos.loader32


[nitpick] The executable is created without any sources, then sources are added later via alkos_target_sources(). Consider adding sources directly in the add_executable() call for better readability, or add a comment explaining why sources are added separately.

Jlisowskyy

Please ensure that all important flags and configurations are preserved and we are good to go

Jlisowskyy · 2025-08-06T14:47:29Z

+################################################################################
+
+if (NOT TARGET target.properties)
+  message(FATAL_ERROR "target.properties INTERFACE library is not defined. This should be defined by main CMakeLists.txt")


Jlisowskyy · 2025-08-06T14:48:26Z

 include(ValidationHelpers)
 include(SourceHelpers)

+add_library(target.properties INTERFACE) # Populated by the toolchain file


Is it possible to move it to the conf file?

Jlisowskyy · 2025-08-06T14:52:11Z


-target_compile_options(uacpi PRIVATE
-        "$<$<COMPILE_LANGUAGE:CXX>:-mcmodel=kernel>"
-        "$<$<COMPILE_LANGUAGE:C>:-mcmodel=kernel>"
+target_link_libraries(uacpi PRIVATE
+        target.properties
 )


Are you sure passing additional flags to external lib will work without issues in future?

### 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 ---------

# Summary Refactored the passing of a ton of arguments from arch to top level cmake using a function to register arch. This is cleaner because it makes abi explicit while removing any magic "data flows" from bottom cmake to top down. Also moved linker flags to .conf file, making targets inherit them instead of repeating for each exe. # Note The scripts are becomming a mess. I don't care. The scripts are next to be shot after cmake. #159 Prev --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

# Summary CMakeLists now includes a target to which to link against. It sets all the necessary properties. Idea is -> there is 1 main property interface library for almost anything. If arch requires some custom one, it's defined in a .cmake file of the arch, and the existance of this library is a contract between the arch CMakeLists.txt and the arch configuration file. I think this is rather clean. # Important! Meddling with this stuff can easily introduce some very subtle bugs if a property for some reason isn't applied to a target etc. So I hope the tests will catch if anything like that was introduced. This PR needs to be approached with care for typos / mistakes etc that could subtly break this. # Things done - Renamed x86_64-flags.cmake to x86_64-conf.cmake as i added the property libraries here. Maybe split into two files? But I dont feel there is a need currently. - Updated all CMakeLists.txt to simply link against those interface libraries and removed much redundant code. #158 Link to previous part of the refactor

### 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 ---------

# Summary Refactored the passing of a ton of arguments from arch to top level cmake using a function to register arch. This is cleaner because it makes abi explicit while removing any magic "data flows" from bottom cmake to top down. Also moved linker flags to .conf file, making targets inherit them instead of repeating for each exe. # Note The scripts are becomming a mess. I don't care. The scripts are next to be shot after cmake. #159 Prev --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

kryczkal added 5 commits August 6, 2025 00:03

refactor: Move compile options to target.properties interface library

ae9ce33

refactor: Remove post-arch action and consolidate target properties l…

18aca3f

…inking

refactor: Update 32-bit loader CMake configuration to use target prop…

a0c6862

…erties instead of source file flags

refactor: Simplify source file handling in CMakeLists.txt for x86_64 …

196a7ad

…architecture

refactor: Simplify CMakeLists.txt by removing redundant source file h…

8d6ada8

…andling and consolidating target properties

Copilot AI review requested due to automatic review settings August 5, 2025 22:27

Copilot AI reviewed Aug 5, 2025

View reviewed changes

kryczkal linked an issue Aug 5, 2025 that may be closed by this pull request

Some mechanism to set properties of files based on arch (a recurring #162

Closed

kryczkal added the improvement Improvement to existing code label Aug 5, 2025

kryczkal mentioned this pull request Aug 6, 2025

Kryczkal/cmake modularization 03 #169

Merged

Jlisowskyy approved these changes Aug 6, 2025

View reviewed changes

kryczkal mentioned this pull request Aug 7, 2025

Kryczkal/cmake modularization 04 #170

Merged

kryczkal merged commit 93935fb into dev Aug 7, 2025
5 of 8 checks passed

kryczkal deleted the kryczkal/cmake-modularization-02 branch August 7, 2025 14:29

kryczkal restored the kryczkal/cmake-modularization-02 branch August 7, 2025 14:33

Jlisowskyy deleted the kryczkal/cmake-modularization-02 branch August 8, 2025 00:28

Navigation Menu

Kryczkal/cmake modularization 02 - #159

Kryczkal/cmake modularization 02#159
kryczkal merged 5 commits into
devfrom
kryczkal/cmake-modularization-02

kryczkal commented Aug 5, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 5, 2025

Uh oh!

Jlisowskyy Aug 6, 2025

Uh oh!

Copilot AI Aug 5, 2025

Uh oh!

Jlisowskyy left a comment

Uh oh!

Jlisowskyy Aug 6, 2025

Uh oh!

Jlisowskyy Aug 6, 2025

Uh oh!

Jlisowskyy Aug 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	message(FATAL_ERROR "target.properties INTERFACE library is not defined. This should be defined by main CMakeLists.txt")
	message(FATAL_ERROR "target.properties INTERFACE library is not defined. This should be defined by the root CMakeLists.txt file.")


		#################################### Exec ####################################

		add_executable(alkos.loader32

Conversation

kryczkal commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Important!

Things done

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Jlisowskyy Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Jlisowskyy left a comment

Choose a reason for hiding this comment

Uh oh!

Jlisowskyy Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Jlisowskyy Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Jlisowskyy Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kryczkal commented Aug 5, 2025 •

edited

Loading