From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 069BE2E0C9 for ; Tue, 18 Jun 2019 07:08:00 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id U8AZ5qLg0dsZ for ; Tue, 18 Jun 2019 07:07:59 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 2DCBC2A714 for ; Tue, 18 Jun 2019 07:07:58 -0400 (EDT) Date: Tue, 18 Jun 2019 14:07:36 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v2 1/2] Gitlab-ci updated after reiview - w/o perf Message-ID: <20190618110736.2xh44y4km2xhqjsy@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "Alexander V. Tikhonov" Cc: tarantool-patches@freelists.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.