Major build-system refactor, adding conan & cleaner CMake scripting [ci patch inc]#18
Conversation
|
I'm getting to work. See my steps and track my cost here.
|
|
bot is going to die, we need to patch CI for this one |
Build System Refactor ReviewThanks for the PR! I've reviewed the changes and have some comments about the refactoring work. OverviewThis is a substantial modernization of the build system with:
Issues that need addressingCMake ConfigurationThere's a bug in the LTO configuration: if(result)
set_property(TARGET foo PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
message(STATUS "HCPU: Enabled lTO")This is setting IPO on a non-existent target named "foo". This should probably be applied to all targets or specific ones after they are defined. Using a global/project property might be better here. Build Script IssuesIn def symlink_compile_commands(self: 'Routines') -> None:
if 'Debug' in self.__params.build_configs:
path = self.__params.build_directory.joinpath('Debug').joinpath('compile_commands.json')
logger.info(f'creating symlink for path \'{path}\'')
if 'Release' in self.__params.build_configs:
path = self.__params.build_directory.joinpath('Release').joinpath('compile_commands.json')
logger.info(f'creating symlink for path \'{path}\'')
if path is None:
raise ValueError('no supported CMake configs detected')If neither 'Debug' nor 'Release' are in build_configs, CI WorkflowsAll the CI workflows will need updating since they:
For each workflow, you'll need to:
FIXME CommentsThere are several
Nice Improvements
Overall, this is a solid refactoring of the build system that will improve developer experience. Once the issues above are addressed, this will be ready to merge. |
|
also we need:
|
PR Review UpdateThanks for fixing the issues with the LTO configuration and the symlink_compile_commands function! Based on my analysis and your comment, here are the remaining tasks needed to complete this PR: 1. CI WorkflowsAll CI workflows still use the old build system approach and won't work with the Conan-based setup. Each workflow file (
2. Versioning SystemThe version increment scripts (
3. Benchmarking SetupThe current include_directories(${HyperCPU_SOURCE_DIR}/src)
add_subdirectory(${HyperCPU_SOURCE_DIR}/dist/benchmark dist/benchmark)
# ...
target_include_directories(memory_fetch_bench_no PUBLIC ${ROOT_DIR}/src/Emulator)This needs to be updated to work with the Conan-sourced dependencies and the new CMake structure. 4. Library PatchesAs you mentioned, several libraries need patches:
5. DocumentationIt would be helpful to add documentation about:
I notice that HCPU_BUILD_BENCHMARKS is OFF by default - is this intentional? This means that benchmarks won't be built unless explicitly enabled. This is a significant refactoring, so these remaining tasks are understandable. Would you like me to help with any specific part of the remaining work? |
|
Can you also add on.push.branches: master trigger into docker-autoupdate workflow? Overall it looks okay, but there are some conflicts. And workflows are not detected, I can't run CI jobs. |
Integrate pog submodule into the source tree
Repo generic changes:
CMake:
Conan:
Patched CI to match new buildsystem. Also need to work on pch and python tests later. Need to patch Hpool.