<div dir="ltr"><div dir="ltr"><div>Hi, Mergen</div><div><br></div><div>Thanks for your patch, see my comments below</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 9 Jul 2020 at 11:38, Mergen Imeev <<a href="mailto:imeevma@tarantool.org">imeevma@tarantool.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi! Thank you for the review. Diff and the new patch below.<br>
<br>
Below is a ChangeLog, but I will add it to where it should be<br>
when I submit it to the second review.<br>
<br>
@ChangeLog:<br>
* Fix rock name in case rock in installed from *.all.rock.<br>
<br>
On Wed, Jul 08, 2020 at 12:12:45PM +0300, Leonid Vasiliev wrote:<br>
> Hi! Thank you for the patch.<br>
> It looks good, but I have some questions/comments.<br>
> <br>
> <br>
> On 07.07.2020 09:45, <a href="mailto:imeevma@tarantool.org" target="_blank">imeevma@tarantool.org</a> wrote:<br>
> > The first luarock installed creates a repository manifest.<br>
> > However, before this patch, the generated manifest was already<br>
> > filled in according to the files in the repository. This results<br>
> > in an error if luarock was installed from * .all.rock and had<br>
> > dependencies. The problem was that when creating the manifest,<br>
> > information about luarock was written to the manifest when<br>
> > installing its dependencies. After that, since information about<br>
> > luarock already appears after installing the dependencies, but<br>
> > before installing luarock, luarock was not installed in the<br>
> > default directory. It was installed in another directory.<br>
> > <br>
> > After this patch, an empty manifest will be created instead of the<br>
> > already filled manifest. It will be gradually filled during<br>
> > installation of the luarock.<br>
> > <br>
> > Closes tarantool/tarantool#4704<br>
> > ---<br>
> > <a href="https://github.com/tarantool/tarantool/issues/4704" rel="noreferrer" target="_blank">https://github.com/tarantool/tarantool/issues/4704</a><br>
> > <a href="https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name" rel="noreferrer" target="_blank">https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name</a><br>
> > <br>
> > src/luarocks/manif.lua | 23 +++++++++++++++++++----<br>
> > 1 file changed, 19 insertions(+), 4 deletions(-)<br>
> > <br>
> > diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua<br>
> > index 34ae02d..2c54200 100644<br>
> > --- a/src/luarocks/manif.lua<br>
> > +++ b/src/luarocks/manif.lua<br>
> > @@ -423,6 +423,21 @@ function manif.make_manifest(repo, deps_mode, remote)<br>
> > return save_table(repo, "manifest", manifest)<br>
> > end<br>
> > +-- Create an empty manifest file. A file called 'manifest' will be<br>
> <br>
> Function descriptions begin with "---".<br>
> <br>
> > +-- written in the root of the given repository directory.<br>
> > +--<br>
> > +-- @param repo A local repository directory.<br>
> > +-- @return boolean or (nil, string): True if manifest was<br>
> > +-- generated, or nil and an error message.<br>
> > +local function make_empty_manifest(repo)<br>
> > + assert(type(repo) == "string")<br>
> > + if not fs.is_dir(repo) then<br>
> > + return nil, "Cannot access repository at "..repo<br>
> > + end<br>
> > + local manifest = { repository = {}, modules = {}, commands = {} }<br>
> <br>
> Seems like a good practice to add a manifest to the cache.<br>
> manif_core.cache_manifest()<br>
> Will be used in manif_core.load_local_manifest().<br>
> <br>
> > + return save_table(repo, "manifest", manifest)<br>
> > +end<br>
> > +<br>
> > --- Update manifest file for a local repository<br>
> > -- adding information about a version of a package installed in that repository.<br>
> > -- @param name string: Name of a package from the repository.<br>
> > @@ -445,10 +460,10 @@ function manif.add_to_manifest(name, version, repo, deps_mode)<br>
> > local manifest, err = manif_core.load_local_manifest(rocks_dir)<br>
> > if not manifest then<br>
> > util.printerr("No existing manifest. Attempting to rebuild...")<br>
> > - -- Manifest built by `manif.make_manifest` should already<br>
> > - -- include information about given name and version,<br>
> > - -- no need to update it.<br>
> > - return manif.make_manifest(rocks_dir, deps_mode)<br>
> > + -- Create an empty manifest.<br>
> > + local ok, err = make_empty_manifest(rocks_dir)<br>
> > + if not ok then return nil, err end<br>
> > + manifest, err = manif_core.load_local_manifest(rocks_dir)<br>
> > end<br>
> > local results = {[name] = {[version] = {{arch = "installed", repo = rocks_dir}}}}<br>
> > <br>
<br>
<br>
Diff:<br>
<br>
<br>
diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua<br>
index 2c54200..56f52b6 100644<br>
--- a/src/luarocks/manif.lua<br>
+++ b/src/luarocks/manif.lua<br>
@@ -423,7 +423,7 @@ function manif.make_manifest(repo, deps_mode, remote)<br>
return save_table(repo, "manifest", manifest)<br>
end<br>
--- Create an empty manifest file. A file called 'manifest' will be<br>
+--- Create an empty manifest file. A file called 'manifest' will be<br>
-- written in the root of the given repository directory.<br>
--<br>
-- @param repo A local repository directory.<br>
@@ -435,6 +435,7 @@ local function make_empty_manifest(repo)<br>
return nil, "Cannot access repository at "..repo<br>
end<br>
local manifest = { repository = {}, modules = {}, commands = {} }<br>
+ manif_core.cache_manifest(repo, nil, manifest)<br>
return save_table(repo, "manifest", manifest)<br>
end<br>
<br>
<br>
New patch:<br>
<br>
<br>
From e115d3449b50ae2357a9b3f098f2b7b2c4bf1944 Mon Sep 17 00:00:00 2001<br>
From: Mergen Imeev <<a href="mailto:imeevma@gmail.com" target="_blank">imeevma@gmail.com</a>><br>
Date: Mon, 6 Jul 2020 18:56:57 +0300<br>
Subject: [PATCH] luarock: change a way to create manifest<br>
<br>
The first luarock installed creates a repository manifest.<br>
However, before this patch, the generated manifest was already<br>
filled in according to the files in the repository. This results<br>
in an error if luarock was installed from * .all.rock and had<br>
dependencies. The problem was that when creating the manifest,<br>
information about luarock was written to the manifest when<br>
installing its dependencies. After that, since information about<br>
luarock already appears after installing the dependencies, but<br>
before installing luarock, luarock was not installed in the<br>
default directory. It was installed in another directory.<br>
<br>
After this patch, an empty manifest will be created instead of the<br>
already filled manifest. It will be gradually filled during<br>
installation of the luarock.<br>
<br>
Closes tarantool/tarantool#4704<br>
<br>
diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua<br>
index 34ae02d..56f52b6 100644<br>
--- a/src/luarocks/manif.lua<br>
+++ b/src/luarocks/manif.lua<br>
@@ -423,6 +423,22 @@ function manif.make_manifest(repo, deps_mode, remote)<br>
return save_table(repo, "manifest", manifest)<br>
end<br>
+--- Create an empty manifest file. A file called 'manifest' will be<br>
+-- written in the root of the given repository directory.<br>
+--<br>
+-- @param repo A local repository directory.<br>
+-- @return boolean or (nil, string): True if manifest was<br>
+-- generated, or nil and an error message.<br>
+local function make_empty_manifest(repo)<br>
+ assert(type(repo) == "string")<br>
+ if not fs.is_dir(repo) then<br>
+ return nil, "Cannot access repository at "..repo<br>
+ end<br>
+ local manifest = { repository = {}, modules = {}, commands = {} }<br>
+ manif_core.cache_manifest(repo, nil, manifest)<br>
+ return save_table(repo, "manifest", manifest)<br>
+end<br></blockquote><div><br></div><div>Why is this entire function needed? `<span style="font-family:monospace">save_table</span>` is already called from the `<span style="font-family:monospace">add_to_manifest</span>` function.</div><div>Checking <span style="font-family:monospace">fs.is_dir</span> seems to be irrelevant - it doesn't affect this particular function.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
--- Update manifest file for a local repository<br>
-- adding information about a version of a package installed in that <br>
repository.<br>
-- @param name string: Name of a package from the repository.<br>
@@ -445,10 +461,10 @@ function manif.add_to_manifest(name, version, <br>
repo, deps_mode)<br>
local manifest, err = manif_core.load_local_manifest(rocks_dir)<br>
if not manifest then<br>
util.printerr("No existing manifest. Attempting to rebuild...")<br></blockquote><div><br></div><div>This message seems obsolete. Consider rephrasing it: <span style="font-family:monospace">"No existing manifest. Creating an empty one..."</span> or so.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- -- Manifest built by `manif.make_manifest` should already<br>
- -- include information about given name and version,<br>
- -- no need to update it.<br>
- return manif.make_manifest(rocks_dir, deps_mode)<br>
+ -- Create an empty manifest.<br>
+ local ok, err = make_empty_manifest(rocks_dir)<br>
+ if not ok then return nil, err end<br>
+ manifest, err = manif_core.load_local_manifest(rocks_dir)<br></blockquote><div> </div></div><div class="gmail_quote">Consider inlining `<span style="font-family:monospace">make_empty_manifest</span>` workload here. Seems like there are only two lines needed:<br></div><div class="gmail_quote">```<span style="font-family:monospace"><br></span></div><div class="gmail_quote"><span style="font-family:monospace">manifest, err = {...}, nil</span></div><div class="gmail_quote"><span style="font-family:monospace">manif_core.cache_manifest(rep</span><span style="font-family:monospace">o, nil, manifest)</span></div><div class="gmail_quote"><div>```</div><div><br></div><div>Also, `<span style="font-family:monospace">err</span>` is assigned to the local variable declared two lines above which shadows the declaration outside the `<span style="font-family:monospace">if</span>` statement. <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
end<br>
local results = {[name] = {[version] = {{arch = "installed", repo <br>
= rocks_dir}}}}<br></blockquote><div><br></div><div><div><br></div><div><div dir="ltr" class="gmail_signature"><div dir="ltr"><span><div><div dir="ltr">Best regards</div><div>Yaroslav Dynnikov</div></div></span></div></div></div> </div></div></div>