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

build: add cmake based build infrastructure #154

Closed
wants to merge 10 commits into from

Conversation

compnerd
Copy link

Rather than rely on make, use CMake to generate the rules in the build
system of choice. This allows building with make or ninja, and
enables building the WASI libc on Windows.

cmake -G Ninja -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_C_COMPILER=clang -DCMAKE_SYSTEM_NAME=wasi -DCMAKE_MODULE_PATH=cmake/Modules
ninja -C build install

@compnerd
Copy link
Author

Moved from #12

@compnerd
Copy link
Author

@sunfishcode I've moved the PR here as per your comments. I also opted to just dump a first cut attempt to enumerate the sources and add support for the new content. This should be able to generate the additional libraries added here.

This abstracts the build system, allowing building with ninja or make.

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

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

At first glance looks like a good start. I find CMake's dislike for wildcards mildly annoying, but if this means we get better Windows support and proper incremental builds, that more than makes up for it :-).

As a heads up, https://github.com/CraneStation/wasi-libc/pull/150/files is proposing to change the Makefile to allow it to build non-LTO and LTO builds in the same sysroot. The CMake file doesn't need to hand things the same way, but it's something to think about.

CMakeLists.txt Outdated
"BUILD_LIBC_BOTTOM_HALF" YES)

if(CMAKE_C_COMPILER_ID MATCHES Clang)
add_compile_options(-target wasm32-unknown-wasi)
Copy link
Member

Choose a reason for hiding this comment

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

We generally abbreviate the target as "wasm32-wasi".

Copy link
Author

Choose a reason for hiding this comment

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

Ah, internally, LLVM will actually normalize the triple to this. Is there a reason to not just use the normalized form? If its just to maintain uniformity with Make (which I think longer term could be replaced with the metabuild), that is reasonable.

cmake/Modules/Platform/wasi.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@compnerd
Copy link
Author

compnerd commented Dec 25, 2019

I'll take a look at #150, thanks for the heads up. I think most of the metabuild systems do the same, requiring explicit listing. I think that it is a one-time cost, incrementally, maintaining the list is pretty easy. It should give proper dependency tracking/incremental builds and better cross-platform support. I'm not 100% tied to CMake, if you prefer gn, I could swap to that.

I think that with CMake, you tend to just build in a different tree with a different set of flags rather than the different variants in one. gn does the same as well, with the difference that you just alter the configuration flags in the same tree.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I am a big fan of being able to build with ninja, but in terms of windows compat, is depending on make really that much of a problem? The windows hosts on github actions certainly don't seem to have a problem with this dependency. I guess this install mingw or something like that?

I also agree with @sunfishcode that using wildcards would make for increased readability here if you we can find a way to make it work.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/Modules/Platform/wasi.cmake Outdated Show resolved Hide resolved
@sbc100
Copy link
Member

sbc100 commented Dec 26, 2019

Can you update the .github actions to use cmake and delete the old Makefile so that we can see how this works in action?

@compnerd
Copy link
Author

I'm not very familiar with the GitHub actions, I'll see if I can figure out how to do that. I think that it may be useful to do that in two stages.

@pchickey
Copy link
Collaborator

This doesn't appear to be working on my machine. I had to install the latest cmake release, but that doesn't appear to be sufficient. I do think we need to get it working on CI before merging.

To add the cmake and ninja (or make) steps to the Github CI, add each command as a - run directive below this line:

https://github.com/CraneStation/wasi-libc/blob/2f6ce572c6a1af48b7392bffddb81b94411c4478/.github/workflows/main.yml#L28

Here is the build failure on my machine:

