>Вторник, 18 июня 2019, 14:08 +03:00 от Alexander Turenko : >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. > Fixed. > >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. > Fixed. > >> + >> +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. > Not possible to use scripts for variables, neither use API to remove images from registry. Not resolved. > >BTW, I would prefer to use tags instead of suffixes; gitlab ci supports >it. > Fixed. > >> + 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. > Fixed, set name to: 'DENABLE_LTO_ON' > >> + 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.) > Working on. > >> + 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. Fixed. > > >> + - ${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). Fixed. > > >> + 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? Fixed. > > >> + 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'. Fixed like '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. > Fixed. > >> +DOCKER_BUILD=docker build --network=host > >I think there is no much reason to extract it from 'docker_bootstrap' >goal. > Fixed. > >> + >> +# 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. Fixed. > > >> + >> +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. Fixed. > > >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 > Fixed. > >> + 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. > Right, the comment for a set of goals there below. > >> +git_submodules_update: >> + ${GIT_SUBMODULES_UPDATE} --force > >Nit: The same as for GIT_SUBMODULES_UPDATE -> GIT_SUBMODULE_UPDATE: >git_submodules_update -> git_submodule_update. > Fixed. > >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? > Once I got the broken repository some how, '--force' flag fixed it, on CentOS 6 will need to fix it manually. > >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. > Fixed. > >> + >> +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)? > Fixed. > >> + '${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'? > Fixed. > >> + >> +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. > Fixed. > >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. > Once I got the broken repository some how, '--force' flag fixed it. Fixed, but left update submodules call. >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. > I don't think so, once I already got this issue with this wrong command and spent time to check all places where it was used, better to use the standalone target. > >> 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). > Fixed. > >> >> 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? Fixed. > > >> + 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. > Fixed. > >> + >> +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. > Fixed. > >> 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? Removed, obvious 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. Fixed. > > >> + 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. Fixed. > > >> + >> +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). Fixed. > > >> + gmake > >Use -j as in other make invocations? Fixed. > > >> + sudo make install > >Why do we need to install tarantool into a system? test-run able to >detect in-source build. > Fixed. > >> + >> +runtest_freebsd: build_freebsd >> + cd test && /usr/bin/python test-run.py --force $(NO_PARALLEL_TESTING) || true > >Volkswagen mode? Currently FreeBSD testing is not ready and will fail pipeline, the issue in github exists, but we really need the test result to check the new fails. > > >> + >> +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. > Fixed. > >> + >> 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. > Fixed. > > -- Alexander Tikhonov