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

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

[imeevma]

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
+-- 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 = {} }
+   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}}}}
-- 
2.25.1

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

[Leonid Vasiliev]

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

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

[Mergen Imeev]

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@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@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
+
  --- 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...")
-      -- 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}}}}

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

[Yaroslav Dynnikov]

[-- Attachment #1: Type: text/plain, Size: 8630 bytes --]

Hi, Mergen

Thanks for your patch, see my comments below

On Thu, 9 Jul 2020 at 11:38, Mergen Imeev <imeevma@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@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@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

[-- Attachment #2: Type: text/html, Size: 11994 bytes --]