Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup cmake files #394

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Cleanup cmake files #394

merged 3 commits into from
Jun 18, 2024

Conversation

makxenov
Copy link
Contributor

@makxenov makxenov commented Jun 6, 2024

I would like to consider this PR as a place for discussions first. I feel like our cmake files in crypto3/blueprint/zkllvm require rework. Here are some points for it:

  • Files contain a lot of unused code. Huge amount of it was removed from crypto3 by crypto3#242 and crypto3#233. But there are still unused or weird parts, like cmake_policy.
  • Seems like a lot of projects were initialized as a copy-paste cmake configuration from some existing one. That's the reason why we can see repetitive errors across the projects, and a bunch of unused files like TargetArchitecture.cmake, CheckAVX.cmake and so on.
  • Boost CMake modules used in each project are overcomplicated and non-intuitive. Their macros set some variables that later used outside the macro scope, which is hard to read and debug. Some of them are just incorrect, like cm_find_package, that is full of workarounds.
  • Statements like add_library(${CMAKE_WORKSPACE_NAME}_${CURRENT_PROJECT_NAME} INTERFACE) blur eyes, this customizability is excessive. add_library(crypto3::blueprint INTERFACE) is much better.

I think that significant part of current cmake configuration of blueprint could be removed. Also it makes sense to replace usage of Boost cmake modules (expect probably CMTest) with standard cmake library functions. The modules were intended for Boost, they are too complex for us and have bugs. And standard functions have better documentation and tons of usecases/troubleshooting in public access.

Below I added comments on some removed/replaced parts. I invite you all to discuss this points here. Hope we will get a common understanding of how cmake files should look like and then propagate it to other repos.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
Copy link

github-actions bot commented Jun 6, 2024

Linux Test Results

 57 files   57 suites   3m 37s ⏱️
268 tests 267 ✅ 1 💤 0 ❌
295 runs  294 ✅ 1 💤 0 ❌

Results for commit 00eb672.

♻️ This comment has been updated with latest results.

CMakeLists.txt Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@akokoshn akokoshn left a comment

Choose a reason for hiding this comment

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

What about CI fails?, expected?

@makxenov
Copy link
Contributor Author

What about CI fails?, expected?

blueprint_zkevm_zkevm_word_test was failed, introduced in this commit. @Iluvmagick @ETatuzova please fix and please do not merge anything without CI passed.

@Iluvmagick
Copy link
Contributor

What about CI fails?, expected?

blueprint_zkevm_zkevm_word_test was failed, introduced in this commit. @Iluvmagick @ETatuzova please fix and please do not merge anything without CI passed.

This test is broken due to goldilocks field not working as expected; a known issue in multiprecision.
I've decided to not hide the test, although we can comment it out.

@makxenov
Copy link
Contributor Author

makxenov commented Jun 13, 2024

@Iluvmagick I've disabled the failing test, now we can see 1 skipped test in the test summary. Also I've removed crypto3 revision from flake.nix file, it is better to use nix flake update rather then changing the file manually.

CMakeLists.txt Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@makxenov makxenov merged commit 1d9cac3 into master Jun 18, 2024
3 checks passed
@makxenov makxenov deleted the cleanup-cmake branch June 18, 2024 12:08
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.

8 participants