[Tarantool-patches] [PATCH] rocks: forward options to luarocks

[lvasiliev]

Hi! Thanks for the review. See PATCH v2 and comments bellow.

On 04.04.2020 22:30, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 5 comments below.
> 
> On 25/03/2020 23:01, Leonid Vasiliev wrote:
>> From: Leonid <lvasiliev at tarantool.org>
>>
>> https://github.com/tarantool/tarantool/issues/4629
>> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4629-forward-flags
>>
>> Corresponding thread https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015143.html
>>
>> @Changelog
>> Add tarantoolctl rocks commands: build config download init lint new_version purge which write_rockspec
>> Fix forwarding flags to luarocks
>> Help section has been updated
> 
> 1. Please, remove ChangeLog from the commit message. We never put
> them here. All what is not added to the commit message should be
> put after '---' below. Branch, issue, and all other links and
> comments too. It was asked by Oleg too, but seems the changelog is
> still here.
Fixed
> 
>> The policy for check of luarocks flags has been changed (moved from tarantoolctl to luarocks).
>> We had a white-list of luarocks options in tarantoolctl before.
>> Now we have a black-list of options (for tarantool) in luarocks.
>> Help section has been updated.
>> Chdir has been moved to luarocks.
>>
>> @TarantoolBot document
>> Title: Update tarantoolctl rocks
>> tarantoolctl rocks commands has been added:
>> build
>> config
>> download
>> init
>> lint
>> new_version
>> purge
>> which
>> write_rockspec
>>
>> Closes #4629
>> ---
> 
> This is the place for all text not added to the commit
> message.
> 
>>   extra/dist/tarantoolctl.in | 157 ++++++++-------------------------------------
>>   third_party/luarocks       |   2 +-
>>   2 files changed, 28 insertions(+), 131 deletions(-)
>>
>> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
>> index 6daf866..66dfec8 100755
>> --- a/extra/dist/tarantoolctl.in
>> +++ b/extra/dist/tarantoolctl.in
>> @@ -21,7 +21,6 @@ ffi.cdef[[
>>   int kill(int pid, int sig);
>>   int isatty(int fd);
>>   int getppid(void);
>> -int chdir(const char *path);
>>   ]]
>>   
>>   local TIMEOUT_INFINITY = 100 * 365 * 86400
>> @@ -936,14 +935,6 @@ local function rocks()
>>       cfg.init()
>>       local util = require("luarocks.util")
>>       local command_line = require("luarocks.cmd")
>> -    local options = keyword_arguments
>> -    local key = nil
>> -    if options["only-server"] ~= nil then
>> -        key = find_arg("only%-server")
>> -    else
>> -        key = find_arg("server")
>> -    end
>> -    table.insert(positional_arguments, key)
>>   
>>       -- Tweak help messages
>>       util.see_help = function(command, program)
>> @@ -951,27 +942,36 @@ local function rocks()
>>           return "See Tarantool documentation for help."
>>       end
>>   
>> -    -- Enable only useful commands
>> +    -- Disabled: path, upload
>>       local rocks_commands = {
>> +        build = "luarocks.cmd.build",
>> +        config = "luarocks.cmd.config",
>> +        doc = "luarocks.cmd.doc",
>> +        download = "luarocks.cmd.download",
>> +        help = "luarocks.cmd.help",
>> +        init = "luarocks.cmd.init",
>>           install = "luarocks.cmd.install",
>> -        search = "luarocks.cmd.search",
>> +        lint = "luarocks.cmd.lint",
>>           list = "luarocks.cmd.list",
>> -        remove = "luarocks.cmd.remove",
>> -        show = "luarocks.cmd.show",
>>           make = "luarocks.cmd.make",
>> +        make_manifest = "luarocks.admin.cmd.make_manifest",
>> +        new_version = "luarocks.cmd.new_version",
>>           pack = "luarocks.cmd.pack",
>> -        unpack = "luarocks.cmd.unpack",
>> -        doc = "luarocks.cmd.doc",
>> -        help = "luarocks.cmd.help",
>> +        purge = "luarocks.cmd.purge",
>> +        remove = "luarocks.cmd.remove",
>> +        search = "luarocks.cmd.search",
>> +        show = "luarocks.cmd.show",
>>           test = "luarocks.cmd.test",
>> -        make_manifest = "luarocks.admin.cmd.make_manifest",
>> +        unpack = "luarocks.cmd.unpack",
>> +        which = "luarocks.cmd.which",
>> +        write_rockspec = "luarocks.cmd.write_rockspec",
>>       }
>>   
>> -    if keyword_arguments.chdir then
>> -        ffi.C.chdir(keyword_arguments.chdir)
>> -    end
>> +    --prepare arguments for luarocks
> 
> 2. Please, add whitespace after '--', start the
> sentence with a capital letter, and end with a dot.
Fixed
> 
>> +    shift_argv(lua_arguments, 0, 1)
>>       -- Call LuaRocks
>> -    command_line.run_command('', rocks_commands, nil, unpack(positional_arguments))
>> +    command_line.run_command('LuaRocks main command-line interface',
> 
> 3. I thought we should not print any 'LuaRocks' text when run
> 'tarantoolctl rocks'. So why did you change this?
Sorry, but I don't understand you.
Now we use luarocks help for luarocks.
Do you propose changing the Usage section?
> 
>> +        rocks_commands, nil, unpack(lua_arguments))
>>   end
>>   
>>   local function exit_wrapper(func)
>> @@ -1186,117 +1186,14 @@ local commands = setmetatable({
>>       }, rocks = {
>>           func = exit_wrapper(rocks), process = process_remote, help = {
>>               header =
>> -                "%s rocks [install|make|remove|show|search|list|pack|unpack|doc|test|make_manifest|help]",
>> +                "%s rocks - package managment",
> 
> 4. 'managment' -> 'management'. Also none of the commands print
> it like that. They print description and command line separately.
> See 'tarantoolctl help'. In the command line after 'rocks' should
> be said something like 'COMMAND'. And in the description you can
> provide, well, description.
managment has beeb fixed.
LuaRocks(rocks) is a submodule. I don't want to copy-paste all helps for 
all command with all flags (This is one of the goals of the task). 
Before the help was huge and useless. All other commands don't use 
submodules.
> 
>>               description =
>>   [=[
>> -        Package management.
>> +        See tarantoolctl rocks help
>>   ]=],
>> @@ -1411,7 +1305,10 @@ local function populate_arguments()
>>   end
>>   
>>   local function main()
>> -    populate_arguments()
>> +    if command_name ~= 'rocks' then -- rocks parse arguments themselves
>> +        populate_arguments()
>> +    end
> 
> 5. You probably don't need to change that. Let it parse.
> You don't use the result anyway.
We don't want to have double parsing. And this doesn't make any sense.
> 
>> +
>>       local cmd_pair = commands[command_name]
>>       if #arg < 2 then
>>           log.error("Not enough arguments for '%s' command\n", command_name)