[tarantool-patches] Re: [PATCH v2 1/2] Gitlab-ci updated after reiview - w/o perf

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks, Alexander!

test_foo runs runtest_foo, which runs test-run.py. This looks confusing
for me. We can use names like `test_ubuntu` and `test_ubuntu_no_deps`.

Comments below are generally re decreasing number of goals that I think
will simplify the configuration at whole.

WBR, Alexander Turenko.

On Sat, Jun 15, 2019 at 07:46:23AM +0300, Alexander V. Tikhonov wrote:
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4156-gitlab-ci-testing
> Issue: https://github.com/tarantool/tarantool/issues/4156
> 
>  .gitlab-ci.yml           | 328 +++++++++++++++++++++++++++++++++++++++
>  .gitlab.mk               |  57 +++++++
>  .travis.mk               | 101 +++++++++---
>  .travis.yml              |   3 +
>  images/Dockerfile.debian |   4 +
>  images/Dockerfile.lto    |   5 +
>  6 files changed, 475 insertions(+), 23 deletions(-)
>  create mode 100644 .gitlab-ci.yml
>  create mode 100644 .gitlab.mk
>  create mode 100644 images/Dockerfile.debian
>  create mode 100644 images/Dockerfile.lto
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> new file mode 100644
> index 000000000..01c4eacea
> --- /dev/null
> +++ b/.gitlab-ci.yml
> @@ -0,0 +1,328 @@
> +stages:
> +  - bootstrap
> +  - test
> +  - perf_deploy

Nit: Maybe perf_and_deploy?

However I would name it just 'deploy' within this patch.

> +
> +variables:
> +  VERSION_SUFFIX: "master"
> +  IMAGE_TEST: "${CI_REGISTRY}/${CI_PROJECT_PATH}:debian-stretch_${VERSION_SUFFIX}"
> +  IMAGE_TEST_LTO: "${CI_REGISTRY}/${CI_PROJECT_PATH}:debian-buster_${VERSION_SUFFIX}"

This suffix does not resolve any problems with changed dependencies. So,
we should either remove it for now or really resolve the problem: say,
use $(md5sum .travis.mk | awk '{ print $1 }') as a tag name.

BTW, I would prefer to use tags instead of suffixes; gitlab ci supports
it.

> +  FLAG_LTO: "-DENABLE_LTO=ON"

I think -DENABLE_LTO-ON is more self-descriptive then this variable
name. There is no need to extract it to a variable.

> +  GITLAB_MAKE: "make -f .gitlab.mk"
> +
> +# Bootstraps
> +
> +debian9:
> +  stage: bootstrap
> +  tags:
> +    - bootstrap
> +  variables:
> +    IMAGE: ${IMAGE_TEST}
> +    DOCKERFILE_SUFFIX: "debian"
> +  script:
> +    - ${GITLAB_MAKE} docker_bootstrap
> +
> +debian10_lto:
> +  only:
> +    refs:
> +      - master
> +      - /.*/

As we discusssed voicely: /.*/ should not be here in a final patch. We
however can provide an ability to run full testing on a branch different
from master. Say, match /.*-full/.

(The same is applicable for all such patterns below.)

> +  stage: bootstrap
> +  tags:
> +    - bootstrap
> +  variables:
> +    IMAGE: ${IMAGE_TEST_LTO}
> +    DOCKERFILE_SUFFIX: "lto"
> +  script:
> +    - ${GITLAB_MAKE} docker_bootstrap
> +
> +# Tests
> +
> +release:
> +  image: ${IMAGE_TEST}
> +  stage: test
> +  tags:
> +    - docker_test
> +  script:
> +    - ${GITLAB_MAKE} runtest_ubuntu
> +
> +debug:
> +  image: ${IMAGE_TEST}
> +  stage: test
> +  tags:
> +    - docker_test
> +  script:
> +    - ${GITLAB_MAKE} runtest_cov_ubuntu
> +
> +release_clang:
> +  image: ${IMAGE_TEST}
> +  stage: test
> +  tags:
> +    - docker_test
> +  variables:
> +    CC: clang
> +    CXX: clang++
> +  script:
> +    - ${GITLAB_MAKE} runtest_ubuntu
> +
> +release_lto:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: ${IMAGE_TEST_LTO}
> +  stage: test
> +  tags:
> +    - docker_test
> +  variables:
> +    CMAKE_EXTRA_PARAMS: ${FLAG_LTO}
> +  script:
> +    - ${GITLAB_MAKE} runtest_ubuntu
> +
> +release_lto_clang8:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: ${IMAGE_TEST_LTO}
> +  stage: test
> +  tags:
> +    - docker_test
> +  variables:
> +    CC: clang-8
> +    CXX: clang++-8
> +    CMAKE_EXTRA_PARAMS: ${FLAG_LTO}
> +  script:
> +    - ${GITLAB_MAKE} runtest_ubuntu
> +
> +osx13_release:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  stage: test
> +  tags:
> +    - vms_sierra
> +  variables:
> +    VMS_NAME: 'Sierra'
> +    VMS_USER: 'tarantool'
> +    VMS_PORT: '2212'
> +  before_script:
> +    - ${GITLAB_MAKE} vms_start
> +  script:
> +    - export

