[Tarantool-patches] [PATCH LUAROCKS v2 0/2] Move a rocks options filter from tarantoolctl to luarocks

[Tarantool-patches] [PATCH LUAROCKS v2 0/2] Move a rocks options filter from tarantoolctl to luarocks

[Leonid]

https://github.com/tarantool/tarantool/issues/4629
https://github.com/tarantool/luarocks/tree/lvasiliev/gh-4629-add-chdir-to-make

Changes in v2:
- Move chdir before loading the rockspec file

Leonid (2):
  Add the chdir option for make
  Add the tarantool options black list. Update help

 src/luarocks/cmd/help.lua | 15 +++++++++------
 src/luarocks/cmd/make.lua | 22 ++++++++++++++++------
 src/luarocks/util.lua     | 25 ++++++++++++++++++++-----
 3 files changed, 45 insertions(+), 17 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH LUAROCKS v2 1/2] Add the chdir option for make

[Leonid]

---
 src/luarocks/cmd/make.lua | 22 ++++++++++++++++------
 src/luarocks/util.lua     | 11 ++++++-----
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/src/luarocks/cmd/make.lua b/src/luarocks/cmd/make.lua
index 4d81386..4cd63f1 100644
--- a/src/luarocks/cmd/make.lua
+++ b/src/luarocks/cmd/make.lua
@@ -1,7 +1,7 @@
 
 --- Module implementing the LuaRocks "make" command.
 -- Builds sources in the current directory, but unlike "build",
--- it does not fetch sources, etc., assuming everything is 
+-- it does not fetch sources, etc., assuming everything is
 -- available in the current directory.
 local make = {}
 
@@ -20,7 +20,7 @@ make.help_summary = "Compile package in current directory using a rockspec."
 make.help_arguments = "[--pack-binary-rock] [<rockspec>]"
 make.help = [[
 Builds sources in the current directory, but unlike "build",
-it does not fetch sources, etc., assuming everything is 
+it does not fetch sources, etc., assuming everything is
 available in the current directory. If no argument is given,
 it looks for a rockspec in the current directory and in "rockspec/"
 and "rockspecs/" subdirectories, picking the rockspec with newest version
@@ -28,7 +28,7 @@ or without version name. If rockspecs for different rocks are found
 or there are several rockspecs without version, you must specify which to use,
 through the command-line.
 
-This command is useful as a tool for debugging rockspecs. 
+This command is useful as a tool for debugging rockspecs.
 To install rocks, you'll normally want to use the "install" and
 "build" commands. See the help on those for details.
 
@@ -45,7 +45,7 @@ only dependencies of the rockspec (see `luarocks help install`).
                     in the configuration file.
 
 --branch=<name>     Override the `source.branch` field in the loaded
-                    rockspec. Allows to specify a different branch to 
+                    rockspec. Allows to specify a different branch to
                     fetch. Particularly for "dev" rocks.
 
 --verify            Verify signature of the rockspec or src.rock being
@@ -59,6 +59,8 @@ only dependencies of the rockspec (see `luarocks help install`).
 --sign              To be used with --pack-binary-rock. Also produce
                     a signature file for the generated .rock file.
 
+--chdir=<path>      Specify a source directory of the rock.
+
 ]]
 
 --- Driver function for "make" command.
@@ -67,7 +69,7 @@ only dependencies of the rockspec (see `luarocks help install`).
 -- error message otherwise. exitcode is optionally returned.
 function make.command(flags, rockspec_filename)
    assert(type(rockspec_filename) == "string" or not rockspec_filename)
-   
+
    if not rockspec_filename then
       local err
       rockspec_filename, err = util.get_default_rockspec()
@@ -78,7 +80,14 @@ function make.command(flags, rockspec_filename)
    if not rockspec_filename:match("rockspec$") then
       return nil, "Invalid argument: 'make' takes a rockspec as a parameter. "..util.see_help("make")
    end
-   
+
+   if flags["chdir"] then
+      local ok, err = fs.change_dir(flags["chdir"])
+      if not ok then
+         return nil, err
+      end
+   end
+
    local rockspec, err, errcode = fetch.load_rockspec(rockspec_filename)
    if not rockspec then
       return nil, err
@@ -96,6 +105,7 @@ function make.command(flags, rockspec_filename)
       verify = not not flags["verify"],
    })
 
