From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 24391445320 for ; Thu, 9 Jul 2020 18:16:15 +0300 (MSK) Received: by smtp10.mail.ru with esmtpa (envelope-from ) id 1jtYHW-0003dQ-9x for tarantool-patches@dev.tarantool.org; Thu, 09 Jul 2020 18:16:14 +0300 Received: by mail-lf1-f49.google.com with SMTP id d21so1365676lfb.6 for ; Thu, 09 Jul 2020 08:16:14 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yaroslav Dynnikov Date: Thu, 9 Jul 2020 18:16:02 +0300 Message-ID: Content-Type: multipart/alternative; boundary="00000000000087101905aa03b449" Subject: Re: [Tarantool-patches] [PATCH v1 1/1] luarock: change a way to create manifest List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: Yaroslav Dynnikov , tml --00000000000087101905aa03b449 Content-Type: text/plain; charset="UTF-8" Hi, Mergen Thanks for your patch, see my comments below On Thu, 9 Jul 2020 at 11:38, Mergen Imeev 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 > 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 --00000000000087101905aa03b449 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi, Mergen

= Thanks for your patch, see my comments below

On Thu, 9 Jul 2020 at 11:= 38, Mergen Imeev <imeevma@taran= tool.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:
=C2=A0 * 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<= br> > > 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<= br> > > 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 th= e
> > already filled manifest. It will be gradually filled during
> > installation of the luarock.
> >
> > Closes tarantool/tarantool#4704
> > ---
> > https://github.com/tarantool/tarantool/i= ssues/4704
> > https://github.com= /tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name
> >
> >=C2=A0 =C2=A0src/luarocks/manif.lua | 23 +++++++++++++++++++---- > >=C2=A0 =C2=A01 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)
> >=C2=A0 =C2=A0 =C2=A0 return save_table(repo, "manifest",= manifest)
> >=C2=A0 =C2=A0end
> > +-- Create an empty manifest file. A file called 'manifest= 9; 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)
> > +=C2=A0 =C2=A0assert(type(repo) =3D=3D "string")
> > +=C2=A0 =C2=A0if not fs.is_dir(repo) then
> > +=C2=A0 =C2=A0 =C2=A0 return nil, "Cannot access repository = at "..repo
> > +=C2=A0 =C2=A0end
> > +=C2=A0 =C2=A0local manifest =3D { repository =3D {}, modules =3D= {}, commands =3D {} }
>
> 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().
>
> > +=C2=A0 =C2=A0return save_table(repo, "manifest", manif= est)
> > +end
> > +
> >=C2=A0 =C2=A0--- Update manifest file for a local repository
> >=C2=A0 =C2=A0-- adding information about a version of a package in= stalled in that repository.
> >=C2=A0 =C2=A0-- @param name string: Name of a package from the rep= ository.
> > @@ -445,10 +460,10 @@ function manif.add_to_manifest(name, versio= n, repo, deps_mode)
> >=C2=A0 =C2=A0 =C2=A0 local manifest, err =3D manif_core.load_local= _manifest(rocks_dir)
> >=C2=A0 =C2=A0 =C2=A0 if not manifest then
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0util.printerr("No existing = manifest. Attempting to rebuild...")
> > -=C2=A0 =C2=A0 =C2=A0 -- Manifest built by `manif.make_manifest` = should already
> > -=C2=A0 =C2=A0 =C2=A0 -- include information about given name and= version,
> > -=C2=A0 =C2=A0 =C2=A0 -- no need to update it.
> > -=C2=A0 =C2=A0 =C2=A0 return manif.make_manifest(rocks_dir, deps_= mode)
> > +=C2=A0 =C2=A0 =C2=A0 -- Create an empty manifest.
> > +=C2=A0 =C2=A0 =C2=A0 local ok, err =3D make_empty_manifest(rocks= _dir)
> > +=C2=A0 =C2=A0 =C2=A0 if not ok then return nil, err end
> > +=C2=A0 =C2=A0 =C2=A0 manifest, err =3D manif_core.load_local_man= ifest(rocks_dir)
> >=C2=A0 =C2=A0 =C2=A0 end
> >=C2=A0 =C2=A0 =C2=A0 local results =3D {[name] =3D {[version] =3D = {{arch =3D "installed", repo =3D 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) =C2=A0 =C2=A0 =C2=A0return save_table(repo, "manifest", manifest)=
=C2=A0 end
=C2=A0 --- Create an empty manifest file. A file called 'manifest' = will be
+--- Create an empty manifest file. A file called 'manifest' will b= e
=C2=A0 -- written in the root of the given repository directory.
=C2=A0 --
=C2=A0 -- @param repo A local repository directory.
@@ -435,6 +435,7 @@ local function make_empty_manifest(repo)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return nil, "Cannot access repository at &= quot;..repo
=C2=A0 =C2=A0 =C2=A0end
=C2=A0 =C2=A0 =C2=A0local manifest =3D { repository =3D {}, modules =3D {},= commands =3D {} }
+=C2=A0 =C2=A0manif_core.cache_manifest(repo, nil, manifest)
=C2=A0 =C2=A0 =C2=A0return save_table(repo, "manifest", manifest)=
=C2=A0 end


New patch:


=C2=A0From e115d3449b50ae2357a9b3f098f2b7b2c4bf1944 Mon Sep 17 00:00:00 200= 1
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)<= br> =C2=A0 =C2=A0 =C2=A0return save_table(repo, "manifest", manifest)=
=C2=A0 end
=C2=A0 +--- 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)
+=C2=A0 =C2=A0assert(type(repo) =3D=3D "string")
+=C2=A0 =C2=A0if not fs.is_dir(repo) then
+=C2=A0 =C2=A0 =C2=A0 return nil, "Cannot access repository at ".= .repo
+=C2=A0 =C2=A0end
+=C2=A0 =C2=A0local manifest =3D { repository =3D {}, modules =3D {}, comma= nds =3D {} }
+=C2=A0 =C2=A0manif_core.cache_manifest(repo, nil, manifest)
+=C2=A0 =C2=A0return save_table(repo, "manifest", manifest)
+end

Why is this entire function needed= ? `save_table` is already call= ed from the `add_to_manifest` = function.
Checking fs.is_di= r seems to be irrelevant - it doesn't affect this particular fun= ction.

"No existing manifest. Creating an empty one..." or so= .
=C2=A0
-=C2=A0 =C2=A0 =C2=A0 -- Manifest built by `manif.make_manifest` should alr= eady
-=C2=A0 =C2=A0 =C2=A0 -- include information about given name and version,<= br> -=C2=A0 =C2=A0 =C2=A0 -- no need to update it.
-=C2=A0 =C2=A0 =C2=A0 return manif.make_manifest(rocks_dir, deps_mode)
+=C2=A0 =C2=A0 =C2=A0 -- Create an empty manifest.
+=C2=A0 =C2=A0 =C2=A0 local ok, err =3D make_empty_manifest(rocks_dir)
+=C2=A0 =C2=A0 =C2=A0 if not ok then return nil, err end
+=C2=A0 =C2=A0 =C2=A0 manifest, err =3D manif_core.load_local_manifest(rock= s_dir)
=C2=A0
Co= nsider inlining `make_empty_manifest<= /span>` workload here. Seems like there are only two lines needed:
```
manifest, err =3D {...}, 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.

=C2=A0 =C2=A0 =C2=A0end
=C2=A0 =C2=A0 =C2=A0 local results =3D {[name] =3D {[version] =3D {{arch = =3D "installed", repo
=3D rocks_dir}}}}


<= div dir=3D"ltr" class=3D"gmail_signature">
Best regards
Yaroslav Dynnikov
=C2=A0
--00000000000087101905aa03b449--