[Tarantool-patches] [PATCH 0/2] Fix broken CI after introduced LuaJIT metrics

[Tarantool-patches] [PATCH 0/2] Fix broken CI after introduced LuaJIT metrics

[Sergey Kaplun]

This patch fixes the regressions introduced in scope of
5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
platform metrics').

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-fix-tests-fails-ci
This branch is rebased on Igor's branch:
https://github.com/tarantool/luajit/tree/imun/gh-5187-fix-disp-encoding-on-gc64

CI: https://gitlab.com/tarantool/tarantool/-/pipelines/202569424/builds
Side note: failing CI at osx jobs related to this issue:
https://github.com/tarantool/tarantool/issues/5423

Sergey Kaplun (2):
  test: force enable assert checks in release build
  test: disable jit related tests at FreeBSD

 test/misclib-getmetrics-capi.skipcond         | 7 +++++++
 test/misclib-getmetrics-capi/testgetmetrics.c | 1 +
 test/misclib-getmetrics-lapi.skipcond         | 7 +++++++
 3 files changed, 15 insertions(+)
 create mode 100644 test/misclib-getmetrics-capi.skipcond
 create mode 100644 test/misclib-getmetrics-lapi.skipcond

-- 
2.28.0

[Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build

[Sergey Kaplun]

This patch fixes the regression introduced in scope of
5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
platform metrics'). As a result of the patch release build
was failed according to -Werror compiler flag and unused variables that
used only for assertions checks.

Force #undef NDEBUG directive leaves asserts on and allows not disable
newly added tests for release build.

Follows up tarantool/tarantool#5187
---
 test/misclib-getmetrics-capi/testgetmetrics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index 8844b17..3b6f599 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -4,6 +4,7 @@
 
 #include <lmisclib.h>
 
+#undef NDEBUG
 #include <assert.h>
 
 static int base(lua_State *L)
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build

[Igor Munkin]

Sergey,

Thanks for the patch! Consider a couple nits below.

On 14.10.20, Sergey Kaplun wrote:
> This patch fixes the regression introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> platform metrics'). As a result of the patch release build
> was failed according to -Werror compiler flag and unused variables that
> used only for assertions checks.
> 
> Force #undef NDEBUG directive leaves asserts on and allows not disable

Minor: IMHO "leaves asserts enabled, so the introduced tests works even
with release build" is a little better, but this doesn't change the
meaning, so feel free to ignore.

> newly added tests for release build.
> 
> Follows up tarantool/tarantool#5187
> ---
>  test/misclib-getmetrics-capi/testgetmetrics.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> index 8844b17..3b6f599 100644
> --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> @@ -4,6 +4,7 @@
>  
>  #include <lmisclib.h>
>  
> +#undef NDEBUG

Minor: It would be nice to drop a few words regarding this hack. At the
same time, the code is well-structured, so it's quite obvious that you
want asserts to be always enabled. Feel free to ignore.

>  #include <assert.h>
>  
>  static int base(lua_State *L)
> -- 
> 2.28.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build

[Igor Munkin]

Sorry, almost forgot the magic letters: LGTM!

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build

[Sergey Kaplun]

Hi, Igor! Thanks for the review!

On 14.10.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Consider a couple nits below.
> 
> On 14.10.20, Sergey Kaplun wrote:
> > This patch fixes the regression introduced in scope of
> > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> > platform metrics'). As a result of the patch release build
> > was failed according to -Werror compiler flag and unused variables that
> > used only for assertions checks.
> > 
> > Force #undef NDEBUG directive leaves asserts on and allows not disable
> 
> Minor: IMHO "leaves asserts enabled, so the introduced tests works even
> with release build" is a little better, but this doesn't change the
> meaning, so feel free to ignore.

I've updated commit message and branch with that.

> 
> > newly added tests for release build.
> > 
> > Follows up tarantool/tarantool#5187
> > ---
> >  test/misclib-getmetrics-capi/testgetmetrics.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> > index 8844b17..3b6f599 100644
> > --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> > +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> > @@ -4,6 +4,7 @@
> >  
> >  #include <lmisclib.h>
> >  
> > +#undef NDEBUG
> 
> Minor: It would be nice to drop a few words regarding this hack. At the
> same time, the code is well-structured, so it's quite obvious that you
> want asserts to be always enabled. Feel free to ignore.

I've Added tiny comment to make changes obvious. See iterative patch
below. Branch is force pushed.

> 
> >  #include <assert.h>
> >  
> >  static int base(lua_State *L)
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

===================================================================
diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index 3b6f599..768c1a9 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -4,6 +4,7 @@
 
 #include <lmisclib.h>
 
+/* Force enable asserts for release build. */
 #undef NDEBUG
 #include <assert.h>
 
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 1/2] test: force enable assert checks in release build

[Sergey Ostanevich]

Hi!

Thanks for the patch!

LGTM.

Sergos.

On 15 окт 10:23, Sergey Kaplun wrote:
> Hi, Igor! Thanks for the review!
> 
> On 14.10.20, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Consider a couple nits below.
> > 
> > On 14.10.20, Sergey Kaplun wrote:
> > > This patch fixes the regression introduced in scope of
> > > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> > > platform metrics'). As a result of the patch release build
> > > was failed according to -Werror compiler flag and unused variables that
> > > used only for assertions checks.
> > > 
> > > Force #undef NDEBUG directive leaves asserts on and allows not disable
> > 
> > Minor: IMHO "leaves asserts enabled, so the introduced tests works even
> > with release build" is a little better, but this doesn't change the
> > meaning, so feel free to ignore.
> 
> I've updated commit message and branch with that.
> 
> > 
> > > newly added tests for release build.
> > > 
> > > Follows up tarantool/tarantool#5187
> > > ---
> > >  test/misclib-getmetrics-capi/testgetmetrics.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> > > index 8844b17..3b6f599 100644
> > > --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> > > +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> > > @@ -4,6 +4,7 @@
> > >  
> > >  #include <lmisclib.h>
> > >  
> > > +#undef NDEBUG
> > 
> > Minor: It would be nice to drop a few words regarding this hack. At the
> > same time, the code is well-structured, so it's quite obvious that you
> > want asserts to be always enabled. Feel free to ignore.
> 
> I've Added tiny comment to make changes obvious. See iterative patch
> below. Branch is force pushed.
> 
> > 
> > >  #include <assert.h>
> > >  
> > >  static int base(lua_State *L)
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> ===================================================================
> diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
> index 3b6f599..768c1a9 100644
> --- a/test/misclib-getmetrics-capi/testgetmetrics.c
> +++ b/test/misclib-getmetrics-capi/testgetmetrics.c
> @@ -4,6 +4,7 @@
>  
>  #include <lmisclib.h>
>  
> +/* Force enable asserts for release build. */
>  #undef NDEBUG
>  #include <assert.h>
>  
> ===================================================================
> 
> -- 
> Best regards,
> Sergey Kaplun

[Tarantool-patches] [PATCH 2/2] test: disable jit related tests at FreeBSD

[Sergey Kaplun]

This patch fixes the regression introduced in scope of
5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
platform metrics').

As far as newly added tests include trace checks and
https://github.com/tarantool/tarantool/issues/4819 (JIT fails to
allocate mcode memory on FreeBSD) has not resolved yet, this tests
should be disabled to for running on FreeBSD.

Follows up tarantool/tarantool#5187
---
 test/misclib-getmetrics-capi.skipcond | 7 +++++++
 test/misclib-getmetrics-lapi.skipcond | 7 +++++++
 2 files changed, 14 insertions(+)
 create mode 100644 test/misclib-getmetrics-capi.skipcond
 create mode 100644 test/misclib-getmetrics-lapi.skipcond

diff --git a/test/misclib-getmetrics-capi.skipcond b/test/misclib-getmetrics-capi.skipcond
new file mode 100644
index 0000000..2a2ec4d
--- /dev/null
+++ b/test/misclib-getmetrics-capi.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:
diff --git a/test/misclib-getmetrics-lapi.skipcond b/test/misclib-getmetrics-lapi.skipcond
new file mode 100644
index 0000000..2a2ec4d
--- /dev/null
+++ b/test/misclib-getmetrics-lapi.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 2/2] test: disable jit related tests at FreeBSD

[Igor Munkin]

Sergey,

Thanks for the patch! It LGTM, but I can't stop nitpicking :)

On 14.10.20, Sergey Kaplun wrote:
> This patch fixes the regression introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> platform metrics').
> 
> As far as newly added tests include trace checks and
> https://github.com/tarantool/tarantool/issues/4819 (JIT fails to

Minor: Let's be consistent in referring external issues:
| tarantool/tarantool#4819

> allocate mcode memory on FreeBSD) has not resolved yet, this tests

Typo: s/this/these/.

> should be disabled to for running on FreeBSD.

Typo: s/to for/while/.

> 
> Follows up tarantool/tarantool#5187
> ---

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 2/2] test: disable jit related tests at FreeBSD

[Sergey Kaplun]

Hi, Igor! Thanks for the review!

I've updated commit message considering your comments and repushed
branch.

On 14.10.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! It LGTM, but I can't stop nitpicking :)
> 
> On 14.10.20, Sergey Kaplun wrote:
> > This patch fixes the regression introduced in scope of
> > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> > platform metrics').
> > 
> > As far as newly added tests include trace checks and
> > https://github.com/tarantool/tarantool/issues/4819 (JIT fails to
> 
> Minor: Let's be consistent in referring external issues:
> | tarantool/tarantool#4819
> 
> > allocate mcode memory on FreeBSD) has not resolved yet, this tests
> 
> Typo: s/this/these/.
> 
> > should be disabled to for running on FreeBSD.
> 
> Typo: s/to for/while/.
> 
> > 
> > Follows up tarantool/tarantool#5187
> > ---
> 
> <snipped>
> 
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 2/2] test: disable jit related tests at FreeBSD

[Sergey Ostanevich]

Hi!

Thanks for the patch!

LGTM.

Sergos

On 15 окт 10:25, Sergey Kaplun wrote:
> Hi, Igor! Thanks for the review!
> 
> I've updated commit message considering your comments and repushed
> branch.
> 
> On 14.10.20, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! It LGTM, but I can't stop nitpicking :)
> > 
> > On 14.10.20, Sergey Kaplun wrote:
> > > This patch fixes the regression introduced in scope of
> > > 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> > > platform metrics').
> > > 
> > > As far as newly added tests include trace checks and
> > > https://github.com/tarantool/tarantool/issues/4819 (JIT fails to
> > 
> > Minor: Let's be consistent in referring external issues:
> > | tarantool/tarantool#4819
> > 
> > > allocate mcode memory on FreeBSD) has not resolved yet, this tests
> > 
> > Typo: s/this/these/.
> > 
> > > should be disabled to for running on FreeBSD.
> > 
> > Typo: s/to for/while/.
> > 
> > > 
> > > Follows up tarantool/tarantool#5187
> > > ---
> > 
> > <snipped>
> > 
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH 0/2] Fix broken CI after introduced LuaJIT metrics

[Igor Munkin]

I believe it's a good practice to add the link with green CI for the
branch too. This is definitely not obligatory, but I guess it can help
to reduce the issues we fixed in scope of this patchset.

On 14.10.20, Sergey Kaplun wrote:
> This patch fixes the regressions introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> platform metrics').
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-fix-tests-fails-ci
> This branch is rebased on Igor's branch:
> https://github.com/tarantool/luajit/tree/imun/gh-5187-fix-disp-encoding-on-gc64
> 
> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/202569424/builds

You've already it here, great!

> Side note: failing CI at osx jobs related to this issue:
> https://github.com/tarantool/tarantool/issues/5423
> 
> Sergey Kaplun (2):
>   test: force enable assert checks in release build
>   test: disable jit related tests at FreeBSD
> 
>  test/misclib-getmetrics-capi.skipcond         | 7 +++++++
>  test/misclib-getmetrics-capi/testgetmetrics.c | 1 +
>  test/misclib-getmetrics-lapi.skipcond         | 7 +++++++
>  3 files changed, 15 insertions(+)
>  create mode 100644 test/misclib-getmetrics-capi.skipcond
>  create mode 100644 test/misclib-getmetrics-lapi.skipcond
> 
> -- 
> 2.28.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 0/2] Fix broken CI after introduced LuaJIT metrics

[Kirill Yukhin]

Hello,

On 14 окт 20:03, Sergey Kaplun wrote:
> This patch fixes the regressions introduced in scope of
> 5a61e1ab54b5c66bfebd836db1ac47996611e065 ('misc: add C and Lua API for
> platform metrics').
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-fix-tests-fails-ci
> This branch is rebased on Igor's branch:
> https://github.com/tarantool/luajit/tree/imun/gh-5187-fix-disp-encoding-on-gc64
> 
> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/202569424/builds
> Side note: failing CI at osx jobs related to this issue:
> https://github.com/tarantool/tarantool/issues/5423
> 
> Sergey Kaplun (2):
>   test: force enable assert checks in release build
>   test: disable jit related tests at FreeBSD

I've checked both patches to tarantool branch of luajit repo and
bumped a new version in 1.10, 2.4, 2.5 and master branches.

--
Regards, Kirill Yukhin