From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 AA73045C304 for ; Tue, 1 Dec 2020 17:56:00 +0300 (MSK) Date: Tue, 1 Dec 2020 17:55:57 +0300 From: "Alexander V. Tikhonov" Message-ID: <20201201145557.GA143754@hpalx> References: <20201129013614.pygfn47szxofgxbi@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201129013614.pygfn47szxofgxbi@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v1] Move .tarantoolctl to test-run tool submodule List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi Alexander, thanks for the review and suggestions. I've made all of them, please check below. CI results are available here [1]. [1] - https://gitlab.com/tarantool/tarantool/-/pipelines/223728321 On Sun, Nov 29, 2020 at 04:36:14AM +0300, Alexander Turenko wrote: > > Github: https://github.com/tarantool/tarantool/tree/avtikhon/move_tarantoolctl_config > > > diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in > > index 0726e7f46..92a5c4225 100755 > > --- a/extra/dist/tarantoolctl.in > > +++ b/extra/dist/tarantoolctl.in > > @@ -70,6 +70,9 @@ local function check_user_level() > > local pwd = os.getenv('PWD') > > local udir = pwd and pwd .. '/.tarantoolctl' > > udir = udir and fio.stat(udir) and udir or nil > > + -- or test-run dir > > + udir = udir or pwd and pwd .. '/../test-run/.tarantoolctl' > > + udir = udir and fio.stat(udir) and udir or nil > > -- or home dir configuration > > local homedir = os.getenv('HOME') > > udir = udir or homedir and homedir .. '/.config/tarantool/tarantool' > > Now test-run copies test/.tarantoolctl to vardir. After [1] it should > copy it either from test/.tarantoolctl or from test-run/.tarantoolctl. > We don't need and should not change tarantoolctl for that. > > [1]: https://github.com/tarantool/test-run/pull/242 > Right, added PWD environment to be able to use .tarantoolctl file right from the test working directories. > > diff --git a/test-run b/test-run > > index 96dea99e6..1638eaa38 160000 > > --- a/test-run > > +++ b/test-run > > @@ -1 +1 @@ > > -Subproject commit 96dea99e69242380258d8dbccfb3735cce753d5a > > +Subproject commit 1638eaa384c9d2c30b6c2240add11e45d27ecd95 > > I suggest to just mention the PR in test-run, where the feature is > implemented. The logic is the following: when you send a patch for > review, it should be in the state that is ready to push to master. > However we should not update a submodule in master to a transient commit > (one that is not in the submodule's master / long term branch). > Otherwise we'll hit by problems like [2]. > > [2]: https://github.com/tarantool/small/issues/20 > It was temporary used just to check the changes, for now removed. > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > > index 10882c6a1..f1c8c8d46 100644 > > --- a/test/CMakeLists.txt > > +++ b/test/CMakeLists.txt > > @@ -80,10 +80,17 @@ add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test > > > > # Move tarantoolctl config > > if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR}) > > - configure_file( > > - "${PROJECT_SOURCE_DIR}/test/.tarantoolctl" > > - "${PROJECT_BINARY_DIR}/test/.tarantoolctl" > > + if(EXISTS "${PROJECT_SOURCE_DIR}/test/.tarantoolctl") > > + configure_file( > > + "${PROJECT_SOURCE_DIR}/test/.tarantoolctl" > > + "${PROJECT_BINARY_DIR}/test/.tarantoolctl" > > ) > > + else() > > + configure_file( > > + "${PROJECT_SOURCE_DIR}/test-run/.tarantoolctl" > > + "${PROJECT_BINARY_DIR}/test/.tarantoolctl" > > + ) > > + endif() > > endif() > > I didn't find anything like @BLABLA@ or ${BLABLA} in the file (see [3]). > So it just copies the file to the build directory. Since we want to ship > it within test-run, there is no need for copying anymore. > > It seems, the block can be just removed with explanation in the commit > message. `git blame` is your friend. > > [3]: https://cmake.org/cmake/help/latest/command/configure_file.html > Sure, removed and comment added in commit message. > > diff --git a/test/box/admin.result b/test/box/admin.result > > index 8c5626c36..59ab4791e 100644 > > --- a/test/box/admin.result > > +++ b/test/box/admin.result > > @@ -86,7 +86,7 @@ cfg_filter(box.cfg) > > - - replication_sync_lag > > - 10 > > - - replication_sync_timeout > > - - 300 > > + - 100 > > - - replication_synchro_quorum > > - 1 > > - - replication_synchro_timeout > > How about add it to _hide in test/box/box.lua with appropriate comment? > AFAIU we test default values here, but this one is not default anymore > (and controlled by test-run). If we'll pass another value into the > test-run option, the test would fail that does not look as a good > effect. > > The same for test/box/cfg.test.lua. Ok, made as suggested.