[Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Since this build flag has been removed as a result of reverting the
tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
cmake file is irrelevant.

Furthermore, considering the breakage faced in #4770 the following tests
are introduced:
* the check whether space __pairs metamethod is set to space.pairs to
  create a Lua Fun iterator that handles __pairs manually underneath.
* the check whether pairs builtin behaviour doesn't change when __pairs
  is set e.g. on space object.

Follow-up #4560
Closes #4770

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

Issue: https://github.com/tarantool/tarantool/issues/4770
Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4770-broken-pairs

@ChangeLog (need to be added to already released versions):
| The feature is removed since we faced the issues with the backward
| compatibility between Lua 5.1 and Lua 5.2 within Tarantool modules as
| well as other third party code (see #4770).

 cmake/luajit.cmake     |  1 -
 test/box/misc.result   | 40 ++++++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua | 15 +++++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 072db8269..10df633d5 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -217,7 +217,6 @@ macro(luajit_build)
         add_definitions(-DLUAJIT_USE_ASAN=1)
         set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
     endif()
-    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
     add_definitions(-DLUAJIT_SMART_STRINGS=1)
     # Add all COMPILE_DEFINITIONS to xcflags
     get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
diff --git a/test/box/misc.result b/test/box/misc.result
index 5ac5e0f26..6fc035171 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1431,3 +1431,43 @@ test_run:grep_log('default', 'test log')
 ---
 - test log
 ...
+--
+-- gh-4770: Iteration through space with Lua builtin pairs routine
+--
+box.cfg{}
+---
+...
+s = box.schema.create_space('test')
+---
+...
+-- Check whether __pairs is set for the space object, since Lua Fun
+-- handles it manually underneath.
+getmetatable(s).__pairs == space.pairs
+---
+- true
+...
+-- Check whether pairs builtin behaviour doesn't change when the
+-- __pairs is set.
+keys = {}
+---
+...
+for k, v in pairs(s) do keys[k] = true end
+---
+...
+keys
+---
+- engine: true
+  before_replace: true
+  field_count: true
+  id: true
+  on_replace: true
+  temporary: true
+  index: true
+  is_local: true
+  enabled: true
+  name: true
+  ck_constraint: true
+...
+s:drop()
+---
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index e1c2f990f..1c8ec95eb 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -415,3 +415,18 @@ tuple_format = box.internal.new_tuple_format(format)
 box.cfg{}
 local ffi = require'ffi' ffi.C._say(ffi.C.S_WARN, nil, 0, nil, "%s", "test log")
 test_run:grep_log('default', 'test log')
+
+--
+-- gh-4770: Iteration through space with Lua builtin pairs routine
+--
+box.cfg{}
+s = box.schema.create_space('test')
+-- Check whether __pairs is set for the space object, since Lua Fun
+-- handles it manually underneath.
+getmetatable(s).__pairs == space.pairs
+-- Check whether pairs builtin behaviour doesn't change when the
+-- __pairs is set.
+keys = {}
+for k, v in pairs(s) do keys[k] = true end
+keys
+s:drop()
-- 
2.25.0

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Konstantin Osipov]

* Igor Munkin <imun@tarantool.org> [20/02/20 18:27]:

This is pathetic. Don't you realize that once a version is
released and declared stable, it's a legacy you have to live with?

Now you have *3* broken behaviours to support, instead of just
two. Some people will still use a version which they believe is
stable, and encounter strange failures.

Eventually you'll have to enable 5.2 compatibility anyway (are you
going to stay with 5.1 forever??), and then break it again.

luafun is a builtin and table.deepcopy are builtins. What prevented you from fixing them 
instead?
That would fix 99% of cases. 


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Kostja,

On 20.02.20, Konstantin Osipov wrote:
> * Igor Munkin <imun@tarantool.org> [20/02/20 18:27]:
> 
> This is pathetic. Don't you realize that once a version is
> released and declared stable, it's a legacy you have to live with?

As we announced the patch was applied by mistake. If you found the place
we missed, please show it. If you want to complain and blame someone,
feel free to choose me, since I initially proposed this amiss and
breaking solution in #4560[1]. Unfortunately this doesn't make the
situation easier for both of us.

The fact we applied the patch makes me sorry (but I'll try to omit
emotions from the ml). Moreover, such breakage can't be introduced
within a minor release. Originally it was needed to implement the PoC
for #4521[2] and wasn't considered as a feature per se. It formally
breaks the semantics of the language currently being used for out
platform. As we can see it really breaks the existing code.

> 
> Now you have *3* broken behaviours to support, instead of just
> two. Some people will still use a version which they believe is
> stable, and encounter strange failures.

I'm bad in math here, since I see only two behaviours: the old one and
the broken one. Three would be if we settle the issue and save the
introduced behaviour the way you proposed below. Then there will be:
* working Tarantool w/o __pairs
* broken Tarantool w/ __pairs
* working Tarantool w/ __pairs

But again: I might miscount it, so it would be great if you clarify your
math.

Yes, there are stable releases poisoned with the introduced breakage. If
you think I'm careless about it, you're wrong. I guess we can ask users
to upgrade like many times you used to say "obnovites" with apoligies. I
wish we could avoid it this time. But I see more problems with the
platform infrastructure and environment as a result than "blacklisting"
several Tarantool versions.

> 
> Eventually you'll have to enable 5.2 compatibility anyway (are you
> going to stay with 5.1 forever??), and then break it again.

I can't answer it now. You are the visionary, I'm even not a mainainer.
We discussed it in tg chat some time ago but I haven't been answered the
question about your vision of this question. I welcome you to proceed
the discussion and you can choose the most convenient way for it.

> 
> luafun is a builtin and table.deepcopy are builtins. What prevented you from fixing them 
> instead?
> That would fix 99% of cases. 

I see no guarantees here, and guess you can provide nothing.

> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

[1]: https://github.com/tarantool/tarantool/issues/4560
[2]: https://github.com/tarantool/tarantool/issues/4521

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Konstantin Osipov]

* Igor Munkin <imun@tarantool.org> [20/02/20 23:57]:
> > This is pathetic. Don't you realize that once a version is
> > released and declared stable, it's a legacy you have to live with?

> 
> As we announced the patch was applied by mistake. If you found the place
> we missed, please show it. If you want to complain and blame someone,
> feel free to choose me, since I initially proposed this amiss and
> breaking solution in #4560[1]. Unfortunately this doesn't make the
> situation easier for both of us.

It's not a blame in this case, I simply reply to a commit I have a
chance to reply to.  The revert commit went in without any
discussion on the list and without a code review.

I looked at 4560 now and realized that the incompatible change was
added to 1.10 as well. Ugh. I guess then it's definitely correct
to revert it there... so I withdraw my suggestion to keep it.

> > Now you have *3* broken behaviours to support, instead of just
> > two. Some people will still use a version which they believe is
> > stable, and encounter strange failures.
> 
> I'm bad in math here, since I see only two behaviours: the old one and
> the broken one. Three would be if we settle the issue and save the
> introduced behaviour the way you proposed below. Then there will be:
> * working Tarantool w/o __pairs
> * broken Tarantool w/ __pairs
> * working Tarantool w/ __pairs

Well, I simply count before-during-after, I'm not that
sophisticated here.

> > Eventually you'll have to enable 5.2 compatibility anyway (are you
> > going to stay with 5.1 forever??), and then break it again.
> 
> I can't answer it now. You are the visionary, I'm even not a mainainer.

1) Nobody is a maintainer, even the people who push patches.
   Maintenance in particular includes assuming personal public
   responsibility in the community for one's work. Maintenance
   also is not something assigned by a management, it has to be 
   supported by contributions to the code base and recognized by
   peers.

2) If Tarantool sticks to the release policy, there won't be any
   other window to introduce __pairs till 3.x, which is still a
   few years ahead. This is what drives me crazy - I thought you
   only pushed it to 2.x and in that case the patch definitely
   should have stayed.

> We discussed it in tg chat some time ago but I haven't been answered the
> question about your vision of this question. I welcome you to proceed
> the discussion and you can choose the most convenient way for it.

> > luafun is a builtin and table.deepcopy are builtins. What prevented you from fixing them 
> > instead?
> > That would fix 99% of cases. 
> 
> I see no guarantees here, and guess you can provide nothing.

I agree, but I trust our test coverage.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 comments below.

>     build: disable LUAJIT_ENABLE_PAIRSMM
>     
>     Since this build flag has been removed as a result of reverting the
>     tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
>     cmake file is irrelevant.
>     
>     Furthermore, considering the breakage faced in #4770 the following tests
>     are introduced:
>     * the check whether space __pairs metamethod is set to space.pairs to
>       create a Lua Fun iterator that handles __pairs manually underneath.
>     * the check whether pairs builtin behaviour doesn't change when __pairs
>       is set e.g. on space object.
>     
>     Follow-up #4560
>     Closes #4770
>     
>     Signed-off-by: Igor Munkin <imun@tarantool.org>
> 
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..10df633d5 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -217,7 +217,6 @@ macro(luajit_build)
>          add_definitions(-DLUAJIT_USE_ASAN=1)
>          set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
>      endif()
> -    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
>      add_definitions(-DLUAJIT_SMART_STRINGS=1)
>      # Add all COMPILE_DEFINITIONS to xcflags
>      get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 5ac5e0f26..6fc035171 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -1431,3 +1431,43 @@ test_run:grep_log('default', 'test log')
>  ---
>  - test log
>  ...
> +--
> +-- gh-4770: Iteration through space with Lua builtin pairs routine
> +--
> +box.cfg{}

1. box.cfg{} is already done by test/box/box.lua, which starts
misc.test.lua.

> +---
> +...
> +s = box.schema.create_space('test')
> +---
> +...
> +-- Check whether __pairs is set for the space object, since Lua Fun
> +-- handles it manually underneath.
> +getmetatable(s).__pairs == space.pairs
> +---
> +- true
> +...
> +-- Check whether pairs builtin behaviour doesn't change when the
> +-- __pairs is set.
> +keys = {}
> +---
> +...
> +for k, v in pairs(s) do keys[k] = true end
> +---
> +...
> +keys
> +---
> +- engine: true
> +  before_replace: true
> +  field_count: true
> +  id: true
> +  on_replace: true
> +  temporary: true
> +  index: true
> +  is_local: true
> +  enabled: true
> +  name: true
> +  ck_constraint: true

2. I wouldn't print all the keys, since this test will break
anytime when something added to space. I would just check that
a couple of fields exists: keys.engine, keys.id, keys.name.
Up to you.

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Kirill Yukhin]

Hello,

