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

Yaroslav Dynnikov yaroslav.dynnikov at tarantool.org
Thu Jul 9 18:16:02 MSK 2020


Hi, Mergen

Thanks for your patch, see my comments below

On Thu, 9 Jul 2020 at 11:38, Mergen Imeev <imeevma at tarantool.org> wrote:

> Hi! Thank you for the review. Diff and the new patch below.
>
> Below is a ChangeLog, but I will add it to where it should be
> when I submit it to the second review.
>
> @ChangeLog:
>   * Fix rock name in case rock in installed from *.all.rock.
>
> On Wed, Jul 08, 2020 at 12:12:45PM +0300, Leonid Vasiliev wrote:
> > Hi! Thank you for the patch.
> > It looks good, but I have some questions/comments.
> >
> >
> > On 07.07.2020 09:45, imeevma at 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}}}}
> > >
>
>
> Diff:
>
>
> diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
> index 2c54200..56f52b6 100644
> --- a/src/luarocks/manif.lua
> +++ b/src/luarocks/manif.lua
> @@ -423,7 +423,7 @@ 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
> +--- Create an empty manifest file. A file called 'manifest' will be
>   -- written in the root of the given repository directory.
>   --
>   -- @param repo A local repository directory.
> @@ -435,6 +435,7 @@ local function make_empty_manifest(repo)
>         return nil, "Cannot access repository at "..repo
>      end
>      local manifest = { repository = {}, modules = {}, commands = {} }
> +   manif_core.cache_manifest(repo, nil, manifest)
>      return save_table(repo, "manifest", manifest)
>   end
>
>
> New patch:
>
>
>  From e115d3449b50ae2357a9b3f098f2b7b2c4bf1944 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma at gmail.com>
> Date: Mon, 6 Jul 2020 18:56:57 +0300
> Subject: [PATCH] luarock: change a way to create manifest
>
> 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
>
> diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
> index 34ae02d..56f52b6 100644
> --- a/src/luarocks/manif.lua
> +++ b/src/luarocks/manif.lua
> @@ -423,6 +423,22 @@ 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
> +-- 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 = {} }
> +   manif_core.cache_manifest(repo, nil, manifest)
> +   return save_table(repo, "manifest", manifest)
> +end
>

Why is this entire function needed? `save_table` is already called from the
`add_to_manifest` function.
Checking fs.is_dir seems to be irrelevant - it doesn't affect this
particular function.

+
>   --- 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 +461,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...")
>

This message seems obsolete. Consider rephrasing it: "No existing manifest.
Creating an empty one..." or so.


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

Consider inlining `make_empty_manifest` workload here. Seems like there are
only two lines needed:
```
manifest, err = {...}, nil
manif_core.cache_manifest(repo, nil, manifest)
```

Also, `err` is assigned to the local variable declared two lines above
which shadows the declaration outside the `if` statement.

     end
>       local results = {[name] = {[version] = {{arch = "installed", repo
> = rocks_dir}}}}
>


Best regards
Yaroslav Dynnikov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200709/9dde4aae/attachment.html>


More information about the Tarantool-patches mailing list