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

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

[Leonid Vasiliev]

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

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
---
 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
+    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',
+        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",
             description =
 [=[
-        Package management.
+        See tarantoolctl rocks help
 ]=],
             weight = 100,
             deprecated = false,
         },
-        subcommands = {
-            install = {
-                weight = 100,
-                help = {
-                    header = "%s rocks install {<rock>|<name> [<version>]}",
-                    description = [=[
-        Install a rock.
-]=],
-                }
-            },
-            make = {
-                weight = 101,
-                help = {
-                    header = "%s rocks make [<rockspec>]",
-                    description = [=[
-        Compile package in current directory using a rockspec.
-]=],
-                }
-            },
-            remove = {
-                weight = 101,
-                help = {
-                    header = "%s rocks remove <name> [<version>]",
-                    description = [=[
-        Uninstall a rock.
-]=],
-                }
-            },
-            show = {
-                weight = 110,
-                help = {
-                    header = "%s rocks list <filter>",
-                    description = [=[
-        Show information about an installed rock
-]=],
-                }
-            },
-            search = {
-                weight = 120,
-                help = {
-                    header = "%s rocks search <name> [<version>]",
-                    description = [=[
-        Search the rocks list for a pattern.
-]=],
-                }
-            },
-            list = {
-                weight = 121,
-                help = {
-                    header = "%s rocks list <filter>",
-                    description = [=[
-        List all installed rocks.
-]=],
-                }
-            },
-            pack = {
-                weight = 131,
-                help = {
-                    header = "%s rocks pack {<rockspec>|<name> [<version>]}",
-                    description = [=[
-        Create a rock, packing sources or binaries.
-]=],
-                }
-            },
-            unpack = {
-                weight = 131,
-                help = {
-                    header = "%s rocks unpack {<rock>|<name> [<version>]}",
-                    description = [=[
-        Unpack the contents of a rock.
-]=],
-                }
-            },
-            test = {
-                weight = 141,
-                help = {
-                    header = "%s rocks test [<rockspec>]",
-                    description = [=[
-        Run the test suite in the current directory.
-]=],
-                }
-            },
-            doc = {
-                weight = 151,
-                help = {
-                    header = "%s rocks doc <rockname>",
-                    description = [=[
-        Show documentation for an installed rock.
-]=],
-                }
-            },
-            make_manifest = {
-                weight = 161,
-                help = {
-                    header = "%s rocks make_manifest <argument>",
-                    description = [=[
-        Compile a manifest file for a repository.
-
-        <argument>, if given, is a local repository pathname.
-]=],
-                }
-            },
-        }
     }
 }, {
     __index = function()
@@ -1396,9 +1293,6 @@ local function populate_arguments()
         { 'help',        'boolean' },
         { 'format',      'string'  },
         { 'replica',     'number+' },
-        { 'chdir',       'string'  },
-        { 'only-server', 'string'  },
-        { 'server',      'string'  },
         { 'language',    'string'  },
     })
 
@@ -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
+
     local cmd_pair = commands[command_name]
     if #arg < 2 then
         log.error("Not enough arguments for '%s' command\n", command_name)
diff --git a/third_party/luarocks b/third_party/luarocks
index 06a657e..ca51fbd 160000
--- a/third_party/luarocks
+++ b/third_party/luarocks
@@ -1 +1 @@
-Subproject commit 06a657e7a68dd475e2afb5a56012c43f3a906640
+Subproject commit ca51fbdac5eda88a87f44ec97dccf9b262a23c59
-- 
2.7.4

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

[Oleg Babin]

Hi! Thanks for your patch I have several comments.

On 26/03/2020 01: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
Please put branch, issue and changelog under "---". This should not be a 
part of commit message.
 > Chdir has been moved to luarocks.

It's quite important thing to preserve backward compatibility.
How it works now:
```
tarantoolctl rocks make --chdir ../tracing

Warning: variable CFLAGS was not passed in build_variables
mkdir -p doc
tracing scm-1 is now installed in 
/Users/o.babin/Projects/tarantool/.rocks (license: BSD)
```

How it looks on your branch:
```
./src/tarantool ./extra/dist/tarantoolctl rocks make --chdir ../tracing

Error: Invalid argument: unknown flag --chdir. See --help.
```

And I don't see any mentions of this option in "rocks --help". May be I 
do something wrong? But make sure that behaviour of this option was not 
changed. It's quite important for solutions.

---
Oleg Babin

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

[lvasiliev]

Hi, thanks for the feedback.

On 01.04.2020 13:44, Oleg Babin wrote:
> Hi! Thanks for your patch I have several comments.
> 
> On 26/03/2020 01: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
> Please put branch, issue and changelog under "---". This should not be a 
> part of commit message.
>  > Chdir has been moved to luarocks.
> 
> It's quite important thing to preserve backward compatibility.
> How it works now:
> ```
> tarantoolctl rocks make --chdir ../tracing
> 
> Warning: variable CFLAGS was not passed in build_variables
> mkdir -p doc
> tracing scm-1 is now installed in 
> /Users/o.babin/Projects/tarantool/.rocks (license: BSD)
> ```
> 
> How it looks on your branch:
> ```
> ./src/tarantool ./extra/dist/tarantoolctl rocks make --chdir ../tracing
> 
> Error: Invalid argument: unknown flag --chdir. See --help.
> ```
> 
> And I don't see any mentions of this option in "rocks --help". May be I 
> do something wrong? But make sure that behaviour of this option was not 
> changed. It's quite important for solutions.
As I understand it, you use luarocks from the system, you need use 
luaroks from the repo (use export LUA_PATH=...).
> 
> ---
> Oleg Babin

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

[Oleg Babin]

Hi! Thanks for answers!

On 02/04/2020 11:30, lvasiliev wrote:
> As I understand it, you use luarocks from the system, you need use 
> luaroks from the repo (use export LUA_PATH=...).

Yes, you are right.
LUA_PATH="$PWD/third_party/luarocks/src/?.lua" ./src/tarantool 
extra/dist/tarantoolctl rocks make
It solved my problem. However, there is another error:

```
LUA_PATH="$PWD/third_party/luarocks/src/?.lua" ./src/tarantool 
extra/dist/tarantoolctl rocks make --chdir ../tracing

Error: Argument missing: please specify a rockspec to use on current 
directory.
```

But it worked before.

Ok. Let's add a rockspec.
```
LUA_PATH="$PWD/third_party/luarocks/src/?.lua;$PWD/src/?/init.lua" 
./src/tarantool extra/dist/tarantoolctl rocks make 
../tracing/tracing-scm-1.rockspec --chdir ../tracing
Missing dependencies for tracing scm-1:
    checks >= 3.0.1 (not installed)

tracing scm-1 depends on checks >= 3.0.1 (not installed)
checks not found for Lua 5.1.
Checking if available for other Lua versions...
Checking for Lua 5.2...
Checking for Lua 5.3...
Checking for Lua 5.4...

Error: Could not satisfy dependency checks >= 3.0.1: No results matching 
query were found for Lua 5.1.
```

Expected result - "checks" will be downloaded and installed as well as 
"tracing" in "$PWD/.rocks"

---
Oleg Babin

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

[lvasiliev]

diff --git a/third_party/luarocks b/third_party/luarocks
index ca51fbd..be88741 160000
--- a/third_party/luarocks
+++ b/third_party/luarocks
@@ -1 +1 @@
-Subproject commit ca51fbdac5eda88a87f44ec97dccf9b262a23c59
+Subproject commit be887418ebef8c6b43ff19e05e1e39f533a442f6

On 26.03.2020 1: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
> 
> 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
> ---
>   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
> +    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',
> +        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",
>               description =
>   [=[
> -        Package management.
> +        See tarantoolctl rocks help
>   ]=],
>               weight = 100,
>               deprecated = false,
>           },
> -        subcommands = {
> -            install = {
> -                weight = 100,
> -                help = {
> -                    header = "%s rocks install {<rock>|<name> [<version>]}",
> -                    description = [=[
> -        Install a rock.
> -]=],
> -                }
> -            },
> -            make = {
> -                weight = 101,
> -                help = {
> -                    header = "%s rocks make [<rockspec>]",
> -                    description = [=[
> -        Compile package in current directory using a rockspec.
> -]=],
> -                }
> -            },
> -            remove = {
> -                weight = 101,
> -                help = {
> -                    header = "%s rocks remove <name> [<version>]",
> -                    description = [=[
> -        Uninstall a rock.
> -]=],
> -                }
> -            },
> -            show = {
> -                weight = 110,
> -                help = {
> -                    header = "%s rocks list <filter>",
> -                    description = [=[
> -        Show information about an installed rock
> -]=],
> -                }
> -            },
> -            search = {
> -                weight = 120,
> -                help = {
> -                    header = "%s rocks search <name> [<version>]",
> -                    description = [=[
> -        Search the rocks list for a pattern.
> -]=],
> -                }
> -            },
> -            list = {
> -                weight = 121,
> -                help = {
> -                    header = "%s rocks list <filter>",
> -                    description = [=[
> -        List all installed rocks.
> -]=],
> -                }
> -            },
> -            pack = {
> -                weight = 131,
> -                help = {
> -                    header = "%s rocks pack {<rockspec>|<name> [<version>]}",
> -                    description = [=[
> -        Create a rock, packing sources or binaries.
> -]=],
> -                }
> -            },
> -            unpack = {
> -                weight = 131,
> -                help = {
> -                    header = "%s rocks unpack {<rock>|<name> [<version>]}",
> -                    description = [=[
> -        Unpack the contents of a rock.
> -]=],
> -                }
> -            },
> -            test = {
> -                weight = 141,
> -                help = {
> -                    header = "%s rocks test [<rockspec>]",
> -                    description = [=[
> -        Run the test suite in the current directory.
> -]=],
> -                }
> -            },
> -            doc = {
> -                weight = 151,
> -                help = {
> -                    header = "%s rocks doc <rockname>",
> -                    description = [=[
> -        Show documentation for an installed rock.
> -]=],
> -                }
> -            },
> -            make_manifest = {
> -                weight = 161,
> -                help = {
> -                    header = "%s rocks make_manifest <argument>",
> -                    description = [=[
> -        Compile a manifest file for a repository.
> -
> -        <argument>, if given, is a local repository pathname.
> -]=],
> -                }
> -            },
> -        }
>       }
>   }, {
>       __index = function()
> @@ -1396,9 +1293,6 @@ local function populate_arguments()
>           { 'help',        'boolean' },
>           { 'format',      'string'  },
>           { 'replica',     'number+' },
> -        { 'chdir',       'string'  },
> -        { 'only-server', 'string'  },
> -        { 'server',      'string'  },
>           { 'language',    'string'  },
>       })
>   
> @@ -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
> +
>       local cmd_pair = commands[command_name]
>       if #arg < 2 then
>           log.error("Not enough arguments for '%s' command\n", command_name)
> diff --git a/third_party/luarocks b/third_party/luarocks
> index 06a657e..ca51fbd 160000
> --- a/third_party/luarocks
> +++ b/third_party/luarocks
> @@ -1 +1 @@
> -Subproject commit 06a657e7a68dd475e2afb5a56012c43f3a906640
> +Subproject commit ca51fbdac5eda88a87f44ec97dccf9b262a23c59
> 

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

[Oleg Babin]

On 02/04/2020 12:56, lvasiliev wrote:
> diff --git a/third_party/luarocks b/third_party/luarocks
> index ca51fbd..be88741 160000
> --- a/third_party/luarocks
> +++ b/third_party/luarocks
> @@ -1 +1 @@
> -Subproject commit ca51fbdac5eda88a87f44ec97dccf9b262a23c59
> +Subproject commit be887418ebef8c6b43ff19e05e1e39f533a442f6

Thanks for changes! I've built tarantool from source and installed 
inside centos:7 docker container. `tarantoolctl rocks make --chdir ...` 
works as expected (as it was before).

---
Oleg Babin

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

[Vladislav Shpilevoy]

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)

Re: [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@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)

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

[Vladislav Shpilevoy]

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

No. I am saying that you changed 'LuaRocks' to 'tarantoolctl' in
luarocks/lvasiliev/gh-4629-add-chdir-to-make in some seemingly
random places. And here you again use 'LuaRocks'. But now I understood
why. You use 'tarantoolctl' as a command name, but 'LuaRocks' as the
module name. Which can be accessed via 'tarantoolctl' command among
other ways.

Also I just noticed a bad indentation here. Second line of
command_line.run_command should be aligned by (.

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

The help wasn't huge. It was small and consisted of one line
listing possible commands.

I didn't say what you cited above. I said that you violated
the 'help' text format. It is supposed to be in the form:

    tarantoolctl <command name> <options> <args>

    <Description>

You did this:

    tarantoolctl <command name> <description>

    <Proposal to use help != description>

So if someone taps 'tarantoolctl --help', he won't even understand
what is 'tarantoolctl rocks' and why would he need to see its 'help'
section.

Also I didn't say I want you to copy-paste all commands from luarocks
and their helps. I said I want a description. Since you seem not to be
willing to do that, let me write it for you below. Please, apply that
diff unless you have something against this. In the latter case lets
discuss.

====================
diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index b34f65558..f7dac4c9c 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -1186,10 +1186,14 @@ local commands = setmetatable({
     }, rocks = {
         func = exit_wrapper(rocks), process = process_remote, help = {
             header =
-                "%s rocks - package management",
+                "%s rocks [OPTIONS] COMMAND",
             description =
 [=[
-        See tarantoolctl rocks help
+        Tarantool package manager. To pack/unpack/install/remove
+        LuaRocks packages, and many more other actions. For more
+        information see
+
+            tarantoolctl rocks --help
 ]=],
             weight = 100,
             deprecated = false,
====================

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

Please, don't put comments on the same line as code, start
them with a capital letter, and end with a dot.

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

What is it? A tool called 1000000 times per second? This is not
perf critical code at all. But it is highly complicated due to
how many tasks tarantoolctl should do. And I want to keep it simple
and moduled. Your code currently violates encapsulation, because
argument parser seems to be an isolated piece of code, which does
not make exceptions for particular commands. Its task is just parse.
And you added some command-specific logic in here.

Also I tried to remove this check, and it appeared to be totally
not related to 'double parsing problem'. The real problem is that
taranctoolctl in the argument list looks for some names and may do
something unexpected depending on what it found.

After reverting this change I called 'taranctoolctl rocks --help' and
got just a help section from 'taranctoolctl', not the whole 'help from
'luarocks':

    $ ./extra/dist/tarantoolctl rocks --help
    Usage:

        tarantoolctl rocks - package management

            See tarantoolctl rocks help

But it does not mean you should break encapsulation here. You can add
a flag like 'is_self_sufficient' to elements of 'commands' array, and
set it to true for 'rocks'. Then you do this:

	diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
	index b34f65558..37eb89a39 100755
	--- a/extra/dist/tarantoolctl.in
	+++ b/extra/dist/tarantoolctl.in
	@@ -1305,11 +1305,11 @@ local function populate_arguments()
	 end
	 
	 local function main()
	-    if command_name ~= 'rocks' then -- rocks parse arguments themselves
	+    local cmd_pair = commands[command_name]
	+    if not cmd_pair.is_self_sufficient then
	         populate_arguments()
	     end
	 
	-    local cmd_pair = commands[command_name]
	     if #arg < 2 then
	         log.error("Not enough arguments for '%s' command\n", command_name)
	         usage(command_name)

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

[Leonid]

https://github.com/tarantool/tarantool/issues/4629
https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4629-forward-flags

---
 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 6daf866ac..66dfec88f 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
+    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',
+        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",
             description =
 [=[
-        Package management.
+        See tarantoolctl rocks help
 ]=],
             weight = 100,
             deprecated = false,
         },
-        subcommands = {
-            install = {
-                weight = 100,
-                help = {
-                    header = "%s rocks install {<rock>|<name> [<version>]}",
-                    description = [=[
-        Install a rock.
-]=],
-                }
-            },
-            make = {
-                weight = 101,
-                help = {
-                    header = "%s rocks make [<rockspec>]",
-                    description = [=[
-        Compile package in current directory using a rockspec.
-]=],
-                }
-            },
-            remove = {
-                weight = 101,
-                help = {
-                    header = "%s rocks remove <name> [<version>]",
-                    description = [=[
-        Uninstall a rock.
-]=],
-                }
-            },
-            show = {
-                weight = 110,
-                help = {
-                    header = "%s rocks list <filter>",
-                    description = [=[
-        Show information about an installed rock
-]=],
-                }
-            },
-            search = {
-                weight = 120,
-                help = {
-                    header = "%s rocks search <name> [<version>]",
-                    description = [=[
-        Search the rocks list for a pattern.
-]=],
-                }
-            },
-            list = {
-                weight = 121,
-                help = {
-                    header = "%s rocks list <filter>",
-                    description = [=[
-        List all installed rocks.
-]=],
-                }
-            },
-            pack = {
-                weight = 131,
-                help = {
-                    header = "%s rocks pack {<rockspec>|<name> [<version>]}",
-                    description = [=[
-        Create a rock, packing sources or binaries.
-]=],
-                }
-            },
-            unpack = {
-                weight = 131,
-                help = {
-                    header = "%s rocks unpack {<rock>|<name> [<version>]}",
-                    description = [=[
-        Unpack the contents of a rock.
-]=],
-                }
-            },
-            test = {
-                weight = 141,
-                help = {
-                    header = "%s rocks test [<rockspec>]",
-                    description = [=[
-        Run the test suite in the current directory.
-]=],
-                }
-            },
-            doc = {
-                weight = 151,
-                help = {
-                    header = "%s rocks doc <rockname>",
-                    description = [=[
-        Show documentation for an installed rock.
-]=],
-                }
-            },
-            make_manifest = {
-                weight = 161,
-                help = {
-                    header = "%s rocks make_manifest <argument>",
-                    description = [=[
-        Compile a manifest file for a repository.
-
-        <argument>, if given, is a local repository pathname.
-]=],
-                }
-            },
-        }
     }
 }, {
     __index = function()
@@ -1396,9 +1293,6 @@ local function populate_arguments()
         { 'help',        'boolean' },
         { 'format',      'string'  },
         { 'replica',     'number+' },
-        { 'chdir',       'string'  },
-        { 'only-server', 'string'  },
-        { 'server',      'string'  },
         { 'language',    'string'  },
     })
 
@@ -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
+
     local cmd_pair = commands[command_name]
     if #arg < 2 then
         log.error("Not enough arguments for '%s' command\n", command_name)
diff --git a/third_party/luarocks b/third_party/luarocks
index 06a657e7a..5b343b354 160000
--- a/third_party/luarocks
+++ b/third_party/luarocks
@@ -1 +1 @@
-Subproject commit 06a657e7a68dd475e2afb5a56012c43f3a906640
+Subproject commit 5b343b354ccfab10598090aefb02dc294eb60c2c
-- 
2.17.1