[tarantool-patches] Re: [PATCH v2 1/2] Gitlab-ci updated after reiview - w/o perf
Alexander Turenko
alexander.turenko at tarantool.org
Tue Jun 18 14:07:36 MSK 2019
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.
More information about the Tarantool-patches
mailing list