Skip to content

Use cmake --build instead of makefiles #139

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

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Nov 5, 2022

The build script assumes that a makefile will exist after running cmake. In my case (MINGW64 / MSYS2 shell), Ninja was the default.

@andrewrk
Copy link
Member

andrewrk commented Nov 5, 2022

Perhaps instead the build script can be enhanced to support the native generator, without introducing any branching logic into the build script?

Perhaps something like cmake --build . --target INSTALL instead of invoking make separately.

@kcbanner kcbanner changed the title Explicitly specify makefiles as the generator, as it may not be the default Use cmake --build instead of makefiles Nov 5, 2022
@kcbanner
Copy link
Contributor Author

kcbanner commented Nov 5, 2022

Updated to use the default cmake generator. However, I ran into some build issues on mingw (ziglang/zig#12703 (comment)), so I can't verify this is working yet.

README.md Outdated
@@ -38,14 +38,16 @@ For other versions, check the git tags of this repository.

All parameters are required:

* `-j1`: Replace with your jobs parameter to make.
* `-j1`: Replace with your jobs parameter to cmake.
Copy link
Member

Choose a reason for hiding this comment

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

Does CMake infer the jobs correctly if this is omitted? Getting rid of the jobs parameter would be a nice improvement to this script.

Copy link
Contributor Author

@kcbanner kcbanner Nov 6, 2022

Choose a reason for hiding this comment

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

Unfortunately it just falls back to the default for that build system. For ninja that is the number of cores, but for make that's just 1.

Copy link
Member

Choose a reason for hiding this comment

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

CMake never fails to disappoint me.

Copy link
Member

Choose a reason for hiding this comment

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

Alright here's my suggestion. We can still remove the -j parameter to the build script, and next to your readme addition, we can also remind users of the existence of the CMAKE_BUILD_PARALLEL_LEVEL environment variable. So, if the user does not bother with these options, it will still succeed, however they can take advantage of the options to try to make it build faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👍

Unrelated question: I'm looking to add a build.bat in a future PR to bootstrap things from Windows - the missing link for me is I'm not sure how the zig+llvm+lld+clang-x64_64-windows-gnu bundles that the CI uses are generated, is that process documented somewhere? Is it something like build llvm(msvc crt) with msvc -> build zig2(msvc crt) with msvc -> build llvm(gnu crt) with zig2 -> build zig with llvm(gnu crt) ?

Copy link
Member

Choose a reason for hiding this comment

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

I run the build script on linux, and then use https://github.com/ziglang/release-cutter/blob/master/update-ci-tarballs.zig to create the zip file

@andrewrk andrewrk merged commit 0bc1dfe into ziglang:master Nov 7, 2022
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