[WIP] Add CMake build system #330

loganek · 2022-09-30T10:09:32Z

This is a work-in-progress PR and still require a few actions. I open the PR to get early feedback. Some of the changes are clone from the other attempt of migrating this repository to CMake build system: #154. One major difference between this change and the previous one is that I'm using install target to create sysroot; I find this to be a bit more natural than specifying destination for every target.

I tested the changes by diff-ing generated install directory with the sysroot generated by Makefile. I also compiled and ran a few apps on WAMR runtime.

remove Makefile
update CI files
update README file

Resolves #322

sbc100 · 2022-09-30T14:04:00Z

I know you mention some rational in #322, but I'm curious what you personal primary motivation is for wanting to make this change? I guess better native windows support is the big one? Without needing to setup something like mingw?

I personally find cmake syntax and semantics pretty confusing (more so than the relative simplicity of a Makefile), but I get that its personal preference.

This does seem to make some things more complex. For example, it looks like it takes roughly twice as many lines of code to implement. We would also be adding a python dependency. I'm generally a fan of python over shell scripts for this kind of thing but others might feel differently.

sbc100

Could you add some instructions (maybe as a mod to the README) so I can try it out? I've love to give it go. Does this setup support out-of-tree building?

sbc100 · 2022-09-30T14:06:00Z

cmake/libc-bottom-half/CMakeLists.txt

+list(REMOVE_ITEM LIBC_BOTTOM_HALF_ALL_SOURCES ${CHDIR_SRC})
+list(APPEND LIBC_BOTTOM_HALF_ALL_SOURCES ${CHDIR_SRC})
+
+add_library(libc-bottom-half OBJECT


I guess this is some kind of "library" that getting somehow inlined into the main libc? More like a list of object files that a library per-say?

I assume all the object files that make up libc-bottom-half will still show up as individual object files within libc.a?

Yes, they're just object files.

abrown · 2022-09-30T18:13:04Z

I personally find cmake syntax and semantics pretty confusing

I'm also not sold on replacing make with cmake. make is available on Windows in various ways: WSL, MinGW, and Visual Studio. I'm not opposed to replacing it, but I feel cmake would have to offer something better than what currently exists. Also, I'm a bit concerned that people working on the project may be familiar with a Makefile but would be slower to hack on a CMakeLists.txt (as @sunfishcode mentions in #322). Just some thoughts...

penzn · 2022-09-30T18:21:09Z

In my opinion, it provides more watertight ways to ensure you have the pieces you need, also it has "first class types" for sub-targets like libraries, including dependence detection, this reduces risk of mistyping or having parts of the build go out of sync. It isn't a silver bullet, though.

loganek · 2022-10-03T12:54:32Z

I know you mention some rational in #322, but I'm curious what you personal primary motivation is for wanting to make this change?

Pretty much what @penzn already mentioned but also having experience with both Makefile and CMake, I find both learning CMake and maintaining CMake project easier. I also think Makefile has a steeper learning curve than CMake but that may be because I ~~learned~~ used Makefile first.

This does seem to make some things more complex. For example, it looks like it takes roughly twice as many lines of code to implement.

I wouldn't use number of lines as a metric for complexity. If needed, I could probably optimize (most likely at a cost of readability, although I didn't look into it).

We would also be adding a python dependency. I'm generally a fan of python over shell scripts for this kind of thing but others might feel differently.

I don't think this is relevant to Makefile vs CMake discussion. They both can run shell and python scripts; I copied it (and slightly modified) from previous PR because I found it more readable.

Also, I'm a bit concerned that people working on the project may be familiar with a Makefile but would be slower to hack on a CMakeLists.txt (as @sunfishcode mentions in #322). Just some thoughts...

What I found is that creating a CMake project from scratch requires some knowledge, but most of the modifications don't often require looking into documentation, as you can do most of what you need based on already written code (I guess just like Makefile)

loganek · 2022-10-03T13:00:41Z

Could you add some instructions (maybe as a mod to the README) so I can try it out? I've love to give it go. Does this setup support out-of-tree building?

I can definitely update the README file; meanwhile, here's a command you can use:

# assume you're in wasi-libc
cd .. # go up; we'll do out-of-tree build
mkdir wasi-libc-build && cd wasi-libc-build
# configure the project
cmake \
  -DCMAKE_C_COMPILER=/usr/bin/clang-15 \ # optional, cmake will try to find a default one
 -DCMAKE_BUILD_TYPE=Release \ # optimized build; other options: Debug, RelWithDebInfo
 -DTHREAD_MODEL=posix \ # thread model: single
 -DCMAKE_INSTALL_PREFIX=../cmake-install \ # where to install the sysroot
 -G Ninja \ # generator, default on Linux is most likely Unix Makefiles
 ../wasi-libc # path to source directory
ninja install # build and install

sunfishcode · 2022-10-10T20:29:05Z

One concrete advantage of moving to CMake is that the current wasi-libc Makefile doesn't do dependency management correctly, so we effectively don't have incremental builds right now.

loganek · 2022-10-10T21:39:37Z

Yes, incremental build is a huge benefit, but I never listed it as a benefit because I'm not sure how hard is it to implement it for the existing Makefile.

compnerd

Part of why I had lost interest was the difficulty to get the proper support for CMake added due to upstream resistance.

compnerd · 2023-07-14T00:40:33Z

CMakeLists.txt

+enable_testing()
+
+if("${CMAKE_C_COMPILER}" MATCHES "(.*)clang(-[0-9]+)?$")
+  set(CMAKE_NM "${CMAKE_MATCH_1}llvm-nm${CMAKE_MATCH_2}")


