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

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

[imeevma]

Whenever a rock is installed it's being added to the repository
manifest. And if a manifest doesn't exist yet, luarocks creates
one by scanning the directory.

It causes a problem when *.all.rock with dependencies is installed
into an empty directory. Luarocks unpacks the all.rock before
installing dependencies, and it's modules are captured during
manifest creation. After the installation finishes, luarocks adds
the all.rock to the manifest once again (now intentionally),
detects collision of module names and renames them uniquely, like
"cartridge_1_2_0_1". As a result, "require()" function doesn't
work.

This patch changes the way of manifest creation: instead of
scanning repo luarocks creates an empty one.

Closes tarantool/tarantool#4704
---
https://github.com/tarantool/tarantool/issues/4704
https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name

@ChangeLog:
 * Don't ruin rock name when freshly installing *.all.rock with
   dependencies.

 src/luarocks/manif.lua | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
index 34ae02d..79a4278 100644
--- a/src/luarocks/manif.lua
+++ b/src/luarocks/manif.lua
@@ -444,11 +444,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)
+      util.printerr("No existing manifest. Creating an empty one...")
+      -- Create an empty manifest.
+      manifest, err = { repository = {}, modules = {}, commands = {} }, nil
+      manif_core.cache_manifest(rocks_dir, nil, manifest)
    end
 
    local results = {[name] = {[version] = {{arch = "installed", repo = rocks_dir}}}}
-- 
2.25.1

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

[Yaroslav Dynnikov]

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

Perfect, LGTM.

Best regards
Yaroslav Dynnikov


On Thu, 9 Jul 2020 at 19:45, <imeevma@tarantool.org> wrote:

> Whenever a rock is installed it's being added to the repository
> manifest. And if a manifest doesn't exist yet, luarocks creates
> one by scanning the directory.
>
> It causes a problem when *.all.rock with dependencies is installed
> into an empty directory. Luarocks unpacks the all.rock before
> installing dependencies, and it's modules are captured during
> manifest creation. After the installation finishes, luarocks adds
> the all.rock to the manifest once again (now intentionally),
> detects collision of module names and renames them uniquely, like
> "cartridge_1_2_0_1". As a result, "require()" function doesn't
> work.
>
> This patch changes the way of manifest creation: instead of
> scanning repo luarocks creates an empty one.
>
> Closes tarantool/tarantool#4704
> ---
> https://github.com/tarantool/tarantool/issues/4704
> https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name
>
> @ChangeLog:
>  * Don't ruin rock name when freshly installing *.all.rock with
>    dependencies.
>
>  src/luarocks/manif.lua | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
> index 34ae02d..79a4278 100644
> --- a/src/luarocks/manif.lua
> +++ b/src/luarocks/manif.lua
> @@ -444,11 +444,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)
> +      util.printerr("No existing manifest. Creating an empty one...")
> +      -- Create an empty manifest.
> +      manifest, err = { repository = {}, modules = {}, commands = {} },
> nil
> +      manif_core.cache_manifest(rocks_dir, nil, manifest)
>     end
>
>     local results = {[name] = {[version] = {{arch = "installed", repo =
> rocks_dir}}}}
> --
> 2.25.1
>
>

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

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

[Leonid Vasiliev]

Hi! Thank you for the patch.
LGTM.

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

[Kirill Yukhin]

Hello,

On 09 июл 19:45, imeevma@tarantool.org wrote:
> Whenever a rock is installed it's being added to the repository
> manifest. And if a manifest doesn't exist yet, luarocks creates
> one by scanning the directory.
> 
> It causes a problem when *.all.rock with dependencies is installed
> into an empty directory. Luarocks unpacks the all.rock before
> installing dependencies, and it's modules are captured during
> manifest creation. After the installation finishes, luarocks adds
> the all.rock to the manifest once again (now intentionally),
> detects collision of module names and renames them uniquely, like
> "cartridge_1_2_0_1". As a result, "require()" function doesn't
> work.
> 
> This patch changes the way of manifest creation: instead of
> scanning repo luarocks creates an empty one.
> 
> Closes tarantool/tarantool#4704

I've checked your patch into tarantool/luarocks's tarantool-1.7
branch and bumped a new version in 1.10 branch.

--
Regards, Kirill Yukhin

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

[imeevma]

From: Mergen Imeev <imeevma@gmail.com>

Hi, Yaroslav

Thank you for the review. New version of the patch below.


On 09.07.2020 18:16, Yaroslav Dynnikov wrote:
> 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
>  




From 8f77bafc8f23d5c860917ce155c4b18e86d5be73 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 9 Jul 2020 18:40:12 +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
---
https://github.com/tarantool/tarantool/issues/4704
https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name

@ChangeLog:
 * Fix rock name in case rock in installed from *.all.rock.

 src/luarocks/manif.lua | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
index 34ae02d..79a4278 100644
--- a/src/luarocks/manif.lua
+++ b/src/luarocks/manif.lua
@@ -444,11 +444,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)
+      util.printerr("No existing manifest. Creating an empty one...")
+      -- Create an empty manifest.
+      manifest, err = { repository = {}, modules = {}, commands = {} }, nil
+      manif_core.cache_manifest(rocks_dir, nil, manifest)
    end
 
    local results = {[name] = {[version] = {{arch = "installed", repo = rocks_dir}}}}
-- 
2.25.1