Re: [Tarantool-patches] [PATCH 2/3] Add a black list of the tarantoolctl options

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 05/04/2020 17:11, lvasiliev wrote:
> Hi! Thanks for the review. See PATCH v2 and comments bellow.
> 
> On 04.04.2020 22:02, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 3 comments below.
>>
>> On 25/03/2020 22:50, Leonid Vasiliev wrote:
>>> Luarocks code style has been used
>>
>> 1. This message does not give any useful info. Better explain, why
>> did you add the list to the luarocks instead of to tarantoolctl.
> It's about changes in tarantool and luaroks code style)

But it is not. You literally made functional changes here.
Code style is indentation level, naming policy and so on. This
patch banned some options. It is not a code style change.

> About black list.
> The main goal of the task:
> "The point of the task is refuse from double parsing of luarocks options (first in tarantoolctl and second in luarocks module), cleanup help and replace a white-list filter (most options are prohibited) to a black-list filter (most options allowed). So, if the functionality is work at luarocks, it must work at tarantoolctl rocks too"

Please, write something about that in the commit messages.
Currently the commit titles are close to useless since they
don't explain the changes. They just narrate the patchset.
I can see what patchset is doing without these titles. But I
don't see why. I don't see the whole picture. I cite these
messages here:

    Add the chdir option for make

    Add a black list for tarantoolctl rocks options

    Adapt luarocks help for tarantoolctl rocks

From these messages it is impossible to understand anything.
They seems like small refactoring changes from these titles
even though they are not. And the ticket is actually big, has
some discussion. Neither of this is reflected here.

> We don't want double parsing. And current parsing in tarantoolctl doesn't take into account matching commands with options.

This has nothing to do with double parsing. This is only
about functionality. By eliminating parsing from tarantoolctl
you will save a few microseconds in a tool, whose typical
operation lasts seconds.

But ok, I understood now. You don't want to complicate
tarantoolctl with checking options of the luarocks submodule.
It that case the blacklist checking should be in luarocks indeed.

This is also not reflected in the commit message.

>>> ---
>>>   src/luarocks/util.lua | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/src/luarocks/util.lua b/src/luarocks/util.lua
>>> index 8ccda27..0172d08 100644
>>> --- a/src/luarocks/util.lua
>>> +++ b/src/luarocks/util.lua
>>> @@ -171,6 +171,14 @@ local supported_flags = {
>>>      ["version"] = true,
>>>   }
>>>   +-- The tarantool unsupported arguments list.
>>> +local tarantool_black_list = {
>>> +   ["global"] = true,
>>> +   ["local"] = true,
>>> +   ["lua-version"] = true,
>>> +   ["lua-dir"] = true,
>>
>> 2. Why can't you just remove these flags from 'supported_flags'?
>> Or make them false there?
> Because the black-filter is more clearly. Easy to see the tarantool exceptions.

They are not 'tarantool' exceptions, from what I see. They are
'taranctoolctl rocks' exceptions. It is ok to depend on tarantool. But
not ok to depend on 'tarantoolctl'. Because this is a cyclic dependency.

These options either should be disabled on the whole, or as a last
resort a blacklist can be passed in cfg.init() by whoever invokes
luarocks tool. In our case - by tarantoolctl, here:

    local cfg = require("luarocks.core.cfg")
    cfg.init({option_blacklist = {...}})