Forgotten debug? Below too.

> +    - ${GITLAB_MAKE} vms_test_osx
> +  after_script:
> +    - ${GITLAB_MAKE} vms_shutdown
> +
> +osx14_release:
> +  stage: test
> +  tags:
> +    - vms_mojave
> +  variables:
> +    VMS_NAME: 'Mojave'
> +    VMS_USER: 'tarantool'
> +    VMS_PORT: '2222'
> +  before_script:
> +    - ${GITLAB_MAKE} vms_start
> +  script:
> +    - export
> +    - ${GITLAB_MAKE} vms_test_osx
> +  after_script:
> +    - ${GITLAB_MAKE} vms_shutdown
> +
> +osx14_release_lto:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  stage: test
> +  tags:
> +    - vms_mojave
> +  variables:
> +    EXTRA_ENV: "export CMAKE_EXTRA_PARAMS=${FLAG_LTO} ;"
> +    VMS_NAME: 'Mojave'
> +    VMS_USER: 'tarantool'
> +    VMS_PORT: '2222'
> +  before_script:
> +    - ${GITLAB_MAKE} vms_start
> +  script:
> +    - export
> +    - ${GITLAB_MAKE} vms_test_osx
> +  after_script:
> +    - ${GITLAB_MAKE} vms_shutdown
> +
> +freebsd_release:
> +  stage: test
> +  tags:
> +    - vms_freebsd
> +  variables:
> +    VMS_NAME: 'Freebsd'

I would include a version number into the virtual machine name (and the
job name too).

> +    VMS_USER: 'vagrant'
> +    VMS_PORT: '2232'
> +    TRAVIS_MAKE: 'gmake -f .travis.mk'
> +  before_script:
> +    - ${GITLAB_MAKE} vms_start
> +  script:
> +    - export
> +    - ${GITLAB_MAKE} vms_test_freebsd
> +  after_script:
> +    - ${GITLAB_MAKE} vms_shutdown
> +
> +# Packs
> +
> +pack_cent6:

Why cent, not centos?

> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:el-6
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run_cent6
> +
> +pack_cent7:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:el-7
> +  stage: perf_deploy
> +  tags:
> +    - deploy_test
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_fedora28:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:fedora-28
> +  stage: perf_deploy
> +  tags:
> +    - deploy_test
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_fedora29:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:fedora-29
> +  stage: perf_deploy
> +  tags:
> +    - deploy_test
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_fedora30:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:fedora-30
> +  stage: perf_deploy
> +  tags:
> +    - deploy_test
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_ubuntu14:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:ubuntu-trusty
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_ubuntu16:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:ubuntu-xenial
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_ubuntu18:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:ubuntu-bionic
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_ubuntu1810:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:ubuntu-cosmic
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_ubuntu19:

I would name it 'package_ubuntu_19_04'.

> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:ubuntu-disco
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_debian8:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:debian-jessie
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_debian9:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:debian-stretch
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> +
> +pack_debian10:
> +  only:
> +    refs:
> +      - master
> +      - /.*/
> +  image: packpack/packpack:debian-buster
> +  stage: perf_deploy
> +  tags:
> +    - deploy
> +  script:
> +    - ${GITLAB_MAKE} pack_run
> diff --git a/.gitlab.mk b/.gitlab.mk
> new file mode 100644
> index 000000000..00a75a389
> --- /dev/null
> +++ b/.gitlab.mk
> @@ -0,0 +1,57 @@
> +GITLAB_MAKE?=make -f .gitlab.mk
> +TRAVIS_MAKE?=make -f .travis.mk
> +GIT_SUBMODULES_UPDATE?=git submodule update --recursive --init

