[Tarantool-patches] [PATCH v1 1/1] luarock: change a way to create manifest
Yaroslav Dynnikov
yaroslav.dynnikov at tarantool.org
Thu Jul 9 18:16:02 MSK 2020
Hi, Mergen
Thanks for your patch, see my comments below
On Thu, 9 Jul 2020 at 11:38, Mergen Imeev <imeevma at 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 at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200709/9dde4aae/attachment.html>
More information about the Tarantool-patches
mailing list