[Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library

[Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library

[Sergey Kaplun]

This patchset consists from 2 parts:
1) Provide corresponding new LuaJIT header within tarantool at install.
2) Add missing public C API symbols from the new lib into exports.h

Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5187-luajit-include-headers

CI is red, but fails related to replication tests or at osx_* jobs
(see also https://github.com/tarantool/tarantool/issues/5423).
CI: https://gitlab.com/tarantool/tarantool/-/pipelines/203892776

Sergey Kaplun (2):
  build: provide missing LuaJIT lmisclib.h header
  lua: add missing LuaJIT export symbols

 FreeBSD/databases/tarantool/pkg-plist | 1 +
 cmake/luajit.cmake                    | 2 +-
 rpm/tarantool.spec                    | 1 +
 src/exports.h                         | 2 ++
 4 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.28.0

[Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header

[Sergey Kaplun]

Since LuaJIT provides extended LuaC API introduced in the scope of
5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API
for platform metrics') corresponding header should be installed
together with tarantool.

Follows up tarantool/tarantool#5187
---

I checked all references of LuaJIT provided headers as follows:

===================================================================
grep "lualib.h" -R ~/builds_workspace/tarantool/master/  | grep -v  -e tags -e CMakeFiles -e "#include" -e Makefile.dep -e third_party
/home/burii/builds_workspace/tarantool/master/rpm/tarantool.spec:%{_includedir}/tarantool/lualib.h
Binary file /home/burii/builds_workspace/tarantool/master/cmake/.luajit.cmake.swp matches
/home/burii/builds_workspace/tarantool/master/cmake/luajit.cmake: install (FILES ${inc}/lua.h ${inc}/lualib.h ${inc}/lauxlib.h
/home/burii/builds_workspace/tarantool/master/FreeBSD/databases/tarantool/pkg-plist:include/tarantool/lualib.h
===================================================================

Did I miss something?


 FreeBSD/databases/tarantool/pkg-plist | 1 +
 cmake/luajit.cmake                    | 2 +-
 rpm/tarantool.spec                    | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/FreeBSD/databases/tarantool/pkg-plist b/FreeBSD/databases/tarantool/pkg-plist
index 9e3905d60..6df72be9d 100644
--- a/FreeBSD/databases/tarantool/pkg-plist
+++ b/FreeBSD/databases/tarantool/pkg-plist
@@ -8,6 +8,7 @@ include/tarantool/lua.hpp
 include/tarantool/luaconf.h
 include/tarantool/luajit.h
 include/tarantool/lualib.h
+include/tarantool/lmisclib.h
 include/tarantool/module.h
 man/man1/tarantool.1.gz
 man/man1/tarantoolctl.1.gz
diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 555bc8371..1c7784e11 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -287,7 +287,7 @@ macro(luajit_build)
     unset (luajit_buildoptions)
     set (inc ${PROJECT_SOURCE_DIR}/third_party/luajit/src)
     install (FILES ${inc}/lua.h ${inc}/lualib.h ${inc}/lauxlib.h
-        ${inc}/luaconf.h ${inc}/lua.hpp ${inc}/luajit.h
+        ${inc}/luaconf.h ${inc}/lua.hpp ${inc}/luajit.h ${inc}/lmisclib.h
         DESTINATION ${MODULE_INCLUDEDIR})
 endmacro()
 
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index eedc0312c..3a1a8f2be 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -256,6 +256,7 @@ fi
 %{_includedir}/tarantool/luajit.h
 %{_includedir}/tarantool/lualib.h
 %{_includedir}/tarantool/module.h
+%{_includedir}/tarantool/lmisclib.h
 
 %changelog
 * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header

[Igor Munkin]

Sergey,

Thanks for the patch! LGTM, except the nits below.

On 17.10.20, Sergey Kaplun wrote:
> Since LuaJIT provides extended LuaC API introduced in the scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API
> for platform metrics') corresponding header should be installed
> together with tarantool.

Typo: s/tarantool/Tarantool/.
Minor: Strictly saying it should not. The header is provided by another
package (so called "devel" package), so I propose the following wording:
| corresponding header should be provided along with other Tarantool
| development files.

> 
> Follows up tarantool/tarantool#5187

Minor: You don't need to use full repo reference here.

> ---
> 
> I checked all references of LuaJIT provided headers as follows:
> 
> ===================================================================
> grep "lualib.h" -R ~/builds_workspace/tarantool/master/  | grep -v  -e tags -e CMakeFiles -e "#include" -e Makefile.dep -e third_party
> /home/burii/builds_workspace/tarantool/master/rpm/tarantool.spec:%{_includedir}/tarantool/lualib.h
> Binary file /home/burii/builds_workspace/tarantool/master/cmake/.luajit.cmake.swp matches
> /home/burii/builds_workspace/tarantool/master/cmake/luajit.cmake: install (FILES ${inc}/lua.h ${inc}/lualib.h ${inc}/lauxlib.h
> /home/burii/builds_workspace/tarantool/master/FreeBSD/databases/tarantool/pkg-plist:include/tarantool/lualib.h
> ===================================================================
> 
> Did I miss something?

I guess no: Gentoo ebuilds are maintained outside the Tarantool repo by
Sasha Tu. However, I have no idea, how packages for Debian-based distros
are built. Anyway, it looks everything inside this repo is fixed.

> 
> 
>  FreeBSD/databases/tarantool/pkg-plist | 1 +
>  cmake/luajit.cmake                    | 2 +-
>  rpm/tarantool.spec                    | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/FreeBSD/databases/tarantool/pkg-plist b/FreeBSD/databases/tarantool/pkg-plist
> index 9e3905d60..6df72be9d 100644
> --- a/FreeBSD/databases/tarantool/pkg-plist
> +++ b/FreeBSD/databases/tarantool/pkg-plist
> @@ -8,6 +8,7 @@ include/tarantool/lua.hpp
>  include/tarantool/luaconf.h
>  include/tarantool/luajit.h
>  include/tarantool/lualib.h
> +include/tarantool/lmisclib.h

Minor: Please sort the entries.

>  include/tarantool/module.h
>  man/man1/tarantool.1.gz
>  man/man1/tarantoolctl.1.gz

<snipped>

> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index eedc0312c..3a1a8f2be 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec
> @@ -256,6 +256,7 @@ fi
>  %{_includedir}/tarantool/luajit.h
>  %{_includedir}/tarantool/lualib.h
>  %{_includedir}/tarantool/module.h
> +%{_includedir}/tarantool/lmisclib.h
>  
>  %changelog
>  * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1

Side note: It looks we totally ignore updating %changelog section.

> -- 
> 2.28.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header

[Sergey Kaplun]

Hi, Igor!
Thanks for the review!

On 18.10.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! LGTM, except the nits below.
> 
> On 17.10.20, Sergey Kaplun wrote:
> > Since LuaJIT provides extended LuaC API introduced in the scope of
> > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API
> > for platform metrics') corresponding header should be installed
> > together with tarantool.
> 
> Typo: s/tarantool/Tarantool/.
> Minor: Strictly saying it should not. The header is provided by another
> package (so called "devel" package), so I propose the following wording:
> | corresponding header should be provided along with other Tarantool
> | development files.
> 
> > 
> > Follows up tarantool/tarantool#5187
> 
> Minor: You don't need to use full repo reference here.

Thanks! I've updated commit message considering your comments here.

> 
> > ---
> > 
> > I checked all references of LuaJIT provided headers as follows:
> > 
> > ===================================================================
> > grep "lualib.h" -R ~/builds_workspace/tarantool/master/  | grep -v  -e tags -e CMakeFiles -e "#include" -e Makefile.dep -e third_party
> > /home/burii/builds_workspace/tarantool/master/rpm/tarantool.spec:%{_includedir}/tarantool/lualib.h
> > Binary file /home/burii/builds_workspace/tarantool/master/cmake/.luajit.cmake.swp matches
> > /home/burii/builds_workspace/tarantool/master/cmake/luajit.cmake: install (FILES ${inc}/lua.h ${inc}/lualib.h ${inc}/lauxlib.h
> > /home/burii/builds_workspace/tarantool/master/FreeBSD/databases/tarantool/pkg-plist:include/tarantool/lualib.h
> > ===================================================================
> > 
> > Did I miss something?
> 
> I guess no: Gentoo ebuilds are maintained outside the Tarantool repo by
> Sasha Tu. However, I have no idea, how packages for Debian-based distros
> are built. Anyway, it looks everything inside this repo is fixed.
> 
> > 
> > 
> >  FreeBSD/databases/tarantool/pkg-plist | 1 +
> >  cmake/luajit.cmake                    | 2 +-
> >  rpm/tarantool.spec                    | 1 +
> >  3 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/FreeBSD/databases/tarantool/pkg-plist b/FreeBSD/databases/tarantool/pkg-plist
> > index 9e3905d60..6df72be9d 100644
> > --- a/FreeBSD/databases/tarantool/pkg-plist
> > +++ b/FreeBSD/databases/tarantool/pkg-plist
> > @@ -8,6 +8,7 @@ include/tarantool/lua.hpp
> >  include/tarantool/luaconf.h
> >  include/tarantool/luajit.h
> >  include/tarantool/lualib.h
> > +include/tarantool/lmisclib.h
> 
> Minor: Please sort the entries.

I've sorted entries here and inside rpm/tarantool.spec. See itterative
patch below. Branch is force pushed.

> 
> >  include/tarantool/module.h
> >  man/man1/tarantool.1.gz
> >  man/man1/tarantoolctl.1.gz
> 
> <snipped>
> 
> > diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> > index eedc0312c..3a1a8f2be 100644
> > --- a/rpm/tarantool.spec
> > +++ b/rpm/tarantool.spec
> > @@ -256,6 +256,7 @@ fi
> >  %{_includedir}/tarantool/luajit.h
> >  %{_includedir}/tarantool/lualib.h
> >  %{_includedir}/tarantool/module.h
> > +%{_includedir}/tarantool/lmisclib.h
> >  
> >  %changelog
> >  * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
> 
> Side note: It looks we totally ignore updating %changelog section.
> 
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

===================================================================
diff --git a/FreeBSD/databases/tarantool/pkg-plist b/FreeBSD/databases/tarantool/pkg-plist
index 6df72be9d..8e2f3afde 100644
--- a/FreeBSD/databases/tarantool/pkg-plist
+++ b/FreeBSD/databases/tarantool/pkg-plist
@@ -3,12 +3,12 @@ bin/tarantoolctl
 %%ETCDIR%%/default/tarantool
 %%ETCDIR%%/instances.available/example.lua
 include/tarantool/lauxlib.h
+include/tarantool/lmisclib.h
 include/tarantool/lua.h
 include/tarantool/lua.hpp
 include/tarantool/luaconf.h
 include/tarantool/luajit.h
 include/tarantool/lualib.h
-include/tarantool/lmisclib.h
 include/tarantool/module.h
 man/man1/tarantool.1.gz
 man/man1/tarantoolctl.1.gz
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 3a1a8f2be..9ac7a1a04 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -250,13 +250,13 @@ fi
 %files devel
 %dir %{_includedir}/tarantool
 %{_includedir}/tarantool/lauxlib.h
+%{_includedir}/tarantool/lmisclib.h
 %{_includedir}/tarantool/luaconf.h
 %{_includedir}/tarantool/lua.h
 %{_includedir}/tarantool/lua.hpp
 %{_includedir}/tarantool/luajit.h
 %{_includedir}/tarantool/lualib.h
 %{_includedir}/tarantool/module.h
-%{_includedir}/tarantool/lmisclib.h
 
 %changelog
 * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header

[Sergey Ostanevich]

Hi!

Thanks, this one LGTM on the latest branch.

Sergos.

On 19 окт 07:55, Sergey Kaplun wrote:
> Hi, Igor!
> Thanks for the review!
> 
> On 18.10.20, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! LGTM, except the nits below.
> > 
> > On 17.10.20, Sergey Kaplun wrote:
> > > Since LuaJIT provides extended LuaC API introduced in the scope of
> > > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API
> > > for platform metrics') corresponding header should be installed
> > > together with tarantool.
> > 
> > Typo: s/tarantool/Tarantool/.
> > Minor: Strictly saying it should not. The header is provided by another
> > package (so called "devel" package), so I propose the following wording:
> > | corresponding header should be provided along with other Tarantool
> > | development files.
> > 
> > > 
> > > Follows up tarantool/tarantool#5187
> > 
> > Minor: You don't need to use full repo reference here.
> 
> Thanks! I've updated commit message considering your comments here.
> 
> > 
> > > ---
> > > 
> > > I checked all references of LuaJIT provided headers as follows:
> > > 
> > > ===================================================================
> > > grep "lualib.h" -R ~/builds_workspace/tarantool/master/  | grep -v  -e tags -e CMakeFiles -e "#include" -e Makefile.dep -e third_party
> > > /home/burii/builds_workspace/tarantool/master/rpm/tarantool.spec:%{_includedir}/tarantool/lualib.h
> > > Binary file /home/burii/builds_workspace/tarantool/master/cmake/.luajit.cmake.swp matches
> > > /home/burii/builds_workspace/tarantool/master/cmake/luajit.cmake: install (FILES ${inc}/lua.h ${inc}/lualib.h ${inc}/lauxlib.h
> > > /home/burii/builds_workspace/tarantool/master/FreeBSD/databases/tarantool/pkg-plist:include/tarantool/lualib.h
> > > ===================================================================
> > > 
> > > Did I miss something?
> > 
> > I guess no: Gentoo ebuilds are maintained outside the Tarantool repo by
> > Sasha Tu. However, I have no idea, how packages for Debian-based distros
> > are built. Anyway, it looks everything inside this repo is fixed.
> > 
> > > 
> > > 
> > >  FreeBSD/databases/tarantool/pkg-plist | 1 +
> > >  cmake/luajit.cmake                    | 2 +-
> > >  rpm/tarantool.spec                    | 1 +
> > >  3 files changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/FreeBSD/databases/tarantool/pkg-plist b/FreeBSD/databases/tarantool/pkg-plist
> > > index 9e3905d60..6df72be9d 100644
> > > --- a/FreeBSD/databases/tarantool/pkg-plist
> > > +++ b/FreeBSD/databases/tarantool/pkg-plist
> > > @@ -8,6 +8,7 @@ include/tarantool/lua.hpp
> > >  include/tarantool/luaconf.h
> > >  include/tarantool/luajit.h
> > >  include/tarantool/lualib.h
> > > +include/tarantool/lmisclib.h
> > 
> > Minor: Please sort the entries.
> 
> I've sorted entries here and inside rpm/tarantool.spec. See itterative
> patch below. Branch is force pushed.
> 
> > 
> > >  include/tarantool/module.h
> > >  man/man1/tarantool.1.gz
> > >  man/man1/tarantoolctl.1.gz
> > 
> > <snipped>
> > 
> > > diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> > > index eedc0312c..3a1a8f2be 100644
> > > --- a/rpm/tarantool.spec
> > > +++ b/rpm/tarantool.spec
> > > @@ -256,6 +256,7 @@ fi
> > >  %{_includedir}/tarantool/luajit.h
> > >  %{_includedir}/tarantool/lualib.h
> > >  %{_includedir}/tarantool/module.h
> > > +%{_includedir}/tarantool/lmisclib.h
> > >  
> > >  %changelog
> > >  * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
> > 
> > Side note: It looks we totally ignore updating %changelog section.
> > 
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> ===================================================================
> diff --git a/FreeBSD/databases/tarantool/pkg-plist b/FreeBSD/databases/tarantool/pkg-plist
> index 6df72be9d..8e2f3afde 100644
> --- a/FreeBSD/databases/tarantool/pkg-plist
> +++ b/FreeBSD/databases/tarantool/pkg-plist
> @@ -3,12 +3,12 @@ bin/tarantoolctl
>  %%ETCDIR%%/default/tarantool
>  %%ETCDIR%%/instances.available/example.lua
>  include/tarantool/lauxlib.h
> +include/tarantool/lmisclib.h
>  include/tarantool/lua.h
>  include/tarantool/lua.hpp
>  include/tarantool/luaconf.h
>  include/tarantool/luajit.h
>  include/tarantool/lualib.h
> -include/tarantool/lmisclib.h
>  include/tarantool/module.h
>  man/man1/tarantool.1.gz
>  man/man1/tarantoolctl.1.gz
> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index 3a1a8f2be..9ac7a1a04 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec
> @@ -250,13 +250,13 @@ fi
>  %files devel
>  %dir %{_includedir}/tarantool
>  %{_includedir}/tarantool/lauxlib.h
> +%{_includedir}/tarantool/lmisclib.h
>  %{_includedir}/tarantool/luaconf.h
>  %{_includedir}/tarantool/lua.h
>  %{_includedir}/tarantool/lua.hpp
>  %{_includedir}/tarantool/luajit.h
>  %{_includedir}/tarantool/lualib.h
>  %{_includedir}/tarantool/module.h
> -%{_includedir}/tarantool/lmisclib.h
>  
>  %changelog
>  * Tue Sep 12 2017 Roman Tsisyk <roman@tarantool.org> 1.7.5.46-1
> ===================================================================
> 
> -- 
> Best regards,
> Sergey Kaplun

[Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols

[Sergey Kaplun]

Since LuaJIT provides public C API symbols that are used in the final
executable, the linker may not just throw it away.
Nevertheless for future compatibility all symbols from LuaJIT public API
should be additionaly added at exports.h file.

Follows up tarantool/tarantool#5187
---
 src/exports.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/exports.h b/src/exports.h
index 8bab22574..867a027dc 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -400,6 +400,7 @@ EXPORT(luaL_traceback)
 EXPORT(luaL_typerror)
 EXPORT(luaL_unref)
 EXPORT(luaL_where)
+EXPORT(luaM_metrics)
 EXPORT(luaopen_base)
 EXPORT(luaopen_bit)
 EXPORT(luaopen_debug)
@@ -407,6 +408,7 @@ EXPORT(luaopen_ffi)
 EXPORT(luaopen_io)
 EXPORT(luaopen_jit)
 EXPORT(luaopen_math)
+EXPORT(luaopen_misc)
 EXPORT(luaopen_os)
 EXPORT(luaopen_package)
 EXPORT(luaopen_string)
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols

[Igor Munkin]

Sergey,

Thanks for the patch! LGTM, except a single nit.

On 17.10.20, Sergey Kaplun wrote:
> Since LuaJIT provides public C API symbols that are used in the final
> executable, the linker may not just throw it away.
> Nevertheless for future compatibility all symbols from LuaJIT public API
> should be additionaly added at exports.h file.
> 
> Follows up tarantool/tarantool#5187

Minor: You don't need to use full repo reference here.

> ---
>  src/exports.h | 2 ++
>  1 file changed, 2 insertions(+)
> 

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols

[Sergey Kaplun]

Igor,

Thanks for the review! I've updated commit message considering your
comment here. Branch is force pushed.

On 18.10.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! LGTM, except a single nit.
> 
> On 17.10.20, Sergey Kaplun wrote:
> > Since LuaJIT provides public C API symbols that are used in the final
> > executable, the linker may not just throw it away.
> > Nevertheless for future compatibility all symbols from LuaJIT public API
> > should be additionaly added at exports.h file.
> > 
> > Follows up tarantool/tarantool#5187
> 
> Minor: You don't need to use full repo reference here.
> 
> > ---
> >  src/exports.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> 
> <snipped>
> 
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols

[Sergey Ostanevich]

Hi!

I reviewed the latest branch, LGTM with some updates in message below.

On 19 окт 07:57, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review! I've updated commit message considering your
> comment here. Branch is force pushed.
> 
> On 18.10.20, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! LGTM, except a single nit.
> > 
> > On 17.10.20, Sergey Kaplun wrote:
> > > Since LuaJIT provides public C API symbols that are used in the final
> > > executable, the linker may not just throw it away.
> > > Nevertheless for future compatibility all symbols from LuaJIT public API
> > > should be additionaly added at exports.h file.
                ^^^^^^^^^^^^^^^^^^^^
Perhaps simply
                added to

> > > 
> > > Follows up tarantool/tarantool#5187
> > 
> > Minor: You don't need to use full repo reference here.
> > 
> > > ---
> > >  src/exports.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > 
> > <snipped>
> > 
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols

[Sergey Kaplun]

Hi, Sergos!

Thanks for the review!

On 20.10.20, Sergey Ostanevich wrote:
> Hi!
> 
> I reviewed the latest branch, LGTM with some updates in message below.
> 
> On 19 окт 07:57, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review! I've updated commit message considering your
> > comment here. Branch is force pushed.
> > 
> > On 18.10.20, Igor Munkin wrote:
> > > Sergey,
> > > 
> > > Thanks for the patch! LGTM, except a single nit.
> > > 
> > > On 17.10.20, Sergey Kaplun wrote:
> > > > Since LuaJIT provides public C API symbols that are used in the final
> > > > executable, the linker may not just throw it away.
> > > > Nevertheless for future compatibility all symbols from LuaJIT public API
> > > > should be additionaly added at exports.h file.
>                 ^^^^^^^^^^^^^^^^^^^^
> Perhaps simply
>                 added to

Yep, sounds better. Updated and force pushed to the branch.
> 
> > > > 
> > > > Follows up tarantool/tarantool#5187
> > > 
> > > Minor: You don't need to use full repo reference here.
> > > 
> > > > ---
> > > >  src/exports.h | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > 
> > > <snipped>
> > > 
> > > > -- 
> > > > 2.28.0
> > > > 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library

[Alexander Turenko]

LGTM from my side.

I don't see any problems in the patchset.

External Gentoo ebuild, Arch PKGBUILD, FreeBSD port do not list headers
to install explicitly (just like in-repo Debian rules).

https://github.com/tarantool/gentoo-overlay/blob/master/dev-db/tarantool/tarantool-9999.ebuild
https://github.com/archlinux/svntogit-community/blob/master/tarantool/trunk/PKGBUILD
https://svnweb.freebsd.org/ports/head/databases/tarantool/Makefile?view=markup

WBR, Alexander Turenko.

On Sat, Oct 17, 2020 at 02:13:29PM +0300, Sergey Kaplun wrote:
> This patchset consists from 2 parts:
> 1) Provide corresponding new LuaJIT header within tarantool at install.
> 2) Add missing public C API symbols from the new lib into exports.h
> 
> Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5187-luajit-include-headers
> 
> CI is red, but fails related to replication tests or at osx_* jobs
> (see also https://github.com/tarantool/tarantool/issues/5423).
> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/203892776
> 
> Sergey Kaplun (2):
>   build: provide missing LuaJIT lmisclib.h header
>   lua: add missing LuaJIT export symbols
> 
>  FreeBSD/databases/tarantool/pkg-plist | 1 +
>  cmake/luajit.cmake                    | 2 +-
>  rpm/tarantool.spec                    | 1 +
>  src/exports.h                         | 2 ++
>  4 files changed, 5 insertions(+), 1 deletion(-)
> 
> -- 
> 2.28.0
> 

Re: [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library

[Alexander V. Tikhonov]

Hi Sergey, Ive checked tests results of your patchset, and no new
degradations found [1]. Patchset LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/205179305

On Sat, Oct 17, 2020 at 02:13:29PM +0300, Sergey Kaplun wrote:
> This patchset consists from 2 parts:
> 1) Provide corresponding new LuaJIT header within tarantool at install.
> 2) Add missing public C API symbols from the new lib into exports.h
> 
> Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5187-luajit-include-headers
> 
> CI is red, but fails related to replication tests or at osx_* jobs
> (see also https://github.com/tarantool/tarantool/issues/5423).
> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/203892776
> 
> Sergey Kaplun (2):
>   build: provide missing LuaJIT lmisclib.h header
>   lua: add missing LuaJIT export symbols
> 
>  FreeBSD/databases/tarantool/pkg-plist | 1 +
>  cmake/luajit.cmake                    | 2 +-
>  rpm/tarantool.spec                    | 1 +
>  src/exports.h                         | 2 ++
>  4 files changed, 5 insertions(+), 1 deletion(-)
> 
> -- 
> 2.28.0
>