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