I would recommend against this. This prevents the user from specifying the name tool to use.

compnerd · 2023-07-14T00:41:44Z

cmake/CMakeLists.txt

+
+add_library(c)
+
+set (LIBC_LINK_LIBRARIES


This is an anti-pattern. Prefer to directly use the targets with target_link_libraries

compnerd · 2023-07-14T00:42:26Z

cmake/CMakeLists.txt

+  libc-bottom-half
+)
+
+if(${BUILD_LIBC_TOP_HALF} STREQUAL "ON")


I tend to prefer the more concise spelling:

Suggested change

if(${BUILD_LIBC_TOP_HALF} STREQUAL "ON")

if(BUILD_LIBC_TOP_HALF)

This has the benefit of allowing the user to specify the value with a truthy value that CMake permits (ON, YES, 1).

compnerd · 2023-07-14T00:43:01Z

cmake/CMakeLists.txt

+if(${BUILD_LIBC_TOP_HALF} STREQUAL "ON")
+    list(APPEND LIBC_LINK_LIBRARIES
+      libc-top-half
+    )


Again, an anti-pattern. Prefer to use a generator expression with target_link_libraries.

compnerd · 2023-07-14T00:43:07Z

cmake/CMakeLists.txt

+
+if(${MALLOC_IMPL} STREQUAL "dlmalloc")
+  list(APPEND LIBC_LINK_LIBRARIES
+    dlmalloc)


compnerd · 2023-07-14T00:45:53Z

cmake/CMakeLists.txt

+
+add_test(NAME check-headers
+  COMMAND ${CMAKE_C_COMPILER}
+    -target ${CMAKE_C_COMPILER_TARGET}


This prevents the use of any compiler other than clang.

compnerd · 2023-07-14T00:46:12Z

cmake/dlmalloc/CMakeLists.txt

@@ -0,0 +1,10 @@
+set(DLMALLOC_DIR ${PROJECT_SOURCE_DIR}/dlmalloc)


Why the variable for a single use?

compnerd · 2023-07-14T00:46:29Z

cmake/global-compiler-options.cmake

@@ -0,0 +1,39 @@
+# WebAssembly floating-point match doesn't trap.
+# TODO: Add -fno-signaling-nans when the compiler supports it.
+add_compile_options(-fno-trapping-math)


You should validate that the compiler supports the option.

compnerd · 2023-07-14T00:46:34Z

cmake/global-compiler-options.cmake

+  -Wno-ignored-pragmas
+  -Wno-unused-but-set-variable
+  -Wno-unknown-warning-option
+)


compnerd · 2023-07-14T00:47:24Z

cmake/libc-bottom-half/CMakeLists.txt

+  LIBC_BOTTOM_HALF_ALL_SOURCES
+  CONFIGURE_DEPENDS
+  ${LIBC_BOTTOM_HALF_CLOUDLIBC_SRC_DIR}/*.c
+  ${LIBC_BOTTOM_HALF_SRC_DIR}/*.c)


This is a highly dubious. This will lose the ability to alter the sources. The glob is applied once at configure time and never again. You should explicitly list the sources. Similar throughout.

sbc100 reviewed Sep 30, 2022

View reviewed changes

Add CMake build system

082ec55

loganek force-pushed the loganek/cmake branch from f9d135e to 082ec55 Compare October 10, 2022 21:31

loganek force-pushed the loganek/cmake branch 5 times, most recently from d9c4692 to b984e91 Compare October 11, 2022 06:37

update github actions

f64759d

loganek force-pushed the loganek/cmake branch from b984e91 to f64759d Compare October 11, 2022 07:26

loganek mentioned this pull request Jan 3, 2023

test: add tests for pthread API #369

Draft

abrown marked this pull request as draft July 11, 2023 22:55

abrown mentioned this pull request Jul 11, 2023

build: add cmake based build infrastructure #154

Closed

compnerd reviewed Jul 14, 2023

View reviewed changes

xphoenix mentioned this pull request Feb 16, 2024

Migrate build system to CMake #322

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add CMake build system #330

[WIP] Add CMake build system #330

loganek commented Sep 30, 2022 •

edited

Loading

sbc100 commented Sep 30, 2022

sbc100 left a comment

sbc100 Sep 30, 2022

loganek Oct 3, 2022

abrown commented Sep 30, 2022

penzn commented Sep 30, 2022

loganek commented Oct 3, 2022

loganek commented Oct 3, 2022 •

edited

Loading

sunfishcode commented Oct 10, 2022

loganek commented Oct 10, 2022

compnerd left a comment

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

compnerd Jul 14, 2023

	if(${BUILD_LIBC_TOP_HALF} STREQUAL "ON")
	if(BUILD_LIBC_TOP_HALF)

		@@ -0,0 +1,10 @@
		set(DLMALLOC_DIR ${PROJECT_SOURCE_DIR}/dlmalloc)


		add_library(c)

		set (LIBC_LINK_LIBRARIES

[WIP] Add CMake build system #330

Are you sure you want to change the base?

[WIP] Add CMake build system #330

Conversation

loganek commented Sep 30, 2022 • edited Loading

sbc100 commented Sep 30, 2022

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrown commented Sep 30, 2022

penzn commented Sep 30, 2022

loganek commented Oct 3, 2022

loganek commented Oct 3, 2022 • edited Loading

sunfishcode commented Oct 10, 2022

loganek commented Oct 10, 2022

compnerd left a 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

loganek commented Sep 30, 2022 •

edited

Loading

loganek commented Oct 3, 2022 •

edited

Loading