From: Yaroslav Dynnikov <yaroslav.dynnikov@tarantool.org> To: Mergen Imeev <imeevma@tarantool.org> Cc: Yaroslav Dynnikov <yaroslav.dynnikov@tarantool.org>, tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] luarock: change a way to create manifest Date: Thu, 9 Jul 2020 18:16:02 +0300 [thread overview] Message-ID: <CAK0MaD1PHAQrz3VYvEVMWuFsV_eJ2fQ=+oJyc03dQvzi+4g-eg@mail.gmail.com> (raw) In-Reply-To: <f65aba5a-34fa-eb2f-86e1-f7536cbfce32@tarantool.org> [-- Attachment #1: Type: text/plain, Size: 8630 bytes --] 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 [-- Attachment #2: Type: text/html, Size: 11994 bytes --]
prev parent reply other threads:[~2020-07-09 15:16 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-07 6:45 imeevma 2020-07-08 9:12 ` Leonid Vasiliev 2020-07-09 8:38 ` Mergen Imeev 2020-07-09 15:16 ` Yaroslav Dynnikov [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAK0MaD1PHAQrz3VYvEVMWuFsV_eJ2fQ=+oJyc03dQvzi+4g-eg@mail.gmail.com' \ --to=yaroslav.dynnikov@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 1/1] luarock: change a way to create manifest' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox