From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Bronnikov <sergeyb@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable Date: Wed, 5 Oct 2022 22:51:16 +0300 [thread overview] Message-ID: <Yz3gNBTva9qTTIw0@tarantool.org> (raw) In-Reply-To: <2771fb73-973b-3dd6-ef87-881f31dcda96@tarantool.org> Sergey, On 02.09.22, Sergey Bronnikov wrote: > Hi, > > On 31.08.2022 17:53, Igor Munkin wrote: > > Sergey, > > > > Thanks for your review! > > > > On 15.08.22, Sergey Bronnikov wrote: > >> Igor, > >> > >> thanks for the patch. See my comments inline. > >> > >> On 11.08.2022 14:17, Igor Munkin wrote: > >>> Before the patch both memprof and sysprof artefacts are generated within > >>> the binary artefacts tree (i.e. in the same directory LuaJIT binary is > >>> generated). However, more convenient way is producing these temporary > >>> profiles in a separate directory (e.g. located on the partition with > >>> more strict space limits). As a result of the patch all memprof and > >>> sysprof test chunks consider LUAJIT_TEST_VARDIR environment variable to > >>> set the directory where test profiles are generated. For the case when > >>> LUAJIT_TEST_VARDIR is not set, everything works as before. > >> Commit message is inconsistent a bit with a patch itself. > >> > >> As far as I understand many hunks are not related to introducing > >> LUAJIT_TEST_VARDIR. > >> > >> Probably it is better to change commit one-line message from "test: > >> introduce LUAJIT_TEST_VARDIR variable" > >> > >> to something like "test: refactoring and introduce LUAJIT_TEST_VARDIR > >> variable". It allows to keep patch > >> > >> as is and change expectations for those who will look at your patch. > > I believe your commit subject is too long, so I propose another > > solution. Since the only thing affected by LUAJIT_TEST_VARDIR is > > <utils.profilename>, so I can write something like "test: introduce > > utils.profilename helper" and describe its rationale and functions > > within the commit message. Are you OK with it? > > I'm ok! I've rewritten the commit message the following way: | test: introduce utils.profilename helper | | Before the patch both memprof and sysprof artefacts were generated | within the binary artefacts tree (i.e. in the same directory LuaJIT | binary is generated). However, more convenient way is producing these | temporary profiles in a separate directory (e.g. located on the | partition with more strict space limits). | | As a result of the patch <utils.profilename> helper is introduced and | all memprof and sysprof test chunks are patched to use it. The new | helper takes the basename of the profile to be created and also | considers LUAJIT_TEST_VARDIR environment variable to set the directory, | where test profiles are generated. For the case when LUAJIT_TEST_VARDIR | is not set, everything works as before. | | Part of tarantool/tarantool#7472 > > <snipped> -- Best regards, IM
next prev parent reply other threads:[~2022-10-05 20:01 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-11 11:17 [Tarantool-patches] [PATCH luajit 0/8] LuaJIT tests and CI enhancements Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable Igor Munkin via Tarantool-patches 2022-08-15 12:08 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 8:27 ` Sergey Kaplun via Tarantool-patches 2022-08-31 14:53 ` Igor Munkin via Tarantool-patches 2022-09-02 12:06 ` Sergey Bronnikov via Tarantool-patches 2022-10-05 19:51 ` Igor Munkin via Tarantool-patches [this message] 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 2/8] test: introduce MakeLuaPath.cmake helper Igor Munkin via Tarantool-patches 2022-08-15 12:08 ` Sergey Bronnikov via Tarantool-patches 2022-08-31 15:07 ` Igor Munkin via Tarantool-patches 2022-09-02 12:09 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 9:37 ` Sergey Kaplun via Tarantool-patches 2022-08-31 15:19 ` Igor Munkin via Tarantool-patches 2022-09-01 10:16 ` Sergey Kaplun via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 3/8] test: fix tarantool suite for out of source build Igor Munkin via Tarantool-patches 2022-08-15 12:10 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 9:49 ` Sergey Kaplun via Tarantool-patches 2022-08-31 17:20 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 4/8] ci: use out of source build in GitHub Actions Igor Munkin via Tarantool-patches 2022-08-15 12:13 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 9:58 ` Sergey Kaplun via Tarantool-patches 2022-08-31 15:34 ` Igor Munkin via Tarantool-patches 2022-08-31 15:33 ` Igor Munkin via Tarantool-patches 2022-09-02 12:09 ` Sergey Bronnikov via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 5/8] ci: remove excess parallel level setup Igor Munkin via Tarantool-patches 2022-08-15 12:14 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:09 ` Sergey Kaplun via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 6/8] ci: remove arch prefix for macOS M1 workflow Igor Munkin via Tarantool-patches 2022-08-15 12:17 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:14 ` Sergey Kaplun via Tarantool-patches 2022-08-31 15:55 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 7/8] ci: merge x86_64 and ARM64 workflows Igor Munkin via Tarantool-patches 2022-08-15 12:22 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:21 ` Sergey Kaplun via Tarantool-patches 2022-08-31 16:02 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 8/8] ci: merge Linux and macOS workflows Igor Munkin via Tarantool-patches 2022-08-15 12:27 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:32 ` Sergey Kaplun via Tarantool-patches 2022-11-11 8:56 ` [Tarantool-patches] [PATCH luajit 0/8] LuaJIT tests and CI enhancements Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yz3gNBTva9qTTIw0@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=sergeyb@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox