Try to Use Ninja to Speed Up Build #666

WhiredPlanck · 2023-06-25T13:02:30Z

Pull request

Issue tracker

Fixes will automatically close the related issue

Fixes # N/A

Feature

Describe feature of pull request

This PR is to use ninja to build the stuffs on Linux and macOS to speed up the build.

Unit test

Done

Manual test

Done

Code Review

Unit and manual test pass
GitHub Action CI pass
At least one contributor reviews and votes
Can be merged clean without conflicts
PR will be merged by rebase upstream base

Additional Info

zhaozg · 2023-06-25T14:00:54Z

在 ci 中启用 Ninja 是合理的

但是在 Makefile 将 Ninja 设为 cmake 默认生成，则要求开发环境中必须要安装 Ninja，是否考虑将这个能力作为一个配置项呢

WhiredPlanck · 2023-06-25T14:37:01Z

在 ci 中启用 Ninja 是合理的

但是在 Makefile 将 Ninja 设为 cmake 默认生成，则要求开发环境中必须要安装 Ninja，是否考虑将这个能力作为一个配置项呢

@zhaozg 确实！你想的很周到，应该允许开发者自由选择。CMake 应该可以通过设定环境变量来选择构建文件生成器，我再完善一下，感谢提议。

不过我认为关键矛盾其实还是因为 ninja 即使在今天用得越来越多，很多系统的提供的默认开发环境还是不提供 ninja。用 ninja 必须要求环境里装 ninja，那其实用 make 其实也必须要求环境里装 make 嘛 ......

eagleoflqj · 2023-06-25T14:58:05Z

我认为librime这个make套cmake再make/ninja是不合理的，最外层的make不跨平台，而且在这个PR之前传-j也不会让内层make并行

WhiredPlanck · 2023-06-25T15:25:13Z

我认为librime这个make套cmake再make/ninja是不合理的，最外层的make不跨平台，而且在这个PR之前传-j也不会让内层make并行

没有特殊情况，一般不需要手动传 -j。CMake 作为一个相当现代的构建系统有着完善的并行构建支持，一般不需要手动管理；CMake 生成的 Makefile 比手写的更不容易出现并行构建竞争问题。

Makefile（还有 Ninja 的 build.ninja）本质就是一个基于规则/任务的脚本。用它去控制每个构建任务其实并没有什么不合理。

eagleoflqj · 2023-06-25T15:45:32Z

没有特殊情况，一般不需要手动传

可我在master branch make release开了top看真的只用了单线程啊😂

我想表达的是，@zhaozg 的需求其实直接调cmake就好了，用不用ninja/带不带glog之类的随便控制，不然要么把最外层的Makefile加一堆条目，要么得从Makefile往cmake传一堆参数

WhiredPlanck · 2023-06-25T15:53:22Z

@eagleoflqj 看起来我误会了什么，应该是要手动传 -j 的，但是其实你可以通过 export MAKEFLAGS+=" -j<n> " 的方式来传，不一定要在命令后面追加。

… windows Ref: 1. https://clang.llvm.org/get_started.html 2. https://github.com/vim/vim/blob/590aae35575cbd74d80c41d87fc647f2812aad70/src/msvc-latest.bat

This is required by non-Visual-Studio makefile generator

The program will not be put in the <build_config> dir if we don't set the makefile generator as Visual Studio stuffs.

WhiredPlanck · 2023-06-26T08:20:12Z

@lotem This PR is ready for review.

zhaozg · 2023-06-26T09:10:24Z

It's better that do rebase and combine to ONE commit, will get a clear commit history and avoid many intermediate commit.

WhiredPlanck · 2023-06-26T14:32:20Z

The CI seems to be malfunctioning, I'd like to close the PR, reorganize the commits then reopen.

WhiredPlanck · 2023-06-26T15:10:40Z

@zhaozg @lotem I open a new PR #667 inherited the works from here because I force push the commits to the branch before reopening this PR by mistake. Go to there to start a new review, please.

lotem · 2023-06-26T09:43:15Z

env.vs2017_xp.bat

+    set CMAKE_GENERATOR="Visual Studio 15 2017"
+    set PLATFORM_TOOLSET=v141_xp
+) else (
+    set "VSVEROPT=-version [15.0^,16.0^)"


What does the else-block do? Why is it on the defined CMAKE_GENERATOR branch?

If CMAKE_GENERATOR is defined outside the scripts, it may not be Visual Studio xx xxxx. To preserve the original settings, I split out a else-block for non-Visual-Studio generator to run a script which call vswhere.exe to find the Visual Studio's install location and call vcvarsall.bat to set almost all environment variables we need.
In a word, if one don't specify CMAKE_GENERATOR before calling env.bat and build.bat to build, it will use our original settings to build, otherwise the scripts will find and set everything we need and build with the specified generator.

lotem · 2023-06-26T09:45:06Z

env.bat.template

@@ -6,10 +6,18 @@ rem REQUIRED: path to Boost source directory
 if not defined BOOST_ROOT set BOOST_ROOT=%RIME_ROOT%\deps\boost_1_78_0

 rem architecture, Visual Studio version and platform toolset
-set ARCH=Win32


Some of these envars are not set in the else-branch. That's probably not okay because build.bat uses these variables.

@lotem These envars like ARCH here only make sense when CMAKE_GENERATOR is something like Visual Studio xx xxxx. I think there are enough refactors in `build.bat to adopt these changes.

lotem · 2023-06-26T09:46:27Z

env.vs2019.bat

+    set CMAKE_GENERATOR="Visual Studio 16 2019"
+    set PLATFORM_TOOLSET=v142
+) else (
+    set "VSVEROPT=-version [16.0^,17.0^)"


How about creating a separate env.bat file with the else branch.

@lotem No need actually. Else-block plays the same role with if-block.

lotem · 2023-06-26T09:48:04Z

env.vswhere.bat

What does this script do?

@lotem To find where Visual Studio installed in and set some envars we need.

lotem · 2023-06-26T09:50:48Z

Makefile

+	-DBOOST_ROOT="$(BOOST_ROOT)"
+endif
+
+# https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_SYSROOT.html


These lines repeats scripts in xcode.mk. Can Makefile replace xcode.mk after this change?

@lotem I think the answer is yes. We can merge Makefile and xcode.mk since the latter just specify CMAKE_GENERATOR as Xcode and nothing else is different with the former.

WhiredPlanck added 2 commits June 25, 2023 23:39

ci(action-install-linux.sh): install ninja build

7365e5d

ci(macos-build.yml): install ninja-build tool

7ba5cca

ci(linux-build.yml): export CMAKE_GENERATOR as Ninja

f87b418

WhiredPlanck force-pushed the ninja-build branch from d3cec19 to 6b15b89 Compare June 25, 2023 16:17

WhiredPlanck added 2 commits June 26, 2023 00:22

ci(macos-build.yml): try to use ninja build on macos

a1f8ac8

build(Makefile,deps.mk): add flag to control parallel build

970f3ae

WhiredPlanck force-pushed the ninja-build branch 4 times, most recently from 38d7f4f to efcaf18 Compare June 26, 2023 05:41

WhiredPlanck added 2 commits June 26, 2023 13:48

feat(env.bat.template...): allow to set desired makefile generator on…

d770a1f

… windows Ref: 1. https://clang.llvm.org/get_started.html 2. https://github.com/vim/vim/blob/590aae35575cbd74d80c41d87fc647f2812aad70/src/msvc-latest.bat

ci(windows-build.yml): try to use ninja to build

709bbae

WhiredPlanck force-pushed the ninja-build branch from efcaf18 to 709bbae Compare June 26, 2023 05:55

build(build.bat): use lowercase target name

19407b5

WhiredPlanck force-pushed the ninja-build branch from d945cbd to 4a715d0 Compare June 26, 2023 06:40

build(build.bat): explicitly specify CMAKE_BUILD_TYPE

c3ba2c3

This is required by non-Visual-Studio makefile generator

WhiredPlanck force-pushed the ninja-build branch from 4a715d0 to c3ba2c3 Compare June 26, 2023 07:07

WhiredPlanck added 2 commits June 26, 2023 15:25

build(build.bat): add alternative run path of rime_test.exe

242ecbc

The program will not be put in the <build_config> dir if we don't set the makefile generator as Visual Studio stuffs.

ci(workflows): minor refactor to workflow steps

99d6b69

WhiredPlanck marked this pull request as ready for review June 26, 2023 08:19

WhiredPlanck closed this Jun 26, 2023

WhiredPlanck mentioned this pull request Jun 26, 2023

Attempt to Use Ninja Build in CI #667

Closed

2 tasks

lotem reviewed Jul 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to Use Ninja to Speed Up Build #666

Try to Use Ninja to Speed Up Build #666

WhiredPlanck commented Jun 25, 2023

zhaozg commented Jun 25, 2023

WhiredPlanck commented Jun 25, 2023 •

edited

Loading

eagleoflqj commented Jun 25, 2023

WhiredPlanck commented Jun 25, 2023

eagleoflqj commented Jun 25, 2023

WhiredPlanck commented Jun 25, 2023

WhiredPlanck commented Jun 26, 2023

zhaozg commented Jun 26, 2023

WhiredPlanck commented Jun 26, 2023

WhiredPlanck commented Jun 26, 2023

lotem Jun 26, 2023

WhiredPlanck Jul 4, 2023 •

edited

Loading

lotem Jun 26, 2023

WhiredPlanck Jul 4, 2023 •

edited

Loading

lotem Jun 26, 2023

WhiredPlanck Jul 4, 2023

lotem Jun 26, 2023

WhiredPlanck Jul 4, 2023

lotem Jun 26, 2023

WhiredPlanck Jul 4, 2023

Try to Use Ninja to Speed Up Build #666

Try to Use Ninja to Speed Up Build #666

Conversation

WhiredPlanck commented Jun 25, 2023

Pull request

Issue tracker

Feature

Unit test

Manual test

Code Review

Additional Info

zhaozg commented Jun 25, 2023

WhiredPlanck commented Jun 25, 2023 • edited Loading

eagleoflqj commented Jun 25, 2023

WhiredPlanck commented Jun 25, 2023

eagleoflqj commented Jun 25, 2023

WhiredPlanck commented Jun 25, 2023

WhiredPlanck commented Jun 26, 2023

zhaozg commented Jun 26, 2023

WhiredPlanck commented Jun 26, 2023

WhiredPlanck commented Jun 26, 2023

Choose a reason for hiding this comment

WhiredPlanck Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhiredPlanck Jul 4, 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

Choose a reason for hiding this comment

WhiredPlanck commented Jun 25, 2023 •

edited

Loading

WhiredPlanck Jul 4, 2023 •

edited

Loading

WhiredPlanck Jul 4, 2023 •

edited

Loading