Re: [Tarantool-patches] [PATCH luajit] ci: conditionally install lua-cjson for perf job

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Hi,

thanks for the comments. LGTM after updating a commit message.

Sergey

On 4/28/26 15:29, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please consider my answers below.
>
> On 28.04.26, Sergey Bronnikov wrote:
>> Hi, Sergey!
>>
>> A few things worry me: we still don't know the root cause of the problem.
> It should be investigated and fixed in the scope of the 12600 [1]. For
> LuaJIT's CI, this workaround works just fine.
Probably it is worth to add to the issue that the problem is affected 
LuaJIT CI as well.
>
>> The proposed patch merely reduces the frequency of module installations.
> Yes, it installs the module only once.
>
>> And if an installation is required, the installation will still break.
> Not always, it usually passes successfully and fails only on the fourth
> run. So, 1 successful installation is enough to prevent flakiness, which
> is OK for our needs in LuaJIT. It is better to have some statistics
> instead of having none, isn't it?
Agree.
>> See comments inline.
>>
>> Sergey
>>
>> On 4/23/26 20:08, Sergey Kaplun wrote:
>>> We have encountered the flakiness of the network on our CI runners. It
>>> happens only on lua-cjson installation and only in some particular job
>>> run.
>>>
>>> This patch helps to deal with it by installing this package only if it
>> s/helps to deal with it/helps to reduce a number of installations/
> Don't get the comment about the number of installations. Maybe rephrase
> it like the following?
> | This patch workarounds it by installing the package only if ...

Ok, let's rephrase like you suggested.

Also, it is worth referring to the issue #12600 ("See also #12600").

>
>>> is not installed in the system. Also, it installs it only for Lua 5.1,
>>> which is compatible with LuaJIT.
>>> ---
>>>
>>> Branch:https://github.com/tarantool/luajit/tree/skaplun/ci-perf-install-conditionally-lua-cjson
>>>
>>>    .github/actions/setup-performance/action.yml | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/.github/actions/setup-performance/action.yml b/.github/actions/setup-performance/action.yml
>>> index 4e0e1929..3c2a8230 100644
>>> --- a/.github/actions/setup-performance/action.yml
>>> +++ b/.github/actions/setup-performance/action.yml
>>> @@ -11,7 +11,9 @@ runs:
>>>            apt install -y curl luarocks util-linux
>>>          shell: bash
>>>        - name: Install Lua modules
>>> -      run: luarocks install lua-cjson
>>> +      run: >
>>> +        luarocks --lua-version=5.1 show lua-cjson ||
>>> +        luarocks --lua-version=5.1 install lua-cjson
>> Why do we need specifying Lua version now? Previously, it worked without
>> setting exact version.
> I've preferred to describe it specifically since the most modern distros
> provide Lua 5.3+ by default. Should I add the comment in the commit
> message?
Yes, please.
>
>>>          shell: bash
>>>        - name: Run script to setup Linux environment
>>>          run: sh ./perf/helpers/setup_env.sh
> [1]:https://github.com/tarantool/tarantool/issues/12600
>

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