Nit: I would name it GIT_SUBMODULE_UPDATE (w/o 'S'), because the command
is named so.

> +DOCKER_BUILD=docker build --network=host

I think there is no much reason to extract it from 'docker_bootstrap'
goal.

> +
> +# docker images bootstrap
> +docker_login:
> +	docker login -u ${CI_REGISTRY_USER} -p ${CI_REGISTRY_PASSWORD} ${CI_REGISTRY}

Why do we need to extract this goal? It is used only in
docker_bootstrap.

> +
> +docker_bootstrap: docker_login
> +	cp .travis.mk images/
> +	${DOCKER_BUILD} -t ${IMAGE} -f images/Dockerfile.${DOCKERFILE_SUFFIX} images

We can give . (a current directory) as a context, so we'll not need to
copy .travis.mk.

Also we can generate a dockerfile on-the-fly: I don't see much need to
keep it in the repository.

If a docker version allows we can even pass a dockerfile to stdin:
https://stackoverflow.com/a/42559367/1598057

> +	docker push ${IMAGE}
> +
> +# build sources with different flags and test

It is the title for the set of goals below? Looks as a comment for one
goal. Also here I see that bootstrap, submodules update and runnign
tests are doing, but not build sources. It seems the comment is not
precise.

> +git_submodules_update:
> +	${GIT_SUBMODULES_UPDATE} --force

Nit: The same as for GIT_SUBMODULES_UPDATE -> GIT_SUBMODULE_UPDATE:
git_submodules_update -> git_submodule_update.

Is there real need in this flag (some specific case when it is needed)?
What will going on CentOS 6 if we'll run into this case?

If it is really needed and we don't want to handle it on CentOS 6 (that
is strange), can we just run it like so?

$ git submodule update --recursive --init --force || git submodule update --recursive --init

It will allow to reduce amount of goals and so simplify the file.

> +
> +git_submodules_update_cent6:
> +	${GIT_SUBMODULES_UPDATE}
> +
> +runtest_%: git_submodules_update
> +	${TRAVIS_MAKE} $@
> +
> +# build sources with different flags and test under VBox virtual machines
> +vms_start:
> +	VBoxManage controlvm ${VMS_NAME} poweroff || true
> +	VBoxManage snapshot ${VMS_NAME} restore ${VMS_NAME}
> +	VBoxManage startvm ${VMS_NAME} --type headless
> +
> +vms_test_%:
> +	ssh -A ${VMS_USER}@127.0.0.1 -p ${VMS_PORT} "/bin/bash -c \

Should we really expose host keys into a virtual environment (-A
option)?

> +		'${EXTRA_ENV} \
> +		export PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin ; \
> +		cd tarantool && \
> +		git fetch -p && \
> +		git checkout --force ${CI_BUILD_REF} && \
> +		${GITLAB_MAKE} git_submodules_update && \
> +		${TRAVIS_MAKE} $(subst vms_,,$@)'"
> +
> +vms_shutdown:
> +	VBoxManage controlvm ${VMS_NAME} poweroff
> +
> +# build packs and test
> +pack_clone:
> +	git clone https://github.com/packpack/packpack.git packpack
> +
> +pack_make:
> +	# to avoid of issue with too long names build path is short
> +	make -f packpack/pack/Makefile -C . BUILDDIR=/builds/pack -j

Why do we need to split 'pack_build' into 'pack_clone' and 'pack_make'?

> +
> +pack_build: pack_clone pack_make
> +
> +pack_run: git_submodules_update pack_build
> +
> +pack_run_cent6: git_submodules_update_cent6 pack_build
> +

Nit: Extra empty line at the of the file.

I would squash all pack goals into one w/o deps (just 'package') and
update submodules just with a command. I guess --force is not really
needed and the command can be the same on all OSes.

Yep, we'll duplicate the command in +vms_test_%, but the structure of
the file will be simpler and one can read a certain sequince of commands
w/o look into dependent goals tree.

> diff --git a/.travis.mk b/.travis.mk
> index 6d0c42207..7d61c218b 100644
> --- a/.travis.mk
> +++ b/.travis.mk
> @@ -3,6 +3,7 @@
>  #
>  
>  DOCKER_IMAGE?=packpack/packpack:debian-stretch
> +NO_PARALLEL_TESTING?=

I would name it TEST_RUN_EXTRA_PARAMS (we already have
CMAKE_EXTRA_PARAMS).

>  
>  all: package
>  
> @@ -33,8 +34,12 @@ docker_%:
>  		${DOCKER_IMAGE} \
>  		make -f .travis.mk $(subst docker_,,$@)
>  
> +#########
> +# Linux #
> +#########
> +
>  deps_ubuntu:
> -	sudo apt-get update && apt-get install -y -f \
> +	apt-get -y update && apt-get install -y -f \
>  		build-essential cmake coreutils sed \
>  		libreadline-dev libncurses5-dev libyaml-dev libssl-dev \
>  		libcurl4-openssl-dev libunwind-dev libicu-dev \
> @@ -42,34 +47,29 @@ deps_ubuntu:
>  		python-msgpack python-yaml python-argparse python-six python-gevent \
>  		lcov ruby clang llvm llvm-dev
>  
> -test_ubuntu: deps_ubuntu
> +deps_buster_clang_8:
> +	echo "deb http://apt.llvm.org/buster/ llvm-toolchain-buster-8 main" > /etc/apt/sources.list.d/clang_8.list
> +	echo "deb-src http://apt.llvm.org/buster/ llvm-toolchain-buster-8 main" >> /etc/apt/sources.list.d/clang_8.list
> +	wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -

You removed 'sudo' above, but added it here. Is it the typo?

> +	apt-get -y update
> +	apt-get -y install clang-8 llvm-8 llvm-8-dev

Are llvm headers really needed? BTW, the same question re llvm-dev
above.

> +
> +build_ubuntu:
>  	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
> -	make -j8
> -	cd test && /usr/bin/python test-run.py --force -j 1
> +	make -j
>  
> -deps_osx:
> -	brew update
> -	brew install openssl readline curl icu4c --force
> -	python2 -V || brew install python2 --force
> -	curl --silent --show-error --retry 5 https://bootstrap.pypa.io/get-pip.py | python
> -	pip install -r test-run/requirements.txt
> +runtest_ubuntu: build_ubuntu
> +	cd test && /usr/bin/python test-run.py --force $(NO_PARALLEL_TESTING)
>  
> -test_osx: deps_osx
> -	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
> -	# Increase the maximum number of open file descriptors on macOS
> -	sudo sysctl -w kern.maxfiles=20480 || :
> -	sudo sysctl -w kern.maxfilesperproc=20480 || :
> -	sudo launchctl limit maxfiles 20480 || :
> -	ulimit -S -n 20480 || :
> -	ulimit -n
> -	make -j8
> -	cd test && ./test-run.py --force -j 1 unit/ app/ app-tap/ box/ box-tap/
> +test_ubuntu: deps_ubuntu runtest_ubuntu
>  
> -coverage_ubuntu: deps_ubuntu
> +build_cov_ubuntu:

I would use the word 'coverage' everywhere instead of 'coverage' and
'cov' here and there.

>  	cmake . -DCMAKE_BUILD_TYPE=Debug -DENABLE_GCOV=ON
> -	make -j8
> +	make -j
> +
> +runtest_cov_ubuntu: build_cov_ubuntu
>  	# Enable --long tests for coverage
> -	cd test && /usr/bin/python test-run.py --force -j 1 --long
> +	cd test && /usr/bin/python test-run.py --force $(NO_PARALLEL_TESTING) --long
>  	lcov --compat-libtool --directory src/ --capture --output-file coverage.info.tmp
>  	lcov --compat-libtool --remove coverage.info.tmp 'tests/*' 'third_party/*' '/usr/*' \
>  		--output-file coverage.info
> @@ -81,6 +81,61 @@ coverage_ubuntu: deps_ubuntu
>  		coveralls-lcov --service-name travis-ci --service-job-id $(TRAVIS_JOB_ID) --repo-token $(COVERALLS_TOKEN) coverage.info; \
>  	fi;
>  
> +coverage_ubuntu: deps_ubuntu runtest_cov_ubuntu
> +
> +#######
> +# OSX #
> +#######
> +
> +deps_osx:
> +	brew update
> +	brew install openssl readline curl icu4c --force
> +	python2 -V || brew install python2 --force
> +	curl --silent --show-error --retry 5 https://bootstrap.pypa.io/get-pip.py >get-pip.py
> +	python get-pip.py --user
> +	pip install --user --ignore-installed -r test-run/requirements.txt
> +
> +build_osx:
> +	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
> +	make -j
> +
> +runtest_osx: build_osx
> +	# Increase the maximum number of open file descriptors on macOS
> +	sudo sysctl -w kern.maxfiles=20480 || :
> +	sudo sysctl -w kern.maxfilesperproc=20480 || :
> +	sudo launchctl limit maxfiles 20480 || :
> +	ulimit -S -n 20480 || :
> +	ulimit -n
> +	cd test && ./test-run.py --force $(NO_PARALLEL_TESTING) unit/ app/ app-tap/ box/ box-tap/
> +
> +test_osx: deps_osx runtest_osx
> +
> +###########
> +# FreeBSD #
> +###########
> +
> +deps_freebsd:
> +	# don't use bad py27-msgpack-python

Can you link exact problem description here or in the commit message?

> +	sudo pkg install -y sudo git cmake gmake gcc coreutils \

> sudo pkg install -y sudo

If sudo is not installed, then this command will fail. If it is
installed, then we don't need to add it to the package list.

> +	readline ncurses libyaml openssl curl libunwind icu \
> +	python27 py27-pip py27-setuptools py27-daemon \
> +	py27-yaml py27-argparse py27-six py27-gevent \
> +	gdb bash vim

Installing vim in a testing environment is the strange thing.

> +
> +build_freebsd:
> +	cmake .

I would explicitly set a build type (I guess it worth to test
RelWithDebInfo), set -DENABLE_WERROR=ON and pass ${CMAKE_EXTRA_PARAMS}
here (even despite we don't use it right now).

> +	gmake

Use -j as in other make invocations?

> +	sudo make install

Why do we need to install tarantool into a system? test-run able to
detect in-source build.

> +
> +runtest_freebsd: build_freebsd
> +	cd test && /usr/bin/python test-run.py --force $(NO_PARALLEL_TESTING) || true

Volkswagen mode?

> +
> +test_freebsd: deps_freebsd runtest_freebsd
> +
> +###################
> +# Sources & Packs #
> +###################

I would say 'Source tarballs', because we have no such separate things
as 'source' and 'pack'. I mean, it is possible to undestand the header
in a wrong way.

> +
>  source:
>  	git clone https://github.com/packpack/packpack.git packpack
>  	TARBALL_COMPRESSOR=gz packpack/packpack tarball
> diff --git a/.travis.yml b/.travis.yml
> index e94d02ef5..a22197845 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -17,6 +17,9 @@ cache:
>  git:
>      depth: 100500
>  
> +env: >
> +    NO_PARALLEL_TESTING="-j 1"
> +
>  jobs:
>      include:
>        # Testing targets (just run tests on Debian Stretch or OS X).
> diff --git a/images/Dockerfile.debian b/images/Dockerfile.debian
> new file mode 100644
> index 000000000..eb5278343
> --- /dev/null
> +++ b/images/Dockerfile.debian
> @@ -0,0 +1,4 @@
> +FROM packpack/packpack:debian-stretch
> +
> +COPY .travis.mk .
> +RUN make -f .travis.mk deps_ubuntu
> diff --git a/images/Dockerfile.lto b/images/Dockerfile.lto
> new file mode 100644
> index 000000000..54c585acc
> --- /dev/null
> +++ b/images/Dockerfile.lto
> @@ -0,0 +1,5 @@
> +FROM packpack/packpack:debian-buster
> +
> +COPY .travis.mk .
> +RUN make -f .travis.mk deps_ubuntu
> +RUN make -f .travis.mk deps_buster_clang_8

I guess we can remove these files, se the comment to 'docker_bootstrap'
goal above.