[Tarantool-patches] [PATCH v2 1/1] luarock: change a way to create manifest
imeevma at tarantool.org
imeevma at tarantool.org
Thu Jul 9 18:51:09 MSK 2020
From: Mergen Imeev <imeevma at gmail.com>
Hi, Yaroslav
Thank you for the review. New version of the patch below.
On 09.07.2020 18:16, Yaroslav Dynnikov wrote:
> 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
>
>From 8f77bafc8f23d5c860917ce155c4b18e86d5be73 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma at gmail.com>
Date: Thu, 9 Jul 2020 18:40:12 +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
---
https://github.com/tarantool/tarantool/issues/4704
https://github.com/tarantool/luarocks/tree/imeevma/gh-4704-fix-rock-name
@ChangeLog:
* Fix rock name in case rock in installed from *.all.rock.
src/luarocks/manif.lua | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
index 34ae02d..79a4278 100644
--- a/src/luarocks/manif.lua
+++ b/src/luarocks/manif.lua
@@ -444,11 +444,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)
+ util.printerr("No existing manifest. Creating an empty one...")
+ -- Create an empty manifest.
+ manifest, err = { repository = {}, modules = {}, commands = {} }, nil
+ manif_core.cache_manifest(rocks_dir, nil, manifest)
end
local results = {[name] = {[version] = {{arch = "installed", repo = rocks_dir}}}}
--
2.25.1
More information about the Tarantool-patches
mailing list