Alternative cmake build system #2089

alaindargelas · 2023-10-22T04:10:39Z

No description provided.

alaindargelas · 2023-10-22T17:37:43Z

alaindargelas · 2023-10-23T17:29:44Z

@kgugala, please merge tomorrow if no review from anyone.
I'm using this in RapidSilicon main build system and maintain a patch until this is merged.

makefile.txt

pgielda · 2023-10-23T19:02:20Z

Just FYI @alaindargelas we will not merge without a review, nothing should be merged without a proper review. We will most likely review in the period of the next few days.

CMakeLists.txt

cmake-makefile

kgugala · 2023-10-23T18:58:59Z

cmake-makefile

+	undefine LC_ALL
+endif
+
+ifeq ($(CPU_CORES),)


this block looks weird. I suppose it is here because you wrapped cmake in make so we have the following flow:

we call make, which is calling cmake to generate makefiles, and then make is calling cmake to call make to build the project

can't we simply call cmake directly? or create a build script calling cmake?

This is in Surelog and UHDM too, the make is a helper so developers don't have to type a cmake command line.

I proposed a build script - it will be convenient for devs and will not over complicate the flow

I don't see the complication.
Developers type:

make -f cmake-makefile

instead of the complex cmake command line while developing.
This has worked fine in Surelog and UHDM, where the command is simply "make"
In this case it has to be make -f cmake-makefile since there is another Makefile.

I removed all the commented out code. I'll let the package maintainers bring it back if they need it.

@kgugala , can you add @timkpaine as a contributor as he might want to add-on to that CMakeLists.txt

cmake-makefile

kgugala · 2023-10-23T19:09:27Z

CMakeLists.txt

+# In cmake it is imposible to read the output of yosys-config and populate variables at cmake configuration time as yosys is built in this cmake
+# The initial content of YOSYS_CONFIG_FLAGS is a bootstrap that enables to build the synlig code
+# 
+set(YOSYS_CONFIG_FLAGS "-MD -MP -D_YOSYS_ -fPIC -Os -DYOSYS_ENABLE_READLINE -DYOSYS_ENABLE_PLUGINS -DYOSYS_ENABLE_GLOB -DYOSYS_ENABLE_ZLIB -I/usr/local/include -I/usr/include/tcl8.6 -DYOSYS_ENABLE_TCL -DYOSYS_ENABLE_ABC -DYOSYS_ENABLE_COVER")


I don't like those hardcoded values. This cmake should take care of those and force setting them correctly for both Yosys and Synlig build, or calculate this during the build (not build scripts generation) by calling yosys-config

Please read the comment above the flags. yosys-config cannot be called from Cmake, during cmake configuration, as cmake has not built yosys yet.
This only applies for the local synlig build.

When synlig is embedded in a larger system that builds Yosys prior, then the $YOSYS_CONFIG is passed from above and and the hardcoded flags are not used.
This has been fully validated in a complex cmake build where synlig is just a component.

To be noted that the current Makefile build system of synlig is impossible to integrate in a larger build context where Yosys is not the one vendored by synlig.

This cmake fixes that.

it cannot be called at the build files generation - this is true, but during the build you can call it (and you should if the flags are not provided). Otherwise we risk a situation when different parts of the lib will be build with different flags.

I understand that you use it with specific settings, but the code in this repo should be generic and usable by any users

BTW: "or calculate this during the build (not build scripts generation) by calling yosys-config" is impossible to achieve in Cmake.

yosys-config was just not though through to be integrated in the pure cmake build.
It needs some pre-build happening outside of cmake.

I pushed a new change:
set(YOSYS_CONFIG_FLAGS "-D_YOSYS_")

The only hardcoded flag is now: -D_YOSYS_, this flag is never going to change in Yosys. No need to fear this: "avoid a situation where the default build flags in Yosys change and we'll have them hardcoded for Synlig"

We don't need any of the convoluted solutions above. We have a self-contained Cmake that does all the job.
Just to make sure you understand, this flag is used to compile the synlig code, not the Yosys code.
Both synlig and Yosys are built concurently by cmake.

Opentitan fails on its own with the latest run. This PR is bringing no changes from Surelog whatsoever.

@kgugala are we done addressing your concerns? Can we get this merged?

Can you stop pinging maintainers every 5 minutes? We are in a different timezone and also this is not the only thing we do, and also we are not online 24/7 to participate in discussions. As for OpenTitan -- there is a fix for OpenTitan waiting for the CI to be green. Please be patient.
As for buildsystem -- definitely there are still concerns, also its not that we planned to have 5 different buildsystems. You can maintain your own buildsystem in your own repo for now while we analyze if this should go in to Synlig... and definitely not in the form it is now...
Again the only system we officially support is Debian.

The current build system is inadequate for a lot of the use cases. I'll maintain (and others) this cmake based build system.

kgugala · 2023-10-26T09:52:10Z

CMakeLists.txt

