Skip to content

Major build-system refactor, adding conan & cleaner CMake scripting [ci patch inc]#18

Merged
AshFungor merged 28 commits intoHyperCPU-Project:masterfrom
AshFungor:buildsystem
May 1, 2025
Merged

Major build-system refactor, adding conan & cleaner CMake scripting [ci patch inc]#18
AshFungor merged 28 commits intoHyperCPU-Project:masterfrom
AshFungor:buildsystem

Conversation

@AshFungor
Copy link
Contributor

@AshFungor AshFungor commented Apr 21, 2025

Repo generic changes:

  • added clang-format (derived from LLVM style)
  • added flake8 (sort of custom)
  • updated gitignore to ignore more (Qt-es, more object, compiled files etc.)

CMake:

  • added ctest
  • removed cmake/ for now, moved all configuration to main file
  • added pch target with precompiled headers

Conan:

  • added conan support, removed most of the dist
  • added build sctipt + tests for it

Patched CI to match new buildsystem. Also need to work on pch and python tests later. Need to patch Hpool.

@mentatbot
Copy link

mentatbot bot commented Apr 21, 2025

I'm getting to work. See my steps and track my cost here.

  • Wake on any new activity.
    I always see new activity while actively working. When paused, I'll only respond when tagged unless this box is checked.

@AshFungor
Copy link
Contributor Author

bot is going to die, we need to patch CI for this one

@mentatbot
Copy link

mentatbot bot commented Apr 21, 2025

Build System Refactor Review

Thanks for the PR! I've reviewed the changes and have some comments about the refactoring work.

Overview

This is a substantial modernization of the build system with:

  • Migration from git submodules to Conan for dependency management
  • Better structured CMake configuration
  • New Python build helper script
  • Code style configuration files

Issues that need addressing

CMake Configuration

There'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 Issues

In scripts/build.py the symlink_compile_commands method has a potential bug:

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, path will be undefined when checking if it's None. You should initialize path = None at the beginning of the method.

CI Workflows

All the CI workflows will need updating since they:

  1. Use the old -DHCPU_COMPILER flag which has been removed
  2. Rely on git submodules instead of Conan
  3. Don't install or configure Conan
  4. Don't pass the new CMake options

For each workflow, you'll need to:

  1. Add steps to install Conan
  2. Configure Conan profiles
  3. Update the CMake commands to use the new options
  4. Use the new build script if possible

FIXME Comments

There are several FIXME comments in the code. Could you clarify the status of these?

  • Are they expected to be fixed in this PR?
  • Are they cleanup tasks for later?
  • Do any of them represent blockers for merging?

Nice Improvements

  1. The code style configuration files (.clang-format and .flake8) will help maintain consistent style
  2. Moving to Conan for dependency management is a good modernization step
  3. The build.py script offers a nice unified interface for building

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.

@AshFungor
Copy link
Contributor Author

also we need:

  • enable bench
  • patch hpool
  • patch pog
  • patch autoincrement & add it to conan & cmake versions

@HyperWinX
Copy link
Member

@MentatBot

@mentatbot
Copy link

mentatbot bot commented Apr 21, 2025

PR Review Update

Thanks 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 Workflows

All CI workflows still use the old build system approach and won't work with the Conan-based setup. Each workflow file (.github/workflows/*.yml) needs updating to:

  • Install Conan
  • Use the new build options instead of -DHCPU_COMPILER=gcc/clang
  • Remove submodule checkout or make it conditional
  • Possibly use the new build.py script

2. Versioning System

The version increment scripts (scripts/increment_*.sh) need to update:

  • src/Version.hpp (as they do now)
  • README.md badges (as they do now)
  • conanfile.py version information
  • CMake project version if applicable

3. Benchmarking Setup

The current bench/CMakeLists.txt references paths and variables that will not exist in the new setup:

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 Patches

As you mentioned, several libraries need patches:

  • hpool: To enable -Werror (mentioned in a FIXME comment)
  • pog: For compatibility with the new system
  • Auto-increment system: To integrate with Conan and CMake versioning

5. Documentation

It would be helpful to add documentation about:

  • How to set up Conan for development
  • Workflow for adding new dependencies
  • Build options and how to use the build.py script

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?

@AshFungor
Copy link
Contributor Author

@MentatBot

@HyperWinX
Copy link
Member

@MentatBot

@HyperWinX
Copy link
Member

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.

@HyperCPU-Project HyperCPU-Project deleted a comment from mentatbot bot Apr 22, 2025
@HyperCPU-Project HyperCPU-Project deleted a comment from mentatbot bot Apr 22, 2025
@AshFungor AshFungor merged commit 941e288 into HyperCPU-Project:master May 1, 2025
1 of 9 checks passed
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.

2 participants

Comments