From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AC6C8441841 for ; Wed, 25 Mar 2020 15:30:05 +0300 (MSK) Date: Wed, 25 Mar 2020 15:30:07 +0300 From: Alexander Turenko Message-ID: <20200325123007.ttvttnuev3xxf5ey@tkn_work_nb> References: <20200325113806.o5xjq7w4gw6dd25z@tkn_work_nb> <1585138345.846789982@f416.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1585138345.846789982@f416.i.mail.ru> Subject: Re: [Tarantool-patches] [PATCH v2] test: fix OSX host setup List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Tikhonov Cc: Oleg Piskunov , tarantool-patches@dev.tarantool.org > >> + brew update || echo | /usr/bin/ruby -e \ > >> + "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install )" > > > >Why don't update formulae after installing brew? > > The tapped formula does not exist in brew any more, though it can’t be > updated. After installing brew you'll have fresh brew, but no formulae, I guess. Will the next `brew install` work? I guess, no. You need `brew update` after this. So it seems logical to do `[ -z "$(which brew) "] && ruby brew_install_script; brew update` rather than `brew update || ruby brew_install_script`. > >From the previous review [1]: > > > >> > > 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. > > > >Are there any customer who run our testing? I don't know anyone. Even if > >one doing it, (s)he prepares an environment and invoke `make test` or > >`cd test && ./test-run.py`. > > > >So it is the dead code. Personally I prefer to don't have dead code. If > >you decided to do it, okay. > > > >[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015062.html > > Actually, I’ve meant that people who run on Mac hosts, like people > in our group. Developers doing `make test` or `cd test && ./test-run.py`. Nobody want to run a testing script and found that something was changed in its system. So I'm still on my view. > >> + # 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} > >> + pip install --force-reinstall -r test-run/requirements.txt > > > >--user (pip option) is not needed anymore? It was added to avoid using > >of sudo or virtualenv. Is it related to a Travis CI infrastructure > >change? > > Right, but in real it was added because of gitlab-ci, for now we have > well configured OSX like travis’s. 'It' -- the option? Okay so. Good piece of information to add to the commit message. > >It looks a bit strance that pip w/o --user just works from 'travis' > >user. > You may check that testing passed with installation part without issues (check from line 8908): > https://www.travis-ci.org/github/tarantool/tarantool/jobs/666683564 Okay. Yep, it seems pip assumes --user by default in Travis CI. Maybe I wrongly remember that --user was added for Travis CI (AFAIR it was done by Arseniy). > >> + 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/ > > > >Why not to use --exclude or TEST_RUN_EXCLUDE? > > > >Are there any issue about disabled tests? > > > >Why the whole suite is going to be disabled? To save time to investigate > >problems? > > > >When we'll investigate and enable tests rather then only disable them? > >The past year trend to don't enable anything back… > > This commit is already complete of changes and the change on the > testing suites better to make in the next patch, due to the list of > suites were not changed. Let's add the info about Mac OS X testing into the relevant issue to track disabled tests (AFAIR you have one, but it was about testing during RPM packages build). > >From the previous review [1]: > > > >> > 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. > > > >It sounds as 'because something going not right, when something was > >changed'. Neat reason. > > > >I cannot say neither 'ok', not 'not ok' for this. Okay, I can guess that > >working directory is changed somehow, but why and where? > > ‘var’ directory lays too deep at the path and the length of the path > is too long for test-run.py tool which fails because of it, the > solution to fix it is to use the short link for the ‘var’ directory. Why it was okay, but becomes too long?