From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 4A2E7445320 for ; Thu, 9 Jul 2020 11:38:03 +0300 (MSK) From: Mergen Imeev References: Message-ID: Date: Thu, 9 Jul 2020 11:38:01 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Leonid Vasiliev Cc: yaroslav.dynnikov@tarantool.org, tarantool-patches@dev.tarantool.org 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 + --- 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}}}}