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

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

Thanks for the patch!

See 5 comments below.

On 25/03/2020 23:01, Leonid Vasiliev wrote:
> From: Leonid <lvasiliev@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.

> 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.

> +    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?

> +        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.

>              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.

> +
>      local cmd_pair = commands[command_name]
>      if #arg < 2 then
>          log.error("Not enough arguments for '%s' command\n", command_name)