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

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

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
>