* [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library @ 2020-10-17 11:13 Sergey Kaplun 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header Sergey Kaplun ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Sergey Kaplun @ 2020-10-17 11:13 UTC (permalink / raw) To: Alexander Turenko, Igor Munkin, Alexander V . Tikhonov; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header 2020-10-17 11:13 [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Sergey Kaplun @ 2020-10-17 11:13 ` Sergey Kaplun 2020-10-18 19:19 ` Igor Munkin 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols Sergey Kaplun ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Sergey Kaplun @ 2020-10-17 11:13 UTC (permalink / raw) To: Alexander Turenko, Igor Munkin, Alexander V . Tikhonov; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header Sergey Kaplun @ 2020-10-18 19:19 ` Igor Munkin 2020-10-19 4:55 ` Sergey Kaplun 0 siblings, 1 reply; 12+ messages in thread From: Igor Munkin @ 2020-10-18 19:19 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches, Alexander Turenko 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header 2020-10-18 19:19 ` Igor Munkin @ 2020-10-19 4:55 ` Sergey Kaplun 2020-10-20 12:07 ` Sergey Ostanevich 0 siblings, 1 reply; 12+ messages in thread From: Sergey Kaplun @ 2020-10-19 4:55 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header 2020-10-19 4:55 ` Sergey Kaplun @ 2020-10-20 12:07 ` Sergey Ostanevich 0 siblings, 0 replies; 12+ messages in thread From: Sergey Ostanevich @ 2020-10-20 12:07 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Alexander Turenko, tarantool-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols 2020-10-17 11:13 [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Sergey Kaplun 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header Sergey Kaplun @ 2020-10-17 11:13 ` Sergey Kaplun 2020-10-18 18:43 ` Igor Munkin 2020-10-21 16:08 ` [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Alexander Turenko 2020-10-21 16:17 ` Alexander V. Tikhonov 3 siblings, 1 reply; 12+ messages in thread From: Sergey Kaplun @ 2020-10-17 11:13 UTC (permalink / raw) To: Alexander Turenko, Igor Munkin, Alexander V . Tikhonov; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols Sergey Kaplun @ 2020-10-18 18:43 ` Igor Munkin 2020-10-19 4:57 ` Sergey Kaplun 0 siblings, 1 reply; 12+ messages in thread From: Igor Munkin @ 2020-10-18 18:43 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches, Alexander Turenko 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols 2020-10-18 18:43 ` Igor Munkin @ 2020-10-19 4:57 ` Sergey Kaplun 2020-10-20 12:09 ` Sergey Ostanevich 0 siblings, 1 reply; 12+ messages in thread From: Sergey Kaplun @ 2020-10-19 4:57 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols 2020-10-19 4:57 ` Sergey Kaplun @ 2020-10-20 12:09 ` Sergey Ostanevich 2020-10-20 13:03 ` Sergey Kaplun 0 siblings, 1 reply; 12+ messages in thread From: Sergey Ostanevich @ 2020-10-20 12:09 UTC (permalink / raw) To: Sergey Kaplun; +Cc: Alexander Turenko, tarantool-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols 2020-10-20 12:09 ` Sergey Ostanevich @ 2020-10-20 13:03 ` Sergey Kaplun 0 siblings, 0 replies; 12+ messages in thread From: Sergey Kaplun @ 2020-10-20 13:03 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: Alexander Turenko, tarantool-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library 2020-10-17 11:13 [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Sergey Kaplun 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header Sergey Kaplun 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols Sergey Kaplun @ 2020-10-21 16:08 ` Alexander Turenko 2020-10-21 16:17 ` Alexander V. Tikhonov 3 siblings, 0 replies; 12+ messages in thread From: Alexander Turenko @ 2020-10-21 16:08 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library 2020-10-17 11:13 [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Sergey Kaplun ` (2 preceding siblings ...) 2020-10-21 16:08 ` [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Alexander Turenko @ 2020-10-21 16:17 ` Alexander V. Tikhonov 3 siblings, 0 replies; 12+ messages in thread From: Alexander V. Tikhonov @ 2020-10-21 16:17 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-21 16:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-17 11:13 [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Sergey Kaplun 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 1/2] build: provide missing LuaJIT lmisclib.h header Sergey Kaplun 2020-10-18 19:19 ` Igor Munkin 2020-10-19 4:55 ` Sergey Kaplun 2020-10-20 12:07 ` Sergey Ostanevich 2020-10-17 11:13 ` [Tarantool-patches] [PATCH 2/2] lua: add missing LuaJIT export symbols Sergey Kaplun 2020-10-18 18:43 ` Igor Munkin 2020-10-19 4:57 ` Sergey Kaplun 2020-10-20 12:09 ` Sergey Ostanevich 2020-10-20 13:03 ` Sergey Kaplun 2020-10-21 16:08 ` [Tarantool-patches] [PATCH 0/2] Minor fixes for new LuaJIT extention library Alexander Turenko 2020-10-21 16:17 ` Alexander V. Tikhonov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox