Make CI faster #587

rhysd · 2018-06-10T13:23:57Z

I made running CI faster.

Before: https://travis-ci.org/vim-jp/vital.vim/builds/390362502
After: https://travis-ci.org/rhysd/vital.vim/builds/390389523

Total: 5min 47sec -> 4min 2sec (1min 45sec faster)

Optimizations:

Enabled a binary package (bottle) on macOS worker by removing installation options which prevented it. (4m54s -> 2m58s)
- macOS worker is slow to start. It was a bottleneck.
- Removed Lua dependency since Lua was not actually used in CI. (--with-lua could be omitted by this change)
- Link vim executable directly (--with-override-system-vim could be omitted by this change)
Separate running linters into another job.
- Since it took 200s.
- After optimizing macOS worker, the job which run linters was a bottleneck.

I assigned people as reviewers who would be familiar with

CI scripts
macOS

Fix #534

because it is not necessary for running CI

Removing --with-override-system-vim can install MacVim with a binary package (bottle).

because running linters takes the longest time among CI jobs

ujihisa · 2018-06-10T13:30:12Z

Great work! I'll leave proper review for someone who knows CIs better than me, but I like these changes 👍

Removed Lua dependency since Lua was not actually used in CI. (--with-lua could be omitted by this change)

Good catch! 👍

rhysd · 2018-06-10T13:33:37Z

@ujihisa Thank you. I assigned you since you're more familiar with macOS than me. From macOS (Homebrew) user point of view, I'd be happy if you check the changes.

ujihisa · 2018-06-10T13:40:14Z

(I used to be a professional MacVim user. Currently I have no MacBooks/iMacs...)

haya14busa · 2018-06-10T14:24:59Z

.travis.yml

-  - covimerage write_coverage $THEMIS_PROFILE
-  - coverage xml
-  - bash <(curl -s https://codecov.io/bash)
+  - bash scripts/coverage.sh


[optional] i slightly prefer moving this line after themis line.
it doesn't need to be run with RUN_LINT=true, so it's a bit confusing.

Let me confirm: 'themis line' means the line which runs unit tests with themis.vim, am I correct?

If so, we can't move this line to script section because coverage should be taken only if CI was successfully done.
The guard $RUN_LINT == "" is in coverage.sh, but separating scripts into another file may make understanding what is done harder.
Do you think we should write the script directly in .travis.yml as follows?

- | if [[ "${RUN_LINT}" == "" ]]; then covimerage write_coverage $THEMIS_PROFILE coverage xml bash <(curl -s https://codecov.io/bash) fi

Cannot we run like this?
Another option is use && to run coverage scripts.

- if [[ "${RUN_LINT}" != "true" ]]; then /tmp/vim-themis/bin/themis --runtimepath /tmp/vimproc --exclude ConcurrentProcess --reporter dot bash scripts/coverage.sh fi

One additional good point to do this is that we can upload coverage scripts when another commands failed.

It's also ok. I'll choose it. Thank you for your advice.

It seems that the fix for this causes some problem on macOS: https://travis-ci.org/vim-jp/vital.vim/jobs/391106600
I don't have enough time to investigate so let me skip this for now (since it's just an option).

tyru · 2018-06-10T15:05:55Z

scripts/install-vim.sh

 		git clone --depth 1 --branch "${VIM_VERSION}" https://github.com/vim/vim /tmp/vim
 		cd /tmp/vim
 		./configure --prefix="${HOME}/vim" --with-features=huge \
 			--enable-perlinterp --enable-pythoninterp --enable-python3interp \
-			--enable-rubyinterp --enable-luainterp --enable-fail-if-missing
+			--enable-rubyinterp --enable-fail-if-missing


I'm just curious how it would speed up if --enable-rubyinterp was removed,
if it is not too much to ask...

I did not consider that. Let me try.

@tyru I tried removing if_perl and if_ruby, but could not see performance improvements https://travis-ci.org/rhysd/vital.vim/builds/390581618

@rhysd Thanks!!
Then I think if_perl and if_ruby should be left.

hmmmmm, @lambdalisue told me that if_perl and if_ruby sometimes causes build error... (on vim-jp #vital-dev slack)
So now I feel removing language interfaces as possible is the right policy then...
Now I remember language interfaces sometimes doesn't follow the latest language's change.

oops, fixed.
❌ if_perl and if_ruby causes build error
⭕ if_perl and if_ruby sometimes causes build error

I don't think --enable-luainterp should be removed while some codes in vital.vim relies on that feature.
https://github.com/vim-jp/vital.vim/search?p=1&q=lua&unscoped_q=lua

I'm very welcome to remove --enable-rubyinterp or --enable-perlinterp while no codes in vital.vim uses that features and it sometime cause build errors.

I think vital needs +lua/dyn for Text/Sexp

lambdalisue · 2018-06-10T21:56:18Z

Note: It's a bit beyond this PR but I've found that using Docker is quite useful to test on Linux.

https://github.com/lambdalisue/gina.vim/blob/master/.travis.yml#L40-L43

rhysd · 2018-06-11T02:45:51Z

Note: It's a bit beyond this PR but I've found that using Docker is quite useful to test on Linux.

Definitely. Moving to Docker container might be a future work to improve this further.

lambdalisue · 2018-06-11T08:27:34Z

scripts/install-vim.sh

 		git clone --depth 1 --branch "${VIM_VERSION}" https://github.com/vim/vim /tmp/vim
 		cd /tmp/vim
 		./configure --prefix="${HOME}/vim" --with-features=huge \
 			--enable-perlinterp --enable-pythoninterp --enable-python3interp \
-			--enable-rubyinterp --enable-luainterp --enable-fail-if-missing
+			--enable-rubyinterp --enable-fail-if-missing


I don't think --enable-luainterp should be removed while some codes in vital.vim relies on that feature.
https://github.com/vim-jp/vital.vim/search?p=1&q=lua&unscoped_q=lua

I'm very welcome to remove --enable-rubyinterp or --enable-perlinterp while no codes in vital.vim uses that features and it sometime cause build errors.

lambdalisue · 2018-06-11T08:29:08Z

scripts/install-vim.sh

+		brew install macvim
+		# Instead of --with-override-system-vim, manually link the executable because
+		# it prevents MacVim installation with a bottle.
+		ln -s "$(brew --prefix macvim)/bin/mvim" "/usr/local/bin/vim"


How about

export THEMIS_VIM="$(brew --prefix macvim)/bin/mvim"

instead?

lambdalisue · 2018-06-11T08:33:51Z

Outdated but

~~I think --enable-luainterp should be kept~~ Let's ignore.
I prefer export THEMIS_VIM=$(brew ...) instead (nit)

rhysd · 2018-06-11T08:41:00Z

@lambdalisue @raa0121

I think --enable-luainterp should be kept

Test using Lua is only Text/Sexp.vim (which has only one test case). Other modules which use Lua don't have any tests. So it's a trade-off. 'Give up one test case for Text/Sexp.vim' v.s. 'Making CI faster (about 1.4x faster)'. I don't have any strong opinion for this, actually.

ujihisa · 2018-06-11T08:46:04Z

Please ignore Text.Sexp. I'm planning to deprecate its lua version, along with #582 (Text.Sexp and brainf**k modules depended on it)

(I'll make another pullreq to deprecate them.)

tyru · 2018-06-11T09:40:11Z

👍 for ignore

ujihisa · 2018-06-11T11:49:36Z

👏 let's merge 👏

tyru · 2018-06-11T12:05:09Z

Still remaining issue exists #587 (comment)

rhysd · 2018-06-12T05:55:59Z

@tyru @ujihisa @haya14busa @lambdalisue @raa0121 @thinca Thank you for your reviews

tyru · 2018-06-12T06:51:47Z

🎉🎉🎉

rhysd added 3 commits June 10, 2018 22:11

CI: Remove Lua dependency from Travis CI

e989baa

because it is not necessary for running CI

CI: Use homebrew bottle on macOS on Travis CI

749da85

Removing --with-override-system-vim can install MacVim with a binary package (bottle).

CI: Separate running linters to another job on Travis CI

c18f991

because running linters takes the longest time among CI jobs

rhysd requested review from ujihisa, thinca, tyru, lambdalisue and haya14busa June 10, 2018 13:23

ujihisa removed their request for review June 10, 2018 13:30

CI: Remove $HOMEBREW_NO_AUTO_UPDATE workaround on macOS worker

f135704

haya14busa approved these changes Jun 10, 2018

View reviewed changes

tyru reviewed Jun 10, 2018

View reviewed changes

CI: Remove perl and Ruby dependencies on Travis CI

4e9a28e

lambdalisue reviewed Jun 11, 2018

View reviewed changes

lambdalisue approved these changes Jun 11, 2018

View reviewed changes

tyru mentioned this pull request Jun 11, 2018

Remove --rubyinterp　とか #534

Closed

ujihisa mentioned this pull request Jun 11, 2018

Deprecate Text.Sexp #591

Merged

thinca approved these changes Jun 11, 2018

View reviewed changes

rhysd force-pushed the ci/make-faster branch from cb0db0f to 4aad5c6 Compare June 12, 2018 05:22

rhysd force-pushed the ci/make-faster branch 4 times, most recently from d5c6616 to 4e9a28e Compare June 12, 2018 05:48

rhysd merged commit f4fca8c into vim-jp:master Jun 12, 2018

rhysd deleted the ci/make-faster branch June 12, 2018 05:56

tyru mentioned this pull request Jun 13, 2018

Remaining improvements/fixes for Data.List #588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CI faster #587

Make CI faster #587

rhysd commented Jun 10, 2018 •

edited

Loading

ujihisa commented Jun 10, 2018

rhysd commented Jun 10, 2018 •

edited

Loading

ujihisa commented Jun 10, 2018

haya14busa Jun 10, 2018

rhysd Jun 11, 2018 •

edited

Loading

haya14busa Jun 12, 2018

rhysd Jun 12, 2018

rhysd Jun 12, 2018 •

edited

Loading

tyru Jun 10, 2018

rhysd Jun 11, 2018

rhysd Jun 11, 2018

tyru Jun 11, 2018 •

edited

Loading

tyru Jun 11, 2018 •

edited

Loading

tyru Jun 11, 2018

lambdalisue Jun 11, 2018

rhysd Jun 11, 2018

raa0121 Jun 11, 2018

lambdalisue commented Jun 10, 2018

rhysd commented Jun 11, 2018 •

edited

Loading

lambdalisue Jun 11, 2018

lambdalisue Jun 11, 2018

lambdalisue commented Jun 11, 2018 •

edited

Loading

rhysd commented Jun 11, 2018 •

edited

Loading

ujihisa commented Jun 11, 2018 •

edited

Loading

tyru commented Jun 11, 2018

ujihisa commented Jun 11, 2018

tyru commented Jun 11, 2018

rhysd commented Jun 12, 2018 •

edited

Loading

tyru commented Jun 12, 2018

Make CI faster #587

Make CI faster #587

Conversation

rhysd commented Jun 10, 2018 • edited Loading

ujihisa commented Jun 10, 2018

rhysd commented Jun 10, 2018 • edited Loading

ujihisa commented Jun 10, 2018

Choose a reason for hiding this comment

rhysd Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhysd Jun 12, 2018 • 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

tyru Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

tyru Jun 11, 2018 • 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

lambdalisue commented Jun 10, 2018

rhysd commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue commented Jun 11, 2018 • edited Loading

rhysd commented Jun 11, 2018 • edited Loading

ujihisa commented Jun 11, 2018 • edited Loading

tyru commented Jun 11, 2018

ujihisa commented Jun 11, 2018

tyru commented Jun 11, 2018

rhysd commented Jun 12, 2018 • edited Loading

tyru commented Jun 12, 2018

rhysd commented Jun 10, 2018 •

edited

Loading

rhysd commented Jun 10, 2018 •

edited

Loading

rhysd Jun 11, 2018 •

edited

Loading

rhysd Jun 12, 2018 •

edited

Loading

tyru Jun 11, 2018 •

edited

Loading

tyru Jun 11, 2018 •

edited

Loading

rhysd commented Jun 11, 2018 •

edited

Loading

lambdalisue commented Jun 11, 2018 •

edited

Loading

rhysd commented Jun 11, 2018 •

edited

Loading

ujihisa commented Jun 11, 2018 •

edited

Loading

rhysd commented Jun 12, 2018 •

edited

Loading