On 20 фев 18:20, Igor Munkin wrote:
> Since this build flag has been removed as a result of reverting the
> tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
> cmake file is irrelevant.
> 
> Furthermore, considering the breakage faced in #4770 the following tests
> are introduced:
> * the check whether space __pairs metamethod is set to space.pairs to
>   create a Lua Fun iterator that handles __pairs manually underneath.
> * the check whether pairs builtin behaviour doesn't change when __pairs
>   is set e.g. on space object.
> 
> Follow-up #4560
> Closes #4770
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/4770
> Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4770-broken-pairs
> 
> @ChangeLog (need to be added to already released versions):
> | The feature is removed since we faced the issues with the backward
> | compatibility between Lua 5.1 and Lua 5.2 within Tarantool modules as
> | well as other third party code (see #4770).
> 
>  cmake/luajit.cmake     |  1 -
>  test/box/misc.result   | 40 ++++++++++++++++++++++++++++++++++++++++
>  test/box/misc.test.lua | 15 +++++++++++++++

Could you please extract the test into separate file?

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Vlad,

Thanks for your review! I sent the second version for the patch.

On 03.03.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 2 comments below.
> 
> >     build: disable LUAJIT_ENABLE_PAIRSMM
> >     
> >     Since this build flag has been removed as a result of reverting the
> >     tarantool/luajit@d4e985a, its definition in the corresponding Tarantool
> >     cmake file is irrelevant.
> >     
> >     Furthermore, considering the breakage faced in #4770 the following tests
> >     are introduced:
> >     * the check whether space __pairs metamethod is set to space.pairs to
> >       create a Lua Fun iterator that handles __pairs manually underneath.
> >     * the check whether pairs builtin behaviour doesn't change when __pairs
> >       is set e.g. on space object.
> >     
> >     Follow-up #4560
> >     Closes #4770
> >     
> >     Signed-off-by: Igor Munkin <imun@tarantool.org>
> > 
> > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> > index 072db8269..10df633d5 100644
> > --- a/cmake/luajit.cmake
> > +++ b/cmake/luajit.cmake
> > @@ -217,7 +217,6 @@ macro(luajit_build)
> >          add_definitions(-DLUAJIT_USE_ASAN=1)
> >          set (luajit_ldflags ${luajit_ldflags} -fsanitize=address)
> >      endif()
> > -    add_definitions(-DLUAJIT_ENABLE_PAIRSMM=1)
> >      add_definitions(-DLUAJIT_SMART_STRINGS=1)
> >      # Add all COMPILE_DEFINITIONS to xcflags
> >      get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS)
> > diff --git a/test/box/misc.result b/test/box/misc.result
> > index 5ac5e0f26..6fc035171 100644
> > --- a/test/box/misc.result
> > +++ b/test/box/misc.result
> > @@ -1431,3 +1431,43 @@ test_run:grep_log('default', 'test log')
> >  ---
> >  - test log
> >  ...
> > +--
> > +-- gh-4770: Iteration through space with Lua builtin pairs routine
> > +--
> > +box.cfg{}
> 
> 1. box.cfg{} is already done by test/box/box.lua, which starts
> misc.test.lua.
> 

Since the test is moved to a separate file (as was requested by Kirill),
I left the box.cfg call.

> > +---
> > +...
> > +s = box.schema.create_space('test')
> > +---
> > +...
> > +-- Check whether __pairs is set for the space object, since Lua Fun
> > +-- handles it manually underneath.
> > +getmetatable(s).__pairs == space.pairs
> > +---
> > +- true
> > +...
> > +-- Check whether pairs builtin behaviour doesn't change when the
> > +-- __pairs is set.
> > +keys = {}
> > +---
> > +...
> > +for k, v in pairs(s) do keys[k] = true end
> > +---
> > +...
> > +keys
> > +---
> > +- engine: true
> > +  before_replace: true
> > +  field_count: true
> > +  id: true
> > +  on_replace: true
> > +  temporary: true
> > +  index: true
> > +  is_local: true
> > +  enabled: true
> > +  name: true
> > +  ck_constraint: true
> 
> 2. I wouldn't print all the keys, since this test will break
> anytime when something added to space. I would just check that
> a couple of fields exists: keys.engine, keys.id, keys.name.
> Up to you.

IMHO this as a minor change and I guess we can postpone this remark
waiting the second reviewer's (I hope Sasha will take a look soon)
opinion. Ignoring for now.

> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] build: disable LUAJIT_ENABLE_PAIRSMM

[Igor Munkin]

Kirill,

On 05.03.20, Kirill Yukhin wrote:
> Hello,
> 
<snipped>
> 
> Could you please extract the test into separate file?
> 

Done, updated the branch, sent the second version of the patch.

> --
> Regards, Kirill Yukhin

-- 
Best regards,
IM