Re: [Tarantool-patches] [PATCH v1 1/1] luarock: change a way to create manifest

[Leonid Vasiliev <lvasiliev@tarantool.org>]

Hi! Thank you for the patch.
It looks good, but I have some questions/comments.


On 07.07.2020 09:45, imeevma@tarantool.org wrote:
> The first luarock installed creates a repository manifest.
> However, before this patch, the generated manifest was already
> filled in according to the files in the repository. This results
> in an error if luarock was installed from * .all.rock and had
> dependencies. The problem was that when creating the manifest,
> information about luarock was written to the manifest when
> installing its dependencies. After that, since information about
> luarock already appears after installing the dependencies, but
> before installing luarock, luarock was not installed in the
> default directory. It was installed in another directory.
> 
> After this patch, an empty manifest will be created instead of the
> already filled manifest. It will be gradually filled during
> installation of the luarock.
> 
> Closes tarantool/tarantool#4704
> ---
> https://github.com/tarantool/tarantool/issues/4704
> https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name
> 
>   src/luarocks/manif.lua | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
> index 34ae02d..2c54200 100644
> --- a/src/luarocks/manif.lua
> +++ b/src/luarocks/manif.lua
> @@ -423,6 +423,21 @@ function manif.make_manifest(repo, deps_mode, remote)
>      return save_table(repo, "manifest", manifest)
>   end
>   
> +-- Create an empty manifest file. A file called 'manifest' will be

Function descriptions begin with "---".

> +-- written in the root of the given repository directory.
> +--
> +-- @param repo A local repository directory.
> +-- @return boolean or (nil, string): True if manifest was
> +-- generated, or nil and an error message.
> +local function make_empty_manifest(repo)
> +   assert(type(repo) == "string")
> +   if not fs.is_dir(repo) then
> +      return nil, "Cannot access repository at "..repo
> +   end
> +   local manifest = { repository = {}, modules = {}, commands = {} }

Seems like a good practice to add a manifest to the cache.
manif_core.cache_manifest()
Will be used in manif_core.load_local_manifest().

> +   return save_table(repo, "manifest", manifest)
> +end
> +
>   --- Update manifest file for a local repository
>   -- adding information about a version of a package installed in that repository.
>   -- @param name string: Name of a package from the repository.
> @@ -445,10 +460,10 @@ function manif.add_to_manifest(name, version, repo, deps_mode)
>      local manifest, err = manif_core.load_local_manifest(rocks_dir)
>      if not manifest then
>         util.printerr("No existing manifest. Attempting to rebuild...")
> -      -- Manifest built by `manif.make_manifest` should already
> -      -- include information about given name and version,
> -      -- no need to update it.
> -      return manif.make_manifest(rocks_dir, deps_mode)
> +      -- Create an empty manifest.
> +      local ok, err = make_empty_manifest(rocks_dir)
> +      if not ok then return nil, err end
> +      manifest, err = manif_core.load_local_manifest(rocks_dir)
>      end
>   
>      local results = {[name] = {[version] = {{arch = "installed", repo = rocks_dir}}}}
>