+      COMMAND make CONFIG=gcc PREFIX=${INSTALL_DIR} install -j${CPU_CORES}
+
+      WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}/third_party/yosys"
+      DEPENDS ${PROJECT_SOURCE_DIR}/third_party/yosys/kernel/yosys.cc


maybe we can do sth like here iree-org/iree#6434 and check for the required submodules and inform the users if they are not.

Done:
if(NOT EXISTS "${PROJECT_SOURCE_DIR}/third_party/surelog/.git"
OR NOT EXISTS "${PROJECT_SOURCE_DIR}/third_party/yosys/.git")
message(SEND_ERROR "The git submodules are not available. Please run
git submodule sync
git submodule update --init --recursive third_party/{surelog,yosys}")

kgugala · 2023-10-26T09:53:01Z

CMakeLists.txt

+endif()
+
+# In cmake it is imposible to read the output of yosys-config and populate variables at cmake configuration time as yosys is built in this cmake
+# The initial content of YOSYS_CONFIG_FLAGS is a bootstrap that enables to build the synlig local code (frontends/systemverilog), not yosys itself.


i think we can drop this comment now

kgugala · 2023-10-26T09:59:15Z

I think we're almost ready to merge this just 2 small requests:

can you run a cmake-format on the files? Indentation seems to be a little inconsistent. I suppose it makes sense to even include cmake format checks in CI so any future updates will keep correct formatting
please rebase the PR on top of main (where the OpenTitan issue is fixed), so we can have green CI here

alaindargelas · 2023-10-26T17:05:59Z

Rebased and formatted.

alaindargelas added 2 commits October 21, 2023 21:08

Alternative cmake build system

dc25d83

Alternative cmake build system

01e6da6

alaindargelas requested a review from hzeller October 22, 2023 04:11

cmake action

1fb9a4c

alaindargelas marked this pull request as draft October 22, 2023 04:22

alaindargelas added 3 commits October 21, 2023 21:27

cmake action

bfe47e8

cmake action

e55d8fe

cmake action

eb6583a

alaindargelas marked this pull request as ready for review October 22, 2023 07:02

alaindargelas requested a review from kgugala October 22, 2023 07:04

hzeller reviewed Oct 23, 2023

View reviewed changes

makefile.txt Outdated Show resolved Hide resolved

hzeller reviewed Oct 23, 2023

View reviewed changes

makefile.txt Outdated Show resolved Hide resolved

alaindargelas added 3 commits October 23, 2023 10:51

rename cmake-makefile

92c723d

rename cmake-makefile

2f8b80b

rename cmake-makefile

76c8f50

kgugala requested changes Oct 23, 2023

View reviewed changes

alaindargelas added 6 commits October 23, 2023 12:39

address code review

a9b9be1

address code review

b7f73c4

address code review

b947acb

address code review

38e1d07

address code review

99dc520

symplify flags to mininum

2fbf4a9

alaindargelas mentioned this pull request Oct 25, 2023

bash: - : invalid option while doing make install on Centos7 #2104

Closed

kgugala reviewed Oct 26, 2023

View reviewed changes

formatting and rebase

db01b9b

alaindargelas added 3 commits October 26, 2023 09:50

formatting and rebase

bd0d0cf

formatting and rebase

c4128b1

error if no subsystem

0abb82d

alaindargelas requested a review from kgugala October 27, 2023 01:39

kgugala approved these changes Oct 27, 2023

View reviewed changes

kgugala merged commit 2884d27 into chipsalliance:main Oct 27, 2023
32 checks passed

alaindargelas deleted the alternative_cmake_build branch October 27, 2023 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative cmake build system #2089

Alternative cmake build system #2089

alaindargelas commented Oct 22, 2023

alaindargelas commented Oct 22, 2023

alaindargelas commented Oct 23, 2023

pgielda commented Oct 23, 2023

kgugala Oct 23, 2023

alaindargelas Oct 23, 2023

kgugala Oct 23, 2023

alaindargelas Oct 23, 2023

alaindargelas Oct 23, 2023

alaindargelas Oct 23, 2023

kgugala Oct 23, 2023

alaindargelas Oct 23, 2023

kgugala Oct 23, 2023

alaindargelas Oct 23, 2023

alaindargelas Oct 23, 2023

alaindargelas Oct 24, 2023

alaindargelas Oct 24, 2023

alaindargelas Oct 25, 2023

pgielda Oct 25, 2023 •

edited

Loading

alaindargelas Oct 25, 2023

kgugala Oct 26, 2023

alaindargelas Oct 26, 2023

kgugala Oct 26, 2023

alaindargelas Oct 26, 2023

kgugala commented Oct 26, 2023

alaindargelas commented Oct 26, 2023

Alternative cmake build system #2089

Alternative cmake build system #2089

Conversation

alaindargelas commented Oct 22, 2023

alaindargelas commented Oct 22, 2023

alaindargelas commented Oct 23, 2023

pgielda commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgielda Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgugala commented Oct 26, 2023

alaindargelas commented Oct 26, 2023

pgielda Oct 25, 2023 •

edited

Loading