+
    if flags["sign"] and not flags["pack-binary-rock"] then
       return nil, "In the make command, --sign is meant to be used only with --pack-binary-rock"
    end
diff --git a/src/luarocks/util.lua b/src/luarocks/util.lua
index abf6d90..fadc93f 100644
--- a/src/luarocks/util.lua
+++ b/src/luarocks/util.lua
@@ -33,7 +33,7 @@ local debug = require("debug")
 -- which can be used to remove the item later from the list.
 function util.schedule_function(f, ...)
    assert(type(f) == "function")
-   
+
    local item = { fn = f, args = {...} }
    table.insert(scheduled_functions, item)
    return item
@@ -92,6 +92,7 @@ local supported_flags = {
    ["binary"] = true,
    ["branch"] = "<branch-name>",
    ["build-deps"] = true,
+   ["chdir"] = "<path>",
    ["debug"] = true,
    ["deps"] = true,
    ["deps-mode"] = "<mode>",
@@ -276,12 +277,12 @@ end
 -- exists in vars. Only string values are processed; this function
 -- does not scan subtables recursively.
 -- @param tbl table: Table to have its string values modified.
--- @param vars table: Table containing string-string key-value pairs 
+-- @param vars table: Table containing string-string key-value pairs
 -- representing variables to replace in the strings values of tbl.
 function util.variable_substitutions(tbl, vars)
    assert(type(tbl) == "table")
    assert(type(vars) == "table")
-   
+
    local updated = {}
    for k, v in pairs(tbl) do
       if type(v) == "string" then
@@ -698,9 +699,9 @@ end
 
 function util.opts_table(type_name, valid_opts)
    local opts_mt = {}
-   
+
    opts_mt.__index = opts_mt
-   
+
    function opts_mt.type()
       return type_name
    end
-- 
2.17.1

Re: [Tarantool-patches] [PATCH LUAROCKS v2 1/2] Add the chdir option for make

[Vladislav Shpilevoy]

Thanks for the patch!

First of all, please, drop the unnecessary diff. 11 from 14 hunks
of your commit are not needed, and their drop does not affect
anything.

Secondly, please, provide a descriptive commit message. Why changes
were made, and how can I test them. I don't know anything about rocks
and don't use them, so it would be cool if you could give me the exact
commands I should try in my console to check how your patch works, and
what is changed.

Also your commit says "add chdir option". So it assumes the public
behaviour is changed. Is it? If it is, then add a docbot request, if
these options are documented anywhere. Otherwise the commit title is
misleading.

On 19/11/2019 09:52, Leonid wrote:
> ---
>  src/luarocks/cmd/make.lua | 22 ++++++++++++++++------
>  src/luarocks/util.lua     | 11 ++++++-----
>  2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/src/luarocks/cmd/make.lua b/src/luarocks/cmd/make.lua
> index 4d81386..4cd63f1 100644
> --- a/src/luarocks/cmd/make.lua
> +++ b/src/luarocks/cmd/make.lua
> @@ -1,7 +1,7 @@
>  
>  --- Module implementing the LuaRocks "make" command.
>  -- Builds sources in the current directory, but unlike "build",
> --- it does not fetch sources, etc., assuming everything is 
> +-- it does not fetch sources, etc., assuming everything is
>  -- available in the current directory.
>  local make = {}
>  
> @@ -20,7 +20,7 @@ make.help_summary = "Compile package in current directory using a rockspec."
>  make.help_arguments = "[--pack-binary-rock] [<rockspec>]"
>  make.help = [[
>  Builds sources in the current directory, but unlike "build",
> -it does not fetch sources, etc., assuming everything is 
> +it does not fetch sources, etc., assuming everything is
>  available in the current directory. If no argument is given,
>  it looks for a rockspec in the current directory and in "rockspec/"
>  and "rockspecs/" subdirectories, picking the rockspec with newest version
> @@ -28,7 +28,7 @@ or without version name. If rockspecs for different rocks are found
>  or there are several rockspecs without version, you must specify which to use,
>  through the command-line.
>  
> -This command is useful as a tool for debugging rockspecs. 
> +This command is useful as a tool for debugging rockspecs.
>  To install rocks, you'll normally want to use the "install" and
>  "build" commands. See the help on those for details.
>  
> @@ -45,7 +45,7 @@ only dependencies of the rockspec (see `luarocks help install`).
>                      in the configuration file.
>  
>  --branch=<name>     Override the `source.branch` field in the loaded
> -                    rockspec. Allows to specify a different branch to 
> +                    rockspec. Allows to specify a different branch to
>                      fetch. Particularly for "dev" rocks.
>  
>  --verify            Verify signature of the rockspec or src.rock being
> @@ -59,6 +59,8 @@ only dependencies of the rockspec (see `luarocks help install`).
>  --sign              To be used with --pack-binary-rock. Also produce
>                      a signature file for the generated .rock file.
>  
> +--chdir=<path>      Specify a source directory of the rock.
> +
>  ]]
>  
>  --- Driver function for "make" command.
> @@ -67,7 +69,7 @@ only dependencies of the rockspec (see `luarocks help install`).
>  -- error message otherwise. exitcode is optionally returned.
>  function make.command(flags, rockspec_filename)
>     assert(type(rockspec_filename) == "string" or not rockspec_filename)
> -   
> +
>     if not rockspec_filename then
>        local err
>        rockspec_filename, err = util.get_default_rockspec()
> @@ -78,7 +80,14 @@ function make.command(flags, rockspec_filename)
>     if not rockspec_filename:match("rockspec$") then
>        return nil, "Invalid argument: 'make' takes a rockspec as a parameter. "..util.see_help("make")
>     end
> -   
> +
> +   if flags["chdir"] then
> +      local ok, err = fs.change_dir(flags["chdir"])
> +      if not ok then
> +         return nil, err
> +      end
> +   end
> +
>     local rockspec, err, errcode = fetch.load_rockspec(rockspec_filename)
>     if not rockspec then
>        return nil, err
> @@ -96,6 +105,7 @@ function make.command(flags, rockspec_filename)
>        verify = not not flags["verify"],
>     })
>  
> +
>     if flags["sign"] and not flags["pack-binary-rock"] then
>        return nil, "In the make command, --sign is meant to be used only with --pack-binary-rock"
>     end
> diff --git a/src/luarocks/util.lua b/src/luarocks/util.lua
> index abf6d90..fadc93f 100644
> --- a/src/luarocks/util.lua
> +++ b/src/luarocks/util.lua
> @@ -33,7 +33,7 @@ local debug = require("debug")
>  -- which can be used to remove the item later from the list.
>  function util.schedule_function(f, ...)
>     assert(type(f) == "function")
> -   
> +
>     local item = { fn = f, args = {...} }
>     table.insert(scheduled_functions, item)
>     return item
> @@ -92,6 +92,7 @@ local supported_flags = {
>     ["binary"] = true,
>     ["branch"] = "<branch-name>",
>     ["build-deps"] = true,
> +   ["chdir"] = "<path>",
>     ["debug"] = true,
>     ["deps"] = true,
>     ["deps-mode"] = "<mode>",
> @@ -276,12 +277,12 @@ end
>  -- exists in vars. Only string values are processed; this function
>  -- does not scan subtables recursively.
>  -- @param tbl table: Table to have its string values modified.
> --- @param vars table: Table containing string-string key-value pairs 
> +-- @param vars table: Table containing string-string key-value pairs
>  -- representing variables to replace in the strings values of tbl.
>  function util.variable_substitutions(tbl, vars)
>     assert(type(tbl) == "table")
>     assert(type(vars) == "table")
> -   
> +
>     local updated = {}
>     for k, v in pairs(tbl) do
>        if type(v) == "string" then
> @@ -698,9 +699,9 @@ end
>  
>  function util.opts_table(type_name, valid_opts)
>     local opts_mt = {}
> -   
> +
>     opts_mt.__index = opts_mt
> -   
> +
>     function opts_mt.type()
>        return type_name
>     end
> 

[Tarantool-patches] [PATCH LUAROCKS v2 2/2] Add the tarantool options black list. Update help

[Leonid]

---
 src/luarocks/cmd/help.lua | 15 +++++++++------
 src/luarocks/util.lua     | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/luarocks/cmd/help.lua b/src/luarocks/cmd/help.lua
index dcc9e35..1db914a 100644
--- a/src/luarocks/cmd/help.lua
+++ b/src/luarocks/cmd/help.lua
@@ -12,6 +12,10 @@ local dir = require("luarocks.dir")
 
 local program = util.this_program("luarocks")
 
+if program:find("tarantoolctl") then
+   program = program.." rocks"
+end
+
 help.help_summary = "Help on commands. Type '"..program.." help <command>' for more."
 
 help.help_arguments = "[<command>]"
@@ -20,7 +24,11 @@ help.help = [[
 ]]
 
 local function print_banner()
-   util.printout("\nLuaRocks "..cfg.program_version..", the Lua package manager")
+   if program:find("tarantoolctl") then
+      util.printout("\nTarantoolctl rocks, the Lua package manager based on LuaRocks "..cfg.program_version)
+   else
+      util.printout("\nLuaRocks "..cfg.program_version..", the Lua package manager")
+   end
 end
 
 local function print_section(section)
@@ -62,12 +70,7 @@ function help.command(description, commands, command)
 	                       (overrides any entries in the config file)
 	--only-sources=<url>   Restrict downloads to paths matching the
 	                       given URL.
-        --lua-dir=<prefix>     Which Lua installation to use.
-	--lua-version=<ver>    Which Lua version to use.
 	--tree=<tree>          Which tree to operate on.
-	--local                Use the tree in the user's home directory.
-	                       To enable it, see ']]..program..[[ help path'.
-	--global               Use the system tree when `local_by_default` is `true`.
 	--verbose              Display verbose output of commands executed.
 	--timeout=<seconds>    Timeout on network operations, in seconds.
 	                       0 means no timeout (wait forever).
diff --git a/src/luarocks/util.lua b/src/luarocks/util.lua
index fadc93f..660d140 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,
+}
+
 --- Extract flags from an arguments list.
 -- Given string arguments, extract flag arguments into a flags set.
 -- For example, given "foo", "--tux=beep", "--bla", "bar", "--baz",