[pat@finknottle:src/wasi-libc]% /usr/local/bin/cmake -G Ninja -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_C_COMPILER=clang -DCMAKE_SYSTEM_NAME=wasi -DCMAKE_MODULE_PATH=cmake/Modules  
-- The ASM compiler identification is Clang
-- Found assembler: /usr/bin/clang
-- The C compiler identification is Clang 7.1.0
-- Check for working C compiler: /usr/bin/clang
System is unknown to cmake, create:
Platform/wasi to use this system, please post your config file on discourse.cmake.org so it can be added to cmake
-- Check for working C compiler: /usr/bin/clang -- works
-- Detecting C compiler ABI info
System is unknown to cmake, create:
Platform/wasi to use this system, please post your config file on discourse.cmake.org so it can be added to cmake
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /home/pat/src/wasi-sdk/src/wasi-libc/build
[pat@finknottle:src/wasi-libc]% ninja -C build
ninja: Entering directory `build'
[6/765] Building C object CMakeFiles/libc.dir/basics/sources/complex-builtins.c.o
FAILED: /usr/bin/clang   -O2 -g -DNDEBUG   -target wasm32-unknown-wasi --sysroot=/home/pat/src/wasi-sdk/src/wasi-libc/build/sysroot -fno-trapping-math -mthread-model single -MD -MT CMakeFiles/libc.dir/basics/sources/complex-builtins.c.o -MF CMakeFiles/libc.dir/basics/sources/complex-builtins.c.o.d -o CMakeFiles/libc.dir/basics/sources/complex-builtins.c.o   -c ../basics/sources/complex-builtins.c
../basics/sources/complex-builtins.c:5:10: fatal error: 'complex.h' file not found
#include <complex.h>
         ^~~~~~~~~~~
1 error generated.
[6/765] Building C object CMakeFiles/libc.dir/basics/sources/reallocarray.c.o
FAILED: /usr/bin/clang   -O2 -g -DNDEBUG   -target wasm32-unknown-wasi --sysroot=/home/pat/src/wasi-sdk/src/wasi-libc/build/sysroot -fno-trapping-math -mthread-model single -MD -MT CMakeFiles/libc.dir/basics/sources/reallocarray.c.o -MF CMakeFiles/libc.dir/basics/sources/reallocarray.c.o.d -o CMakeFiles/libc.dir/basics/sources/reallocarray.c.o   -c ../basics/sources/reallocarray.c
../basics/sources/reallocarray.c:1:10: fatal error: 'stdlib.h' file not found
#include <stdlib.h>
         ^~~~~~~~~~
1 error generated.
[6/765] Building C object CMakeFiles/libc.dir/basics/sources/abort.c.o
FAILED: /usr/bin/clang   -O2 -g -DNDEBUG   -target wasm32-unknown-wasi --sysroot=/home/pat/src/wasi-sdk/src/wasi-libc/build/sysroot -fno-trapping-math -mthread-model single -MD -MT CMakeFiles/libc.dir/basics/sources/abort.c.o -MF CMakeFiles/libc.dir/basics/sources/abort.c.o.d -o CMakeFiles/libc.dir/basics/sources/abort.c.o   -c ../basics/sources/abort.c
../basics/sources/abort.c:1:10: fatal error: 'stdlib.h' file not found
#include <stdlib.h>
         ^~~~~~~~~~
1 error generated.
[6/765] Building C object CMakeFiles/libc.dir/libc-top-half/musl/src/misc/getsubopt.c.o
FAILED: /usr/bin/clang   -O2 -g -DNDEBUG   -target wasm32-unknown-wasi --sysroot=/home/pat/src/wasi-sdk/src/wasi-libc/build/sysroot -fno-trapping-math -mthread-model single -MD -MT CMakeFiles/libc.dir/libc-top-half/musl/src/misc/getsubopt.c.o -MF CMakeFiles/libc.dir/libc-top-half/musl/src/misc/getsubopt.c.o.d -o CMakeFiles/libc.dir/libc-top-half/musl/src/misc/getsubopt.c.o   -c ../libc-top-half/musl/src/misc/getsubopt.c
../libc-top-half/musl/src/misc/getsubopt.c:1:10: fatal error: 'stdlib.h' file not found
#include <stdlib.h>
         ^~~~~~~~~~
1 error generated.
[6/765] Building C object CMakeFiles/libc.dir/basics/sources/math/fmin-fmax.c.o
FAILED: /usr/bin/clang   -O2 -g -DNDEBUG   -target wasm32-unknown-wasi --sysroot=/home/pat/src/wasi-sdk/src/wasi-libc/build/sysroot -fno-trapping-math -mthread-model single -MD -MT CMakeFiles/libc.dir/basics/sources/math/fmin-fmax.c.o -MF CMakeFiles/libc.dir/basics/sources/math/fmin-fmax.c.o.d -o CMakeFiles/libc.dir/basics/sources/math/fmin-fmax.c.o   -c ../basics/sources/math/fmin-fmax.c
../basics/sources/math/fmin-fmax.c:10:10: fatal error: 'math.h' file not found
#include <math.h>
         ^~~~~~~~
1 error generated.
[6/765] Building C object CMakeFiles/dlmalloc.dir/dlmalloc/src/dlmalloc.c.o
FAILED: /usr/bin/clang  -I../dlmalloc/include -O2 -g -DNDEBUG   -target wasm32-unknown-wasi --sysroot=/home/pat/src/wasi-sdk/src/wasi-libc/build/sysroot -fno-trapping-math -mthread-model single -MD -MT CMakeFiles/dlmalloc.dir/dlmalloc/src/dlmalloc.c.o -MF CMakeFiles/dlmalloc.dir/dlmalloc/src/dlmalloc.c.o.d -o CMakeFiles/dlmalloc.dir/dlmalloc/src/dlmalloc.c.o   -c ../dlmalloc/src/dlmalloc.c
../dlmalloc/src/dlmalloc.c:7:10: fatal error: 'malloc.h' file not found
#include <malloc.h>
         ^~~~~~~~~~
1 error generated.
ninja: build stopped: subcommand failed.

@compnerd
Copy link
Author

I’m pretty sure that complex.h is provided by the compiler resource directory. Is the clang install complete?

Thanks for the hints on the GitHub actions. I’m going to try to see if I can drop the minimum version to 3.12 as the current azure image seems to have that. Odd since I use the 3.15 on the Swift builds.

@compnerd compnerd force-pushed the cmake branch 7 times, most recently from 96dab69 to 3eace1a Compare January 2, 2020 06:13
Make crt1 a static library so that we can have a post-build command copy
the object file to the correct location and rename it properly.
Unfortunately, renaming the object file itself is somewhat challenging.
Create targets for the stubs and provide proper dependency tracking.
This adds a python script to generate libc.imports as the shell
utilities may not be available on Windows.
Add a new `gen-predefined-macros` tool to generate the
`predefined-macros.txt` and `include-all.c` resources.  This allows for
a portable way to generate these files, which is important for build
environments like Windows which do not have the unix tools available.
@compnerd
Copy link
Author

I think that this is complete now. The only thing that it doesn't generate are two intermediate files (which could be added to gen-imports.py if desired). There is a test target that diffs the expected macros and syntax checks the headers. The Makefile has been removed as that is no longer needed for anything - everything is covered by the CMake based build. Updated the CI configuration to build with CMake as well.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I'm slightly concerned about the amount of complexity and extra dependencies this adds compared to simply requiring GNU make on windows.

@compnerd can you explain exactly why the existing GNU make dependency is a problem for you? Is depending on python + cmake really any better?

cmake_minimum_required(VERSION 3.12.1)

project(wasi-libc
LANGUAGES C)
Copy link
Member

Choose a reason for hiding this comment

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

But its doesn't seem consistent. For example below you call find_package and you put all the args on single line.. which I think is fine.

@compnerd
Copy link
Author

compnerd commented Jan 14, 2020

I don't really see this is being any more complex, but rather less complex because I can immediately see what files are going where. With the glob based make, it was very unclear and the order of the commands were particularly important but invisible as they copied files over one another. It also makes null-builds simpler (see #156). This not only brings that transparency, but also enables building with the GNU tools, building with Ninja, and the possibility of using other build systems more easily (e.g. gn). Yes, the change in dependency, IMO, is massively an improvement given that all the dependencies to build this project are now available through a standard installation of Visual Studio on Windows (Python, Ninja, CMake, and clang/LLVM) are distributed through Visual Studio's installer.

This also makes it easier for me to integrate with the project for another larger scoped project of trying to get WASM/WASI support in Swift.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

I agree; I like how this gets us to proper dependencies.

The Makefile builds libc with -DNDEBUG, and I didn't see that being set in the CMake files; does CMake itself set that?

The Makefiles have fairly tricky logic to build multiple versions of printf and friends, to optimize for the cases where long double or floating-point support aren't needed. I see you have logic for that; can you confirm that the printf optimizations still work? To help with this, I create this:
WebAssembly/wasi-sdk#97
to start some tests for printf in wasi-sdk. It would be good to confirm that the no-float version compiles to a smaller binary than the no-long-double version, which compiles to a smaller binary than the full long double version.

Are the check-metadata checks run automatically anywhere? One of the things I'm hoping will give us confidence in this new build system is to ensure that the resulting set of predefined macros, defined symbols, and undefined symbols is the same as before.

libc-top-half
dlmalloc)

foreach(stub m rt pthread crypt util xnet resolve dl)
Copy link
Member

Choose a reason for hiding this comment

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

Could you preserve the original comment "Create empty placeholder libraries" here?

add_dependencies(libc.imports c crt1.o)

add_test(NAME check-metadata
COMMAND ${CMAKE_COMMAND} -E compare_files --ignore-eol ${PROJECT_SOURCE_DIR}/expected/wasm32-wasi/predefined-macros.txt ${CMAKE_BINARY_DIR}/sysroot/share/predefined-macros.txt)
Copy link
Member

Choose a reason for hiding this comment

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

This is less friendly than running diff because it doesn't show what the differences were. Could you add a rule which runs an actual diff command?

@compnerd
Copy link
Author

-DNDEBUG is implicit in Release mode, and undefined in debug mode. Do you want that always defined?

I believe that there are only two variants being built in the CMake build. The third variant didn't have anyone building it, it's easy to add if desired.

Yes, running ninja test / cmake --test out will run the metadata tests.

@pchickey
Copy link
Collaborator

@compnerd do you know how to resolve the build failure here https://github.com/CraneStation/wasi-sdk/pull/94/checks?check_run_id=394413947 - we're using the new CMake build for the sdk, but the libc is not installed yet because this is the step to build it.

@compnerd
Copy link
Author

@pchickey weird, I wonder how I didn't hit that, however, its just a benign check for the libc. You can simply add a -DCMAKE_C_COMPILER_WORKS=YES when configuring wasi-libc to bootstrap the SDK.

Base automatically changed from master to main January 19, 2021 23:28
@loganek loganek mentioned this pull request Sep 30, 2022
3 tasks
@abrown
Copy link
Collaborator

abrown commented Jul 11, 2023

Can we close this? #330 is attempting the same kind of thing but is more recent.

@abrown abrown self-assigned this Jul 11, 2023
@abrown
Copy link
Collaborator

abrown commented Jul 13, 2023

Ok, I'm closing this; feel free to reopen if this is a better approach.

@abrown abrown closed this Jul 13, 2023
@compnerd compnerd deleted the cmake branch July 14, 2023 00:38
@abrown
Copy link
Collaborator

abrown commented Jul 18, 2023

@compnerd, looking through your comments on #330, did you feel this was the better PR to push this idea forward? I can reopen it if that is the case! (I was just trying to clean up old PRs and issues).

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.

5 participants