-
Notifications
You must be signed in to change notification settings - Fork 563
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
Attempt to Use Ninja Build in CI #667
Conversation
Also merge the setup in xcode.mk into Makefile.
1aecf0f
to
50fb110
Compare
clean: | ||
rm -Rf build debug | ||
|
||
librime: release | ||
install-librime: install | ||
uninstall-librime: uninstall | ||
|
||
ifdef NOPARALLEL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put variable definitions above all targets
@@ -25,4 +25,6 @@ jobs: | |||
run: ./action-install-linux.sh | |||
|
|||
- name: Build and test | |||
run: make test | |||
run: | | |||
export CMAKE_GENERATOR=Ninja |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this work equally, but written in one line:
make CMAKE_GENERATOR=Ninja test
|
||
test: release | ||
(cd $(build)/test; ./rime_test) | ||
(cd $(build)/test; ./rime_test || DYLD_LIBRARY_PATH=../lib/Release Release/rime_test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DYLD_LIBRARY_PATH=../lib/Release
does this work for Linux as well?
You added --config Release
, if this makes a difference in the build directory structure on Linux, the same DYLD_LIBRARY_PATH
should also work on Linux.
If not, what's the point of making any change on Linux by specifying --config
?
@echo off | ||
if errorlevel 1 goto error | ||
|
||
if "%build_test%" == "ON" ( | ||
copy /y dist\lib\rime.dll build\test | ||
pushd build\test | ||
.\Release\rime_test.exe || goto error | ||
.\Release\rime_test.exe || .\rime_test.exe || goto error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case is .\rime_test.exe
used?
set PLATFORM_TOOLSET=v142 | ||
rem architecture, Visual Studio version and compiler | ||
set "VSVEROPT=-version [16.0^,17.0^)" | ||
call ./env.vswhere.bat x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 4 lines, line 10-13, are not part of the configuration.
Move them to build.bat
.
set PLATFORM_TOOLSET=v141_xp | ||
rem architecture, Visual Studio version and compiler | ||
set "VSVEROPT=-version [15.0^,16.0^)" | ||
call ./env.vswhere.bat x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
set PLATFORM_TOOLSET=v142 | ||
rem architecture, Visual Studio version and compiler | ||
set "VSVEROPT=-version [16.0^,17.0^)" | ||
call ./env.vswhere.bat x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
set PLATFORM_TOOLSET=v143 | ||
rem architecture, Visual Studio version and compiler | ||
set "VSVEROPT=-version [17.0^,18.0^)" | ||
call ./env.vswhere.bat x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -5,11 +5,12 @@ set RIME_ROOT=%CD% | |||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removing these variables? They are used in build.bat
.
Part of this PR is merged into #718. Closing. |
Pull request
Issue tracker
Fixes will automatically close the related issue
Fixes # N/A
Feature
Describe feature of pull request
Try to use Ninja build to speed up the build in CI.
Inherits the conversations in #666 .
Unit test
Manual test
Code Review
Additional Info