@@ -189,6 +197,9 @@ function util.parse_flags(...)
       elseif state == "initial" and flag then
          local var,val = flag:match("([a-z_%-]*)=(.*)")
          if val then
+            if tarantool_black_list[var] then
+               return { ERROR = "Invalid argument: flag --"..var.." does not supported by tarantoolctl roks." }
+            end
             local vartype = supported_flags[var]
             if type(vartype) == "string" then
                if val == "" and vartype:sub(1,1) ~= '"' then
@@ -204,6 +215,9 @@ function util.parse_flags(...)
             end
          else
             local var = flag
+            if tarantool_black_list[var] then
+               return { ERROR = "Invalid argument: flag --"..var.." does not supported by tarantoolctl roks." }
+            end
             local vartype = supported_flags[var]
             if type(vartype) == "string" then
                i = i + 1
-- 
2.17.1

Re: [Tarantool-patches] [PATCH LUAROCKS v2 2/2] Add the tarantool options black list. Update help

[Vladislav Shpilevoy]

Thanks for the patch!

The same comment regarding the commit description as
for the previous commit.

Also see 3 comments below.

On 19/11/2019 09:52, Leonid wrote:
> ---
>  src/luarocks/cmd/help.lua | 15 +++++++++------
>  src/luarocks/util.lua     | 14 ++++++++++++++
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/src/luarocks/cmd/help.lua b/src/luarocks/cmd/help.lua
> index dcc9e35..1db914a 100644
> --- a/src/luarocks/cmd/help.lua
> +++ b/src/luarocks/cmd/help.lua
> @@ -20,7 +24,11 @@ help.help = [[
>  ]]
>  
>  local function print_banner()
> -   util.printout("\nLuaRocks "..cfg.program_version..", the Lua package manager")
> +   if program:find("tarantoolctl") then
> +      util.printout("\nTarantoolctl rocks, the Lua package manager based on LuaRocks "..cfg.program_version)
> +   else
> +      util.printout("\nLuaRocks "..cfg.program_version..", the Lua package manager")
> +   end

