Re: [Tarantool-patches] [PATCH v1] test: fix OSX dependencies installation

["Alexander Tikhonov" <avtikhon@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5263 bytes --]


Hi Alexander, thanks for your review, I’ve made the changes you suggested.

  
>Понедельник, 23 марта 2020, 15:11 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
> 
>Looked briefly.
>
>It looks as a bunch of fixes for different problems. It seems they
>should be splitted to properly described separately.
>
>WBR, Alexander Turenko.
Right, I’ve moved all the changes for mac mini to separate patch.
>
>On Mon, Mar 23, 2020 at 02:49:24PM +0300, Alexander V. Tikhonov wrote:
>> Set in use Python 2 latest commit from tapped local
>> formula, since Python 2 is EOL.
>>
>> Set 'var' directory for test-run tool to the shorter
>> path name to avoid of issues with long names.
>> ---
>>
>> Github:  https://github.com/tarantool/tarantool/tree/avtikhon/osx_depends-full-ci
>> First commit from Github:  https://github.com/tarantool/tarantool/tree/avtikhon/osx_on_mini14-full-ci
>> Which already has LGTM:
>>  https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014734.html
>
>> diff --git a/.gitlab.mk b/.gitlab.mk
>> index b39c5c651..d804d3cf6 100644
>> --- a/.gitlab.mk
>> +++ b/.gitlab.mk
>> @@ -14,7 +14,7 @@ git_submodule_update:
>> git submodule update --recursive --init
>>
>> # Pass *_no_deps goals to .travis.mk.
>> -test_%_no_deps: git_submodule_update
>> +test_%: git_submodule_update
>> ${TRAVIS_MAKE} $@
>
>This looks suspectful. Will not it break Linux builds?
>
>The comment still states that this goal is about *_no_deps.
It has no any affect on any other jobs/tasks, so it can be used, anyway I’’ve
removed this change from this patch.
>
>> # #######################################################
>> diff --git a/.travis.mk b/.travis.mk
>> index 42969ff56..2a4ff8046 100644
>> --- a/.travis.mk
>> +++ b/.travis.mk
>> @@ -5,6 +5,7 @@
>> DOCKER_IMAGE?=packpack/packpack:debian-stretch
>> TEST_RUN_EXTRA_PARAMS?=
>> MAX_FILES?=65534
>> +MAX_PROC?=2500
>
>It seems you fixed some other problem, but it is not described neither
>in an issue, nor in the commit message.
Right, the MAX_PROC limit value should be set too, it was found running
OSX on hosts w/o previous changes.
>
>> @@ -127,13 +128,24 @@ test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps
>> # OSX #
>> #######
>>
>> +# since Python 2 is EOL it's latest commit from tapped local formula is used
>> +OSX_PKGS=openssl readline curl icu4c libiconv zlib autoconf automake libtool \
>> + cmake file://${PWD}/tools/brew_taps/tntpython2.rb
>> +
>> deps_osx:
>> - brew update
>> - brew install openssl readline curl icu4c libiconv zlib autoconf automake libtool --force
>> - python2 -V || brew install python2 --force
>> + # install brew using command from Homebrew repository instructions:
>> + #  https://github.com/Homebrew/install
>> + # NOTE: 'echo' command below is required since brew installation
>> + # script obliges the one to enter a newline for confirming the
>> + # installation via Ruby script.
>> + brew update || echo | /usr/bin/ruby -e \
>> + "$(curl -fsSL  https://raw.githubusercontent.com/Homebrew/install/master/install )"
>
>So you added brew installation when it is not installed. Add this to the
>commit message at least.
>
>Is it possible at all for a machine that is enabled into CI? Aren't you
>use brew to install gitlab runner?
I’ve added the commit message about it. This change is really needed to be sure that
our customers can use this script too.
>
>> + # try to install the packages either upgrade it to avoid of fails
>> + # if the package already exists with the previous version
>> + brew install --force ${OSX_PKGS} || brew upgrade ${OSX_PKGS}
>> curl --silent --show-error --retry 5  https://bootstrap.pypa.io/get-pip.py >get-pip.py
>> - python get-pip.py --user
>> - pip install --user --force-reinstall -r test-run/requirements.txt
>> + sudo python get-pip.py
>
>Why do we need get-pip.py if you already install python2 from tap?
Right, tapped formula for python2 installs pip — I’ve removed this code from
the make rule at all.
>
>> + pip install --force-reinstall -r test-run/requirements.txt
>
>> @@ -148,11 +160,16 @@ test_osx_no_deps: build_osx
>> # call as tests runs call.
>> # Tests: Temporary excluded replication/ suite with some tests
>> # from other suites by issues #4357 and #4370
>> - echo tarantool | sudo -S launchctl limit maxfiles ${MAX_FILES} || : ; \
>> + sudo -S launchctl limit maxfiles ${MAX_FILES} || : ; \
>> launchctl limit maxfiles || : ; \
>> ulimit -n ${MAX_FILES} || : ; \
>> ulimit -n ; \
>> - cd test && ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS) \
>> + sudo -S launchctl limit maxproc ${MAX_PROC} || : ; \
>> + launchctl limit maxproc || : ; \
>> + ulimit -u ${MAX_PROC} || : ; \
>> + ulimit -u ; \
>> + rm -rf /tmp/tnt ; \
>> + cd test && ./test-run.py --vardir /tmp/tnt --force $(TEST_RUN_EXTRA_PARAMS) \
>> app/ app-tap/ box/ box-py/ box-tap/ engine/ engine_long/ long_run-py/ luajit-tap/ \
>> replication-py/ small/ sql/ sql-tap/ swim/ unit/ vinyl/ wal_off/ xlog/ xlog-py/
>
>The problem with 'var' directory is not related to the new way to Python
>2 installation? Why everything is in one commit?
This commit is about OSX host setup for building and testing, so I think it is ok to set it here.
 
--
Alexander Tikhonov
 

[-- Attachment #2: Type: text/html, Size: 7720 bytes --]