

<HTML><BODY><div>Alexander, thanks a lot for the review, the lack of ‘--user’ did not break the testing on Travis-CI:<br><a href="https://www.travis-ci.org/github/tarantool/tarantool/jobs/666683564">https://www.travis-ci.org/github/tarantool/tarantool/jobs/666683564</a><br><br><br> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Среда, 25 марта 2020, 14:38 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15851362842138360182_BODY">If lack of --user (for pip) does not fail Travis CI jobs, then the whole<br>commit should be harmless. I cannot say that I'm happy with all changes,<br>but let's don't freeze on me: fix what you personally think should be<br>fixed and go to Kirill to push. No need to re-review with me.<br><br>WBR, Alexander Turenko.<br><br>On Mon, Mar 23, 2020 at 09:09:05PM +0300, Alexander V. Tikhonov wrote:<br>> Fixed OSX host setup for Tarantool build:<br>> - set brew installation from Homebrew repository instructions;<br>> - set in use Python 2 latest commit from tapped local formula,<br>> since Python 2 is EOL, also removed extra pip installation with<br>> get-pip script, because tapped formula installs pip itself;<br>> - added upgrade packages call to avoid of fails on already<br>> installed packages, but with previous version.<br>><br>> Fixed OSX host setup for Tarantool test:<br>> - set maximum processes limit value to 2500 for testing process;<br>> - set 'var' directory for test-run tool to the shorter path name<br>> to avoid of issues with long names;<br>> ---<br>><br>> Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/osx_depends-full-ci" target="_blank">https://github.com/tarantool/tarantool/tree/avtikhon/osx_depends-full-ci</a><br>> Based on 1st commit from Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/osx_on_mini14-full-ci" target="_blank">https://github.com/tarantool/tarantool/tree/avtikhon/osx_on_mini14-full-ci</a><br>> Which already has LGTM:<br>> <a href="https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014734.html" target="_blank">https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014734.html</a><br><br>> + brew update || echo | /usr/bin/ruby -e \<br>> + "$(curl -fsSL <a href="https://raw.githubusercontent.com/Homebrew/install/master/install" target="_blank">https://raw.githubusercontent.com/Homebrew/install/master/install</a>)"<br><br>Why don't update formulae after installing brew?</div></div></div></div></blockquote></div><div>The tapped formula does not exist in brew any more, though it can’t be updated.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>From the previous review [1]:<br><br>> > > So you added brew installation when it is not installed. Add this to the<br>> > > commit message at least.<br>> ><br>> > Is it possible at all for a machine that is enabled into CI? Aren't you<br>> > use brew to install gitlab runner?<br>> ><br>> > I’ve added the commit message about it. This change is really needed to be sure that<br>> > our customers can use this script too.<br><br>Are there any customer who run our testing? I don't know anyone. Even if<br>one doing it, (s)he prepares an environment and invoke `make test` or<br>`cd test && ./test-run.py`.<br><br>So it is the dead code. Personally I prefer to don't have dead code. If<br>you decided to do it, okay.<br><br>[1]: <a href="https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015062.html" target="_blank">https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015062.html</a></div></div></div></div></blockquote></div><div>Actually, I’ve meant that people who run on Mac hosts, like people in our group.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + # try to install the packages either upgrade it to avoid of fails<br>> + # if the package already exists with the previous version<br>> + brew install --force ${OSX_PKGS} || brew upgrade ${OSX_PKGS}<br>> + pip install --force-reinstall -r test-run/requirements.txt<br><br>--user (pip option) is not needed anymore? It was added to avoid using<br>of sudo or virtualenv. Is it related to a Travis CI infrastructure<br>change?</div></div></div></div></blockquote></div><div>Right, but in real it was added because of gitlab-ci, for now we have well configured OSX like travis’s.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>It looks a bit strance that pip w/o --user just works from 'travis'<br>user.</div></div></div></div></blockquote></div><div>You may check that testing passed with installation part without issues (check from line 8908):<br><a href="https://www.travis-ci.org/github/tarantool/tarantool/jobs/666683564">https://www.travis-ci.org/github/tarantool/tarantool/jobs/666683564</a></div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>Please, don't do such subtle changes. At least describe a reason in the<br>commit message. Otherwise it will be nightmare in the future, when one<br>will try to understand what is made for what, what is actual, what can<br>be simplified / removed / refactored.</div></div></div></div></blockquote></div><div>Ok, sure</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> + cd test && ./test-run.py --vardir /tmp/tnt --force $(TEST_RUN_EXTRA_PARAMS) \<br>> app/ app-tap/ box/ box-py/ box-tap/ engine/ engine_long/ long_run-py/ luajit-tap/ \<br>> replication-py/ small/ sql/ sql-tap/ swim/ unit/ vinyl/ wal_off/ xlog/ xlog-py/<br><br>Why not to use --exclude or TEST_RUN_EXCLUDE?<br><br>Are there any issue about disabled tests?<br><br>Why the whole suite is going to be disabled? To save time to investigate<br>problems?<br><br>When we'll investigate and enable tests rather then only disable them?<br>The past year trend to don't enable anything back…</div></div></div></div></blockquote></div><div>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.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>From the previous review [1]:<br><br>> > The problem with 'var' directory is not related to the new way to Python<br>> > 2 installation? Why everything is in one commit?<br>><br>> This commit is about OSX host setup for building and testing, so I<br>> think it is ok to set it here.<br><br>It sounds as 'because something going not right, when something was<br>changed'. Neat reason.<br><br>I cannot say neither 'ok', not 'not ok' for this. Okay, I can guess that<br>working directory is changed somehow, but why and where?</div></div></div></div></blockquote></div><div>‘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.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>Anyway, it should be harmless. So kinda ok.</div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Alexander Tikhonov</div></div></div><div> </div></div></BODY></HTML>