1. These lines are huge. Please, keep them inside 80 symbols border.

>  end
>  
>  local function print_section(section)
> @@ -62,12 +70,7 @@ function help.command(description, commands, command)
>  	                       (overrides any entries in the config file)
>  	--only-sources=<url>   Restrict downloads to paths matching the
>  	                       given URL.
> -        --lua-dir=<prefix>     Which Lua installation to use.
> -	--lua-version=<ver>    Which Lua version to use.
>  	--tree=<tree>          Which tree to operate on.
> -	--local                Use the tree in the user's home directory.
> -	                       To enable it, see ']]..program..[[ help path'.
> -	--global               Use the system tree when `local_by_default` is `true`.
>  	--verbose              Display verbose output of commands executed.
>  	--timeout=<seconds>    Timeout on network operations, in seconds.
>  	                       0 means no timeout (wait forever).
> diff --git a/src/luarocks/util.lua b/src/luarocks/util.lua
> index fadc93f..660d140 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,
> +}
> +
>  --- Extract flags from an arguments list.
>  -- Given string arguments, extract flag arguments into a flags set.
>  -- For example, given "foo", "--tux=beep", "--bla", "bar", "--baz",
> @@ -189,6 +197,9 @@ function util.parse_flags(...)
>        elseif state == "initial" and flag then
>           local var,val = flag:match("([a-z_%-]*)=(.*)")
>           if val then
> +            if tarantool_black_list[var] then
> +               return { ERROR = "Invalid argument: flag --"..var.." does not supported by tarantoolctl roks." }

2. does -> is.

The same below.

3. You seem to try to distinguish the tool for tarantoolctl
and non-tarantoolctl from what I see in help.lua. But you
check the blacklist even if the code works not from tarantoolctl.
And even print a message about tarantoolctl.Why?

> +            end
>              local vartype = supported_flags[var]
>              if type(vartype) == "string" then
>                 if val == "" and vartype:sub(1,1) ~= '"' then
> @@ -204,6 +215,9 @@ function util.parse_flags(...)
>              end
>           else
>              local var = flag
> +            if tarantool_black_list[var] then
> +               return { ERROR = "Invalid argument: flag --"..var.." does not supported by tarantoolctl roks." }
> +            end
>              local vartype = supported_flags[var]
>              if type(vartype) == "string" then
>                 i = i + 1
> 

Re: [Tarantool-patches] [PATCH LUAROCKS v2 0/2] Move a rocks options filter from tarantoolctl to luarocks

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

Generally the comments are the same as for the other rocks-related
patchset:

- Add a @ChangeLog;
- Provide a description of the patchset in the cover-letter. What
  was made, how, why?

On 19/11/2019 09:52, Leonid wrote:
> https://github.com/tarantool/tarantool/issues/4629
> https://github.com/tarantool/luarocks/tree/lvasiliev/gh-4629-add-chdir-to-make
> 
> Changes in v2:
> - Move chdir before loading the rockspec file
> 
> Leonid (2):
>   Add the chdir option for make
>   Add the tarantool options black list. Update help
> 
>  src/luarocks/cmd/help.lua | 15 +++++++++------
>  src/luarocks/cmd/make.lua | 22 ++++++++++++++++------
>  src/luarocks/util.lua     | 25 ++++++++++++++++++++-----
>  3 files changed, 45 insertions(+), 17 deletions(-)
> 

Re: [Tarantool-patches] [PATCH LUAROCKS v2 0/2] Move a rocks options filter from tarantoolctl to luarocks

[lvasiliev]

On 19.03.2020 1:12, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> Generally the comments are the same as for the other rocks-related
> patchset:
> 
> - Add a @ChangeLog;
> - Provide a description of the patchset in the cover-letter. What
>    was made, how, why?
> 
> On 19/11/2019 09:52, Leonid wrote:
>> https://github.com/tarantool/tarantool/issues/4629
>> https://github.com/tarantool/luarocks/tree/lvasiliev/gh-4629-add-chdir-to-make
>>
>> Changes in v2:
>> - Move chdir before loading the rockspec file
>>
>> Leonid (2):
>>    Add the chdir option for make
>>    Add the tarantool options black list. Update help
>>
>>   src/luarocks/cmd/help.lua | 15 +++++++++------
>>   src/luarocks/cmd/make.lua | 22 ++++++++++++++++------
>>   src/luarocks/util.lua     | 25 ++++++++++++++++++++-----
>>   3 files changed, 45 insertions(+), 17 deletions(-)
>>
Hi. Thanks for the review!
New version of patchset with the corrections was sent.
Sorry, in first time I forgot to send a patch with tarantoolctl changes.

About testing:
You can read help (tarantoolctl rocks help) and try something from new 
commands(build config download init lint new_version purge which 
write_rockspec)
For example: tarantoolctl rocks download xavante
Or test some from breaking flags:
For example: tarantoolctl rocks search --all