[Tarantool-patches] [PATCH luajit 0/3] Fix out-of-source testing on MacOS

[Tarantool-patches] [PATCH luajit 0/3] Fix out-of-source testing on MacOS

[Igor Munkin via Tarantool-patches]

This series consists of three patches, but only one is related to the
problem per se. So I ought to explain the rationale for two others.

The reason is quite clear: I finally got a round tuit[1]. The first
patch removes an excess artefact left after converting all testing
targets to the corresponding .PHONY rules in scope of #4862. The
changeset was so huge, so it's no wonder such minor thing has been left
unnoticed. The second patch enhances the testing "framework" used in
tarantool-tests set. As a result, one can implement tests for JIT
machinery in a simple way via <utils.selfrun>. The third patch of the
series is the one fixin #5959: there are many words left for posterity
in the commit message (hope you'll have a good time reading it and the
blogpost[2] found by Mons).

Issue: https://github.com/tarantool/tarantool/issues/5959
Branch: https://github.com/tarantool/luajit/tree/imun/gh-5959-fix-dynamic-modules-loading-on-macos
CI: https://github.com/tarantool/tarantool/tree/imun/gh-5959-fix-dynamic-modules-loading-on-macos

[1]: https://en.wiktionary.org/wiki/round_tuit
[2]: https://briandfoy.github.io/macos-s-system-integrity-protection-sanitizes-your-environment/

Igor Munkin (3):
  test: remove excess dependency for tests target
  test: make utils.selfrun usage easier
  test: fix dynamic modules loading on MacOS

 test/tarantool-tests/CMakeLists.txt           | 42 ++++++++++++--
 .../gh-4427-ffi-sandwich.test.lua             | 55 ++++++++++---------
 .../lj-flush-on-trace.test.lua                | 55 ++++++++++---------
 test/tarantool-tests/utils.lua                | 44 +++++++++++++--
 4 files changed, 134 insertions(+), 62 deletions(-)

-- 
2.25.0

[Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Igor Munkin via Tarantool-patches]

TEST_DEPS list was introduced to prevent <tarantool-tests> target from
being run in case no dependencies are changed. As a result of review it
was decided to make all *-tests targets be .PHONY rules, hence TEST_DEPS
variable is excess and can be dropped.

Follows up tarantool/tarantool#4862

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/tarantool-tests/CMakeLists.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 9bb26f32..3e36ff86 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -69,7 +69,6 @@ set(LUA_PATH
 )
 set(LUA_TEST_SUFFIX .test.lua)
 set(LUA_TEST_FLAGS --failures --shuffle)
-file(GLOB TEST_DEPS ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX})
 
 if(CMAKE_VERBOSE_MAKEFILE)
   list(APPEND LUA_TEST_FLAGS --verbose)
@@ -78,7 +77,7 @@ endif()
 # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
 # with dependecies are set in scope of BuildTestLib macro.
 add_custom_target(tarantool-tests
-  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS}
+  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
 )
 add_custom_command(TARGET tarantool-tests
   COMMENT "Running Tarantool tests"
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for the patch!

LGTM, except a few nits below.

On 05.04.21, Igor Munkin wrote:

Nit: I propose the following commit title rewording:
| test: remove excess dependency for tarantool suite
Feel free to ignore.

> TEST_DEPS list was introduced to prevent <tarantool-tests> target from
> being run in case no dependencies are changed. As a result of review it

Typo: s/result of review/result of the review/

> was decided to make all *-tests targets be .PHONY rules, hence TEST_DEPS

Typo? "be" looks excess here. I propose the following rewording
| As a result of the review it was decided to turn all *-tests targets
| into .PHONY rules, hence TEST_DEPS variable is excess and can be
| dropped.

> variable is excess and can be dropped.
> 
> Follows up tarantool/tarantool#4862
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/tarantool-tests/CMakeLists.txt | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 9bb26f32..3e36ff86 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -69,7 +69,6 @@ set(LUA_PATH
>  )
>  set(LUA_TEST_SUFFIX .test.lua)

Nit: looks like `LUA_TEST_SUFFIX` variable is excess now, and we can
move suffix ".test.lua" to `--ext` flag directly.
Feel free to ignore.

>  set(LUA_TEST_FLAGS --failures --shuffle)
> -file(GLOB TEST_DEPS ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX})
>  
>  if(CMAKE_VERBOSE_MAKEFILE)
>    list(APPEND LUA_TEST_FLAGS --verbose)
> @@ -78,7 +77,7 @@ endif()
>  # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
>  # with dependecies are set in scope of BuildTestLib macro.
>  add_custom_target(tarantool-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS}
> +  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
>  )
>  add_custom_command(TARGET tarantool-tests
>    COMMENT "Running Tarantool tests"
> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 06.04.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> 
> LGTM, except a few nits below.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> On 05.04.21, Igor Munkin wrote:
> 
> Nit: I propose the following commit title rewording:
> | test: remove excess dependency for tarantool suite
> Feel free to ignore.

I was thinking a lot about this commit subject and was so close to the
one you proposed, but I afraid it can misguide the reader: I tried to
emphasize, that we removed the *target prerequisite* (in terms of GNU
Make), but from yours it looks we removed the suite dependency. Changed
"dependency" to "prerequisite" to follow GNU Make terms.

> 
> > TEST_DEPS list was introduced to prevent <tarantool-tests> target from
> > being run in case no dependencies are changed. As a result of review it
> 
> Typo: s/result of review/result of the review/

Fixed.

> 
> > was decided to make all *-tests targets be .PHONY rules, hence TEST_DEPS
> 
> Typo? "be" looks excess here. I propose the following rewording

No, it's not, but changed to yours if it makes the sense clearer.

> | As a result of the review it was decided to turn all *-tests targets
> | into .PHONY rules, hence TEST_DEPS variable is excess and can be
> | dropped.

Here is the final wording:

================================================================================

test: remove excess prerequisite for tests target

TEST_DEPS list was introduced to prevent <tarantool-tests> target from
being run in case no dependencies are changed. As a result of the review
it was decided to turn all *-tests targets into .PHONY rules, hence
TEST_DEPS variable is excess and can be dropped.

Follows up tarantool/tarantool#4862

Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>
Signed-off-by: Igor Munkin <imun@tarantool.org>

================================================================================

> 
> > variable is excess and can be dropped.
> > 
> > Follows up tarantool/tarantool#4862
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  test/tarantool-tests/CMakeLists.txt | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index 9bb26f32..3e36ff86 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -69,7 +69,6 @@ set(LUA_PATH
> >  )
> >  set(LUA_TEST_SUFFIX .test.lua)
> 
> Nit: looks like `LUA_TEST_SUFFIX` variable is excess now, and we can
> move suffix ".test.lua" to `--ext` flag directly.

I don't think so. Considering your comment we need to drop every
variable that can be used in-place :)

> Feel free to ignore.

Ignoring.

> 

<snipped>

> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the fixes!

LGTM.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Sergey Ostanevich via Tarantool-patches]

Hi!

Thanks for the patch, it is LGTM.

Sergos

> On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> 
> TEST_DEPS list was introduced to prevent <tarantool-tests> target from
> being run in case no dependencies are changed. As a result of review it
> was decided to make all *-tests targets be .PHONY rules, hence TEST_DEPS
> variable is excess and can be dropped.
> 
> Follows up tarantool/tarantool#4862
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> test/tarantool-tests/CMakeLists.txt | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 9bb26f32..3e36ff86 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -69,7 +69,6 @@ set(LUA_PATH
> )
> set(LUA_TEST_SUFFIX .test.lua)
> set(LUA_TEST_FLAGS --failures --shuffle)
> -file(GLOB TEST_DEPS ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX})
> 
> if(CMAKE_VERBOSE_MAKEFILE)
>   list(APPEND LUA_TEST_FLAGS --verbose)
> @@ -78,7 +77,7 @@ endif()
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> # with dependecies are set in scope of BuildTestLib macro.
> add_custom_target(tarantool-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS}
> +  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
> )
> add_custom_command(TARGET tarantool-tests
>   COMMENT "Running Tarantool tests"
> -- 
> 2.25.0
> 

Re: [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target

[Igor Munkin via Tarantool-patches]

Sergos,

Thanks for your review!

On 06.04.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch, it is LGTM.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> Sergos
> 

<snipped>

> 

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

[Igor Munkin via Tarantool-patches]

Previous implementation of <utils.selfrun> implicitly obliges the
developer to pass the arguments to the test script for the second run.
This has been unnoticed, since the existing tests are invoked for two
different cases, so the script arguments were kinda obligatory, but they
are not required in a general case.

As a result the function signals the testing system that the script is
being run the second time via TEST_SELFRUN environment variable.

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
 .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
 test/tarantool-tests/utils.lua                |  7 +++
 3 files changed, 57 insertions(+), 52 deletions(-)

diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
index 56363608..64df5dbd 100644
--- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
+++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
@@ -1,31 +1,30 @@
-if #arg == 0 then
-
-  local utils = require('utils')
-
-  -- Disabled on *BSD due to #4819.
-  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
-
-  utils.selfrun(arg, {
-    {
-      arg = {
-        1, -- hotloop (arg[1])
-        1, -- trigger (arg[2])
-      },
-      msg = 'Trace is aborted',
-      res = tostring(3), -- hotloop + trigger + 1
-      test = 'is',
+local utils = require('utils')
+
+-- Disabled on *BSD due to #4819.
+utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
+
+utils.selfrun(arg, {
+  {
+    arg = {
+      1, -- hotloop (arg[1])
+      1, -- trigger (arg[2])
     },
-    {
-      arg = {
-        1, -- hotloop (arg[1])
-        2, -- trigger (arg[2])
-      },
-      msg = 'Trace is recorded',
-      res = 'Lua VM re%-entrancy is detected while executing the trace',
-      test = 'like',
+    msg = 'Trace is aborted',
+    res = tostring(3), -- hotloop + trigger + 1
+    test = 'is',
+  },
+  {
+    arg = {
+      1, -- hotloop (arg[1])
+      2, -- trigger (arg[2])
     },
-  })
-end
+    msg = 'Trace is recorded',
+    res = 'Lua VM re%-entrancy is detected while executing the trace',
+    test = 'like',
+  },
+})
+
+----- Test payload. ------------------------------------------------------------
 
 local cfg = {
   hotloop = arg[1] or 1,
diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
index 46fee533..edc5cf61 100644
--- a/test/tarantool-tests/lj-flush-on-trace.test.lua
+++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
@@ -1,31 +1,30 @@
-if #arg == 0 then
-
-  local utils = require('utils')
-
-  -- Disabled on *BSD due to #4819.
-  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
-
-  utils.selfrun(arg, {
-    {
-      arg = {
-        1, -- hotloop (arg[1])
-        1, -- trigger (arg[2])
-      },
-      msg = 'Trace is aborted',
-      res = 'OK',
-      test = 'is',
+local utils = require('utils')
+
+-- Disabled on *BSD due to #4819.
+utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
+
+utils.selfrun(arg, {
+  {
+    arg = {
+      1, -- hotloop (arg[1])
+      1, -- trigger (arg[2])
     },
-    {
-      arg = {
-        1, -- hotloop (arg[1])
-        2, -- trigger (arg[2])
-      },
-      msg = 'Trace is recorded',
-      res = 'JIT mode change is detected while executing the trace',
-      test = 'like',
+    msg = 'Trace is aborted',
+    res = 'OK',
+    test = 'is',
+  },
+  {
+    arg = {
+      1, -- hotloop (arg[1])
+      2, -- trigger (arg[2])
     },
-  })
-end
+    msg = 'Trace is recorded',
+    res = 'JIT mode change is detected while executing the trace',
+    test = 'like',
+  },
+})
+
+----- Test payload. ------------------------------------------------------------
 
 local cfg = {
   hotloop = arg[1] or 1,
diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index aebbf6ac..d2dd71b0 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -14,6 +14,12 @@ local function luacmd(args)
 end
 
 function M.selfrun(arg, checks)
+  -- If TEST_SELFRUN is set, just execute the test payload below
+  -- <selfrun> call, ...
+  if os.getenv('TEST_SELFRUN') then return end
+
+  -- ... otherwise run this chunk via <io.popen>.
+
   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
 
   test:plan(#checks)
@@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
   local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
                           'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
                           'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
+                          'TEST_SELFRUN=1' ..
                           '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
 
   for _, ch in pairs(checks) do
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the patch!

LGTM, except a few nits below.

Nit: I suggest to use `` for codenames mentions (if it is a function, then use
brackets, for example `function_name()`), and <> for filenames mentions
in the commit message and comments in the code.
This style has already been used for lua-Harness suite.
Thoughts?

On 05.04.21, Igor Munkin wrote:
> Previous implementation of <utils.selfrun> implicitly obliges the
> developer to pass the arguments to the test script for the second run.
> This has been unnoticed, since the existing tests are invoked for two
> different cases, so the script arguments were kinda obligatory, but they
> are not required in a general case.
> 
> As a result the function signals the testing system that the script is
> being run the second time via TEST_SELFRUN environment variable.
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
>  .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
>  test/tarantool-tests/utils.lua                |  7 +++
>  3 files changed, 57 insertions(+), 52 deletions(-)
> 
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index 56363608..64df5dbd 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -1,31 +1,30 @@

<skipped>

> +----- Test payload. ------------------------------------------------------------

Typo? comment length is too long, isn't it?
But, in other side, we already have the 80-symbols comments inside
<lj_memprof.c>...
But, we have no strict code style for LuaJIT part of the code. So:
Feel free to ignore.

>  
>  local cfg = {
>    hotloop = arg[1] or 1,
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index 46fee533..edc5cf61 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -1,31 +1,30 @@

<skipped>

> +----- Test payload. ------------------------------------------------------------

Ditto.

>  
>  local cfg = {
>    hotloop = arg[1] or 1,
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index aebbf6ac..d2dd71b0 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -14,6 +14,12 @@ local function luacmd(args)
>  end
>  
>  function M.selfrun(arg, checks)
> +  -- If TEST_SELFRUN is set, just execute the test payload below
> +  -- <selfrun> call, ...
> +  if os.getenv('TEST_SELFRUN') then return end

Typo? should we avoid inline return, shouldn't we?

> +
> +  -- ... otherwise run this chunk via <io.popen>.
> +
>    local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
>  
>    test:plan(#checks)
> @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
>    local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
>                            'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
>                            'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> +                          'TEST_SELFRUN=1' ..
>                            '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
>  
>    for _, ch in pairs(checks) do
> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 06.04.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the patch!
> 
> LGTM, except a few nits below.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> Nit: I suggest to use `` for codenames mentions (if it is a function, then use
> brackets, for example `function_name()`), and <> for filenames mentions
> in the commit message and comments in the code.

I personally don't like this style: it looks ugly at first. This is used
neither for rendering commit messages (even on GitHub web interface) nor
for doc related postprocessing, so I see no motivation to write
something like this.

> This style has already been used for lua-Harness suite.

I would like ask you not to use this notation there, but we had no words
regarding it in our Tarantool style guide, so I couldn't stop you from
writing commit messages the way you want. Finally, we have no guide for
LuaJIT sources and patches for now.

> Thoughts?

It would be nice to come to agreement about using the similar style in
writing plain text messages, but definitely not such ugly one.

> 
> On 05.04.21, Igor Munkin wrote:
> > Previous implementation of <utils.selfrun> implicitly obliges the
> > developer to pass the arguments to the test script for the second run.
> > This has been unnoticed, since the existing tests are invoked for two
> > different cases, so the script arguments were kinda obligatory, but they
> > are not required in a general case.
> > 
> > As a result the function signals the testing system that the script is
> > being run the second time via TEST_SELFRUN environment variable.
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
> >  .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
> >  test/tarantool-tests/utils.lua                |  7 +++
> >  3 files changed, 57 insertions(+), 52 deletions(-)
> > 
> > diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > index 56363608..64df5dbd 100644
> > --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > @@ -1,31 +1,30 @@
> 
> <skipped>
> 
> > +----- Test payload. ------------------------------------------------------------
> 
> Typo? comment length is too long, isn't it?

Well... I did it unintentionally: just typed 80a- in my text editor :)

> But, in other side, we already have the 80-symbols comments inside
> <lj_memprof.c>...
> But, we have no strict code style for LuaJIT part of the code. So:

Yes, we have not, but let's follow Tarantool guidelines for now. I left
the comment but stripped it up to 66 symbols in both chunks.

> Feel free to ignore.
> 
> >  
> >  local cfg = {
> >    hotloop = arg[1] or 1,

<snipped>

> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index aebbf6ac..d2dd71b0 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
> > @@ -14,6 +14,12 @@ local function luacmd(args)
> >  end
> >  
> >  function M.selfrun(arg, checks)
> > +  -- If TEST_SELFRUN is set, just execute the test payload below
> > +  -- <selfrun> call, ...
> > +  if os.getenv('TEST_SELFRUN') then return end
> 
> Typo? should we avoid inline return, shouldn't we?

No, it's not. There is the similar usage in <utils.skipcond>, and I just
follow the style. BTW, there are comments around this block, so I hope
this is not a problem for the reader.

> 
> > +
> > +  -- ... otherwise run this chunk via <io.popen>.
> > +
> >    local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> >  
> >    test:plan(#checks)
> > @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
> >    local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
> >                            'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
> >                            'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> > +                          'TEST_SELFRUN=1' ..
> >                            '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> >  
> >    for _, ch in pairs(checks) do
> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

[Sergey Ostanevich via Tarantool-patches]

Hi!

LGTM with some ignorable nits below.

Sergos


> On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> 
> Previous implementation of <utils.selfrun> implicitly obliges the
> developer to pass the arguments to the test script for the second run.
> This has been unnoticed, since the existing tests are invoked for two
> different cases, so the script arguments were kinda obligatory, but they
> are not required in a general case.
> 
> As a result the function signals the testing system that the script is
> being run the second time via TEST_SELFRUN environment variable.
> 

To me it looks like it was an infinite recursion, that was interrupted
only by chance in case a test does not tolerate the reentrance. 


> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
> .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
> test/tarantool-tests/utils.lua                |  7 +++
> 3 files changed, 57 insertions(+), 52 deletions(-)
> 
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index 56363608..64df5dbd 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -1,31 +1,30 @@
> -if #arg == 0 then
> -
> -  local utils = require('utils')
> -
> -  -- Disabled on *BSD due to #4819.
> -  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> -
> -  utils.selfrun(arg, {
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        1, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is aborted',
> -      res = tostring(3), -- hotloop + trigger + 1
> -      test = 'is',
> +local utils = require('utils')
> +
> +-- Disabled on *BSD due to #4819.
> +utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +
> +utils.selfrun(arg, {
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      1, -- trigger (arg[2])
>     },
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        2, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is recorded',
> -      res = 'Lua VM re%-entrancy is detected while executing the trace',
> -      test = 'like',
> +    msg = 'Trace is aborted',
> +    res = tostring(3), -- hotloop + trigger + 1
> +    test = 'is',
> +  },
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      2, -- trigger (arg[2])
>     },
> -  })
> -end
> +    msg = 'Trace is recorded',
> +    res = 'Lua VM re%-entrancy is detected while executing the trace',
> +    test = 'like',
> +  },
> +})
> +
> +----- Test payload. ------------------------------------------------------------
> 
> local cfg = {
>   hotloop = arg[1] or 1,
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index 46fee533..edc5cf61 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -1,31 +1,30 @@
> -if #arg == 0 then
> -
> -  local utils = require('utils')
> -
> -  -- Disabled on *BSD due to #4819.
> -  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> -
> -  utils.selfrun(arg, {
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        1, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is aborted',
> -      res = 'OK',
> -      test = 'is',
> +local utils = require('utils')
> +
> +-- Disabled on *BSD due to #4819.
> +utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +
> +utils.selfrun(arg, {
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      1, -- trigger (arg[2])
>     },
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        2, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is recorded',
> -      res = 'JIT mode change is detected while executing the trace',
> -      test = 'like',
> +    msg = 'Trace is aborted',
> +    res = 'OK',
> +    test = 'is',
> +  },
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      2, -- trigger (arg[2])
>     },
> -  })
> -end
> +    msg = 'Trace is recorded',
> +    res = 'JIT mode change is detected while executing the trace',
> +    test = 'like',
> +  },
> +})
> +
> +----- Test payload. ------------------------------------------------------------
> 
> local cfg = {
>   hotloop = arg[1] or 1,
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index aebbf6ac..d2dd71b0 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -14,6 +14,12 @@ local function luacmd(args)
> end
> 
> function M.selfrun(arg, checks)
> +  -- If TEST_SELFRUN is set, just execute the test payload below

To make a symmetry to the next comment: "we’re running the io.popen version
of the test, proceed with test payload execution past "

> +  -- <selfrun> call, ...
> +  if os.getenv('TEST_SELFRUN') then return end
> +
> +  -- ... otherwise run this chunk via <io.popen>.

I would add that the test payload won’t be run and the io.popen results
will be reported instead.

> +
>   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> 
>   test:plan(#checks)
> @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
>   local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
>                           'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
>                           'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> +                          'TEST_SELFRUN=1' ..
>                           '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> 
>   for _, ch in pairs(checks) do
> -- 
> 2.25.0
> 

Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

[Igor Munkin via Tarantool-patches]

Sergos,

Thanks for your review!

On 06.04.21, Sergey Ostanevich wrote:
> Hi!
> 
> LGTM with some ignorable nits below.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> Sergos
> 
> 
> > On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > Previous implementation of <utils.selfrun> implicitly obliges the
> > developer to pass the arguments to the test script for the second run.
> > This has been unnoticed, since the existing tests are invoked for two
> > different cases, so the script arguments were kinda obligatory, but they
> > are not required in a general case.
> > 
> > As a result the function signals the testing system that the script is
> > being run the second time via TEST_SELFRUN environment variable.
> > 
> 
> To me it looks like it was an infinite recursion, that was interrupted
> only by chance in case a test does not tolerate the reentrance. 

Precisely. Honestly, it was a "soft" requirement for such tests (as I
have written above), but considering your attempts with implementing
tests for JIT machinery, I've finally realized this problem.

> 
> 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
> > .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
> > test/tarantool-tests/utils.lua                |  7 +++
> > 3 files changed, 57 insertions(+), 52 deletions(-)

<snipped>

> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index aebbf6ac..d2dd71b0 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
> > @@ -14,6 +14,12 @@ local function luacmd(args)
> > end
> > 
> > function M.selfrun(arg, checks)
> > +  -- If TEST_SELFRUN is set, just execute the test payload below
> 
> To make a symmetry to the next comment: "we’re running the io.popen version
> of the test, proceed with test payload execution past "
> 
> > +  -- <selfrun> call, ...
> > +  if os.getenv('TEST_SELFRUN') then return end
> > +
> > +  -- ... otherwise run this chunk via <io.popen>.
> 
> I would add that the test payload won’t be run and the io.popen results
> will be reported instead.

I've adjusted the comments the following way:

================================================================================

diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index 9bdb71ec..c0403cf1 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -25,11 +25,15 @@ local function unshiftenv(variable, value, sep)
 end
 
 function M.selfrun(arg, checks)
-  -- If TEST_SELFRUN is set, just execute the test payload below
-  -- <selfrun> call, ...
+  -- If TEST_SELFRUN is set, it means the test has been run via
+  -- <io.popen>, so just return from this routine and proceed
+  -- the execution to the test payload, ...
   if os.getenv('TEST_SELFRUN') then return end
 
-  -- ... otherwise run this chunk via <io.popen>.
+  -- ... otherwise initialize <tap>, setup testing environment
+  -- and run this chunk via <io.popen> for each case in <checks>.
+  -- XXX: The function doesn't return back from this moment. It
+  -- checks whether all assertions are fine and exits.
 
   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
 

================================================================================

> 
> > +
> >   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> > 
> >   test:plan(#checks)
> > @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
> >   local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
> >                           'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
> >                           'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> > +                          'TEST_SELFRUN=1' ..
> >                           '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> > 
> >   for _, ch in pairs(checks) do
> > -- 
> > 2.25.0
> > 
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

Great, thanks for explanation and comments update!

Sergos

> On 6 Apr 2021, at 21:32, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergos,
> 
> Thanks for your review!
> 
> On 06.04.21, Sergey Ostanevich wrote:
>> Hi!
>> 
>> LGTM with some ignorable nits below.
> 
> Added your tag:
> | Reviewed-by: Sergey Ostanevich <sergos@tarantool.org <mailto:sergos@tarantool.org>>
> 
>> 
>> Sergos
>> 
>> 
>>> On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
>>> 
>>> Previous implementation of <utils.selfrun> implicitly obliges the
>>> developer to pass the arguments to the test script for the second run.
>>> This has been unnoticed, since the existing tests are invoked for two
>>> different cases, so the script arguments were kinda obligatory, but they
>>> are not required in a general case.
>>> 
>>> As a result the function signals the testing system that the script is
>>> being run the second time via TEST_SELFRUN environment variable.
>>> 
>> 
>> To me it looks like it was an infinite recursion, that was interrupted
>> only by chance in case a test does not tolerate the reentrance. 
> 
> Precisely. Honestly, it was a "soft" requirement for such tests (as I
> have written above), but considering your attempts with implementing
> tests for JIT machinery, I've finally realized this problem.
> 
>> 
>> 
>>> Signed-off-by: Igor Munkin <imun@tarantool.org <mailto:imun@tarantool.org>>
>>> ---
>>> .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
>>> .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
>>> test/tarantool-tests/utils.lua                |  7 +++
>>> 3 files changed, 57 insertions(+), 52 deletions(-)
> 
> <snipped>
> 
>>> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
>>> index aebbf6ac..d2dd71b0 100644
>>> --- a/test/tarantool-tests/utils.lua
>>> +++ b/test/tarantool-tests/utils.lua
>>> @@ -14,6 +14,12 @@ local function luacmd(args)
>>> end
>>> 
>>> function M.selfrun(arg, checks)
>>> +  -- If TEST_SELFRUN is set, just execute the test payload below
>> 
>> To make a symmetry to the next comment: "we’re running the io.popen version
>> of the test, proceed with test payload execution past "
>> 
>>> +  -- <selfrun> call, ...
>>> +  if os.getenv('TEST_SELFRUN') then return end
>>> +
>>> +  -- ... otherwise run this chunk via <io.popen>.
>> 
>> I would add that the test payload won’t be run and the io.popen results
>> will be reported instead.
> 
> I've adjusted the comments the following way:
> 
> ================================================================================
> 
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index 9bdb71ec..c0403cf1 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -25,11 +25,15 @@ local function unshiftenv(variable, value, sep)
> end
> 
> function M.selfrun(arg, checks)
> -  -- If TEST_SELFRUN is set, just execute the test payload below
> -  -- <selfrun> call, ...
> +  -- If TEST_SELFRUN is set, it means the test has been run via
> +  -- <io.popen>, so just return from this routine and proceed
> +  -- the execution to the test payload, ...
>   if os.getenv('TEST_SELFRUN') then return end
> 
> -  -- ... otherwise run this chunk via <io.popen>.
> +  -- ... otherwise initialize <tap>, setup testing environment
> +  -- and run this chunk via <io.popen> for each case in <checks>.
> +  -- XXX: The function doesn't return back from this moment. It
> +  -- checks whether all assertions are fine and exits.
> 
>   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> 
> 
> ================================================================================
> 
>> 
>>> +
>>>  local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
>>> 
>>>  test:plan(#checks)
>>> @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
>>>  local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
>>>                          'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
>>>                          'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
>>> +                          'TEST_SELFRUN=1' ..
>>>                          '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
>>> 
>>>  for _, ch in pairs(checks) do
>>> -- 
>>> 2.25.0
>>> 
>> 
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 39891 bytes --]

[Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS

[Igor Munkin via Tarantool-patches]

This patch resolves the issue with running the tests with auxiliary
dynamically loaded modules in case of out-of-source build.

The first issue relates to the way modules loaded at runtime are built
on MacOS. Since the auxiliary libraries are built as a dynamically
loaded modules on MacOS instead of shared libraries as it is done on
Linux and BSD, another environment variable should be used to guide
<ffi.load> while searching the extension. Hence the values collected in
scope of <BuildTestCLib> macro need to be set to DYLD_LIBRARY_PATH
variable instead of LD_LIBRARY_PATH on MacOS.

Unfortunately, this rather small change doesn't resolve the problem at
all and the root cause lies much deeper than it seems at the beginning.

Apple tries their best to "protect their users from malicious software".
As a result SIP[1] has been designed and released. Now, Apple developers
are *so protected*, that they can load nothing being not installed in
the system, since some programs sanitize the environment before they
start child processes. Specifically, environment variables starting with
DYLD_ and LD_ are unset for child process started by system programs[2].

That which does not kill us makes us stronger: fortunately, these
environment variables are used by FFI machinery to find the proper
shared library, hence we can still tweak testing environment before
calling <ffi.load>. However, the value can't be passed via the standard
environment variable, so we prepend TEST_ prefix to its name to get
around SIP magic tricks. Finally, to set the variable required by FFI
machinery the introduced <utils.tweakenv> routine is used. PROFIT!
Your move, Cupertino geniuses.

[1]: https://support.apple.com/en-us/HT204899
[2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html

Resolves tarantool/tarantool#5959
Follows up tarantool/tarantool#4862

Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
Co-authored-by: Mons Anderson <mons@cpan.org>
Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/tarantool-tests/CMakeLists.txt           | 39 +++++++++++++++++--
 .../gh-4427-ffi-sandwich.test.lua             |  4 ++
 .../lj-flush-on-trace.test.lua                |  4 ++
 test/tarantool-tests/utils.lua                | 39 ++++++++++++++++---
 4 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 3e36ff86..c793ad60 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -69,11 +69,46 @@ set(LUA_PATH
 )
 set(LUA_TEST_SUFFIX .test.lua)
 set(LUA_TEST_FLAGS --failures --shuffle)
+set(LUA_TEST_ENV
+  "LUA_PATH=\"${LUA_PATH}\;\;\""
+  "LUA_CPATH=\"${LUA_CPATH}\;\;\""
+)
 
 if(CMAKE_VERBOSE_MAKEFILE)
   list(APPEND LUA_TEST_FLAGS --verbose)
 endif()
 
+# XXX: Since the auxiliary libraries are built as a dynamically
+# loaded modules on MacOS instead of shared libraries as it is
+# done on Linux and BSD, another environment variable should be
+# used to guide <ffi.load> while searching the extension.
+if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
+  # XXX: Apple tries their best to "protect their users from
+  # malware". As a result SIP (see the link[1] below) has been
+  # designed and released. Now, Apple developers are so protected,
+  # that they can load nothing being not installed in the system,
+  # since some programs sanitize the environment before they start
+  # child processes. Specifically, environment variables starting
+  # with DYLD_ and LD_ are unset for child process started by
+  # system programs (like /usr/bin/env used for preparing testing
+  # environment). For more info, see the docs[2] below.
+  #
+  # That which does not kill us makes us stronger: fortunately,
+  # these environment variables are used by FFI machinery to find
+  # the proper shared library, hence we can still tweak testing
+  # environment before calling <ffi.load>. However, the value
+  # can't be passed via the standard environment variable, so we
+  # prepend TEST_ prefix to its name to get around SIP magic
+  # tricks. Finally, to set the variable required by FFI machinery
+  # the introduced <utils.tweakenv> routine is used.
+  #
+  # [1]: https://support.apple.com/en-us/HT204899
+  # [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
+  list(APPEND LUA_TEST_ENV TEST_DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
+else()
+  list(APPEND LUA_TEST_ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
+endif()
+
 # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
 # with dependecies are set in scope of BuildTestLib macro.
 add_custom_target(tarantool-tests
@@ -83,9 +118,7 @@ add_custom_command(TARGET tarantool-tests
   COMMENT "Running Tarantool tests"
   COMMAND
   env
-    LUA_PATH="${LUA_PATH}\;\;"
-    LUA_CPATH="${LUA_CPATH}\;\;"
-    LD_LIBRARY_PATH="${LD_LIBRARY_PATH}"
+    ${LUA_TEST_ENV}
     ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
       --exec '${LUAJIT_TEST_COMMAND}'
       --ext ${LUA_TEST_SUFFIX}
diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
index 64df5dbd..651dc3f4 100644
--- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
+++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
@@ -3,6 +3,10 @@ local utils = require('utils')
 -- Disabled on *BSD due to #4819.
 utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 
+-- XXX: Tweak the process environment to get around SIP.
+-- See the comment in suite CMakeLists.txt for more info.
+utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
+
 utils.selfrun(arg, {
   {
     arg = {
diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
index edc5cf61..1ad4f832 100644
--- a/test/tarantool-tests/lj-flush-on-trace.test.lua
+++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
@@ -3,6 +3,10 @@ local utils = require('utils')
 -- Disabled on *BSD due to #4819.
 utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 
+-- XXX: Tweak the process environment to get around SIP.
+-- See the comment in suite CMakeLists.txt for more info.
+utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
+
 utils.selfrun(arg, {
   {
     arg = {
diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index d2dd71b0..9bdb71ec 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -1,7 +1,12 @@
 local M = {}
 
+local ffi = require('ffi')
 local tap = require('tap')
 
+ffi.cdef([[
+  int setenv(const char *name, const char *value, int overwrite);
+]])
+
 local function luacmd(args)
   -- arg[-1] is guaranteed to be not nil.
   local idx = -2
@@ -13,6 +18,12 @@ local function luacmd(args)
   return table.concat(args, ' ', idx + 1, -1)
 end
 
+local function unshiftenv(variable, value, sep)
+  local envvar = os.getenv(variable)
+  return ('%s="%s%s"'):format(variable, value,
+                              envvar and ('%s%s'):format(sep, envvar) or '')
+end
+
 function M.selfrun(arg, checks)
   -- If TEST_SELFRUN is set, just execute the test payload below
   -- <selfrun> call, ...
@@ -24,18 +35,22 @@ function M.selfrun(arg, checks)
 
   test:plan(#checks)
 
+  local libext = package.cpath:match('?.(%a+);')
   local vars = {
     LUABIN = luacmd(arg),
     SCRIPT = arg[0],
     PATH   = arg[0]:gsub('%.test%.lua', ''),
-    SUFFIX = package.cpath:match('?.(%a+);'),
+    SUFFIX = libext,
+    ENV = table.concat({
+      unshiftenv('LUA_PATH', '<PATH>/?.lua', ';'),
+      unshiftenv('LUA_CPATH', '<PATH>/?.<SUFFIX>', ';'),
+      unshiftenv((libext == 'dylib' and 'DYLD' or 'LD') .. '_LIBRARY_PATH',
+                 '<PATH>', ':'),
+      'TEST_SELFRUN=1',
+    }, ' '),
   }
 
-  local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
-                          'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
-                          'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
-                          'TEST_SELFRUN=1' ..
-                          '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
+  local cmd = string.gsub('<ENV> <LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
 
   for _, ch in pairs(checks) do
     local testf = test[ch.test]
@@ -58,4 +73,16 @@ function M.skipcond(condition, message)
   os.exit(test:check() and 0 or 1)
 end
 
+function M.tweakenv(condition, variable)
+  if not condition or os.getenv(variable) then return end
+  local testvar = assert(os.getenv('TEST_' .. variable),
+                         ('Neither %s nor auxiliary TEST_%s variables are set')
+                         :format(variable, variable))
+  -- XXX: The third argument of setenv(3) is set to zero to forbid
+  -- overwriting the <variable>. Since there is the check above
+  -- whether this <variable> is set in the process environment, it
+  -- just makes this solution foolproof.
+  ffi.C.setenv(variable, testvar, 0)
+end
+
 return M
-- 
2.25.0

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS

[Sergey Ostanevich via Tarantool-patches]

Hi!

Thanks for the patch!

Couple of nits, LGTM.

regards,
Sergos


> On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> 
> This patch resolves the issue with running the tests with auxiliary
> dynamically loaded modules in case of out-of-source build.
> 
I wonder how it works for an in-source one… (just a comment)

> The first issue relates to the way modules loaded at runtime are built
					    ^ to be
> on MacOS. Since the auxiliary libraries are built as a dynamically
> loaded modules on MacOS instead of shared libraries as it is done on
> Linux and BSD, another environment variable should be used to guide
> <ffi.load> while searching the extension. Hence the values collected in

This might overlap: ffi should use ‘LUA_CPATH’, while it is the dynamic 
loader and dlopen() interface of it that relies on {DY}LD_ set of variables. 

> scope of <BuildTestCLib> macro need to be set to DYLD_LIBRARY_PATH
> variable instead of LD_LIBRARY_PATH on MacOS.
> 
> Unfortunately, this rather small change doesn't resolve the problem at
> all and the root cause lies much deeper than it seems at the beginning.
> 
> Apple tries their best to "protect their users from malicious software".
> As a result SIP[1] has been designed and released. Now, Apple developers
> are *so protected*, that they can load nothing being not installed in
> the system, since some programs sanitize the environment before they
> start child processes. Specifically, environment variables starting with
> DYLD_ and LD_ are unset for child process started by system programs[2].
> 
> That which does not kill us makes us stronger: fortunately, these
> environment variables are used by FFI machinery to find the proper
> shared library, hence we can still tweak testing environment before
> calling <ffi.load>. However, the value can't be passed via the standard
> environment variable, so we prepend TEST_ prefix to its name to get
> around SIP magic tricks. Finally, to set the variable required by FFI
> machinery the introduced <utils.tweakenv> routine is used. PROFIT!
> Your move, Cupertino geniuses.
> 
> [1]: https://support.apple.com/en-us/HT204899
> [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> 
> Resolves tarantool/tarantool#5959
> Follows up tarantool/tarantool#4862
> 
> Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
> Co-authored-by: Mons Anderson <mons@cpan.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> test/tarantool-tests/CMakeLists.txt           | 39 +++++++++++++++++--
> .../gh-4427-ffi-sandwich.test.lua             |  4 ++
> .../lj-flush-on-trace.test.lua                |  4 ++
> test/tarantool-tests/utils.lua                | 39 ++++++++++++++++---
> 4 files changed, 77 insertions(+), 9 deletions(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 3e36ff86..c793ad60 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -69,11 +69,46 @@ set(LUA_PATH
> )
> set(LUA_TEST_SUFFIX .test.lua)
> set(LUA_TEST_FLAGS --failures --shuffle)
> +set(LUA_TEST_ENV
> +  "LUA_PATH=\"${LUA_PATH}\;\;\""
> +  "LUA_CPATH=\"${LUA_CPATH}\;\;\""
> +)
> 
> if(CMAKE_VERBOSE_MAKEFILE)
>   list(APPEND LUA_TEST_FLAGS --verbose)
> endif()
> 
> +# XXX: Since the auxiliary libraries are built as a dynamically
> +# loaded modules on MacOS instead of shared libraries as it is
> +# done on Linux and BSD, another environment variable should be
> +# used to guide <ffi.load> while searching the extension.
> +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +  # XXX: Apple tries their best to "protect their users from
> +  # malware". As a result SIP (see the link[1] below) has been
> +  # designed and released. Now, Apple developers are so protected,
> +  # that they can load nothing being not installed in the system,
> +  # since some programs sanitize the environment before they start
> +  # child processes. Specifically, environment variables starting
> +  # with DYLD_ and LD_ are unset for child process started by
> +  # system programs (like /usr/bin/env used for preparing testing
> +  # environment). For more info, see the docs[2] below.
> +  #
> +  # That which does not kill us makes us stronger: fortunately,
> +  # these environment variables are used by FFI machinery to find
> +  # the proper shared library, hence we can still tweak testing
> +  # environment before calling <ffi.load>. However, the value
> +  # can't be passed via the standard environment variable, so we
> +  # prepend TEST_ prefix to its name to get around SIP magic
> +  # tricks. Finally, to set the variable required by FFI machinery
> +  # the introduced <utils.tweakenv> routine is used.
> +  #
> +  # [1]: https://support.apple.com/en-us/HT204899
> +  # [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> +  list(APPEND LUA_TEST_ENV TEST_DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +else()
> +  list(APPEND LUA_TEST_ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +endif()
> +
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> # with dependecies are set in scope of BuildTestLib macro.
> add_custom_target(tarantool-tests
> @@ -83,9 +118,7 @@ add_custom_command(TARGET tarantool-tests
>   COMMENT "Running Tarantool tests"
>   COMMAND
>   env
> -    LUA_PATH="${LUA_PATH}\;\;"
> -    LUA_CPATH="${LUA_CPATH}\;\;"
> -    LD_LIBRARY_PATH="${LD_LIBRARY_PATH}"
> +    ${LUA_TEST_ENV}
>     ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
>       --exec '${LUAJIT_TEST_COMMAND}'
>       --ext ${LUA_TEST_SUFFIX}
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index 64df5dbd..651dc3f4 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -3,6 +3,10 @@ local utils = require('utils')
> -- Disabled on *BSD due to #4819.
> utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> 
> +-- XXX: Tweak the process environment to get around SIP.
> +-- See the comment in suite CMakeLists.txt for more info.
> +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> +
> utils.selfrun(arg, {
>   {
>     arg = {
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index edc5cf61..1ad4f832 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -3,6 +3,10 @@ local utils = require('utils')
> -- Disabled on *BSD due to #4819.
> utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> 
> +-- XXX: Tweak the process environment to get around SIP.
> +-- See the comment in suite CMakeLists.txt for more info.
> +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> +
> utils.selfrun(arg, {
>   {
>     arg = {
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index d2dd71b0..9bdb71ec 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -1,7 +1,12 @@
> local M = {}
> 
> +local ffi = require('ffi')
> local tap = require('tap')
> 
> +ffi.cdef([[
> +  int setenv(const char *name, const char *value, int overwrite);
> +]])
> +

Why? os.setenv() didn’t work for some reason? Comment it and I’m fine.

tarantool> os.setenv("a","b")
---
...

tarantool> os.getenv("a")
---
- b
...

tarantool> os.execute('echo $a')
b
---

> local function luacmd(args)
>   -- arg[-1] is guaranteed to be not nil.
>   local idx = -2
> @@ -13,6 +18,12 @@ local function luacmd(args)
>   return table.concat(args, ' ', idx + 1, -1)
> end
> 
> +local function unshiftenv(variable, value, sep)
> +  local envvar = os.getenv(variable)
> +  return ('%s="%s%s"'):format(variable, value,
> +                              envvar and ('%s%s'):format(sep, envvar) or '')
> +end
> +
> function M.selfrun(arg, checks)
>   -- If TEST_SELFRUN is set, just execute the test payload below
>   -- <selfrun> call, ...
> @@ -24,18 +35,22 @@ function M.selfrun(arg, checks)
> 
>   test:plan(#checks)
> 
> +  local libext = package.cpath:match('?.(%a+);')
>   local vars = {
>     LUABIN = luacmd(arg),
>     SCRIPT = arg[0],
>     PATH   = arg[0]:gsub('%.test%.lua', ''),
> -    SUFFIX = package.cpath:match('?.(%a+);'),
> +    SUFFIX = libext,
> +    ENV = table.concat({
> +      unshiftenv('LUA_PATH', '<PATH>/?.lua', ';'),
> +      unshiftenv('LUA_CPATH', '<PATH>/?.<SUFFIX>', ';'),
> +      unshiftenv((libext == 'dylib' and 'DYLD' or 'LD') .. '_LIBRARY_PATH',
> +                 '<PATH>', ':'),
> +      'TEST_SELFRUN=1',
> +    }, ' '),
>   }
> 
> -  local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
> -                          'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
> -                          'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> -                          'TEST_SELFRUN=1' ..
> -                          '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> +  local cmd = string.gsub('<ENV> <LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> 
>   for _, ch in pairs(checks) do
>     local testf = test[ch.test]
> @@ -58,4 +73,16 @@ function M.skipcond(condition, message)
>   os.exit(test:check() and 0 or 1)
> end
> 
> +function M.tweakenv(condition, variable)
> +  if not condition or os.getenv(variable) then return end
> +  local testvar = assert(os.getenv('TEST_' .. variable),
> +                         ('Neither %s nor auxiliary TEST_%s variables are set')
> +                         :format(variable, variable))
> +  -- XXX: The third argument of setenv(3) is set to zero to forbid
> +  -- overwriting the <variable>. Since there is the check above
> +  -- whether this <variable> is set in the process environment, it
> +  -- just makes this solution foolproof.

So, if you check the presence there’s no need in third argument and os.setenv()
can be used? This can be done instead of comment above - and I’m fine. 

> +  ffi.C.setenv(variable, testvar, 0)
> +end
> +
> return M
> -- 
> 2.25.0
> 

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS

[Igor Munkin via Tarantool-patches]

Sergos,

Thanks for your review!

On 06.04.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> Couple of nits, LGTM.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> regards,
> Sergos
> 
> 
> > On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > This patch resolves the issue with running the tests with auxiliary
> > dynamically loaded modules in case of out-of-source build.
> > 
> I wonder how it works for an in-source one… (just a comment)

There is no magic: the paths are adjusted within <utils.selfrun> to make
in-source testing (e.g. while developing) easier.

> 
> > The first issue relates to the way modules loaded at runtime are built
> 					    ^ to be

Thanks, fixed.

> > on MacOS. Since the auxiliary libraries are built as a dynamically
> > loaded modules on MacOS instead of shared libraries as it is done on
> > Linux and BSD, another environment variable should be used to guide
> > <ffi.load> while searching the extension. Hence the values collected in
> 
> This might overlap: ffi should use ‘LUA_CPATH’, while it is the dynamic 
> loader and dlopen() interface of it that relies on {DY}LD_ set of variables. 

No, it shouldn't. Despite the fact both native shared libraries and ones
implemented via Lua C API are loaded dynamically, they have different
loading principles. Hence, since FFI use the native libraries, it ought
to consider {DY}LD_ set of variables. LUA_CPATH on its turn provides the
paths to the Lua modules loaded via <require> and has another format and
ergo requirements for the libraries (e.g. an obligatory external
luaopen_<module> function).

> 
> > scope of <BuildTestCLib> macro need to be set to DYLD_LIBRARY_PATH
> > variable instead of LD_LIBRARY_PATH on MacOS.
> > 
> > Unfortunately, this rather small change doesn't resolve the problem at
> > all and the root cause lies much deeper than it seems at the beginning.
> > 
> > Apple tries their best to "protect their users from malicious software".
> > As a result SIP[1] has been designed and released. Now, Apple developers
> > are *so protected*, that they can load nothing being not installed in
> > the system, since some programs sanitize the environment before they
> > start child processes. Specifically, environment variables starting with
> > DYLD_ and LD_ are unset for child process started by system programs[2].
> > 
> > That which does not kill us makes us stronger: fortunately, these
> > environment variables are used by FFI machinery to find the proper
> > shared library, hence we can still tweak testing environment before
> > calling <ffi.load>. However, the value can't be passed via the standard
> > environment variable, so we prepend TEST_ prefix to its name to get
> > around SIP magic tricks. Finally, to set the variable required by FFI
> > machinery the introduced <utils.tweakenv> routine is used. PROFIT!
> > Your move, Cupertino geniuses.
> > 
> > [1]: https://support.apple.com/en-us/HT204899
> > [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> > 
> > Resolves tarantool/tarantool#5959
> > Follows up tarantool/tarantool#4862
> > 
> > Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
> > Co-authored-by: Mons Anderson <mons@cpan.org>
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > test/tarantool-tests/CMakeLists.txt           | 39 +++++++++++++++++--
> > .../gh-4427-ffi-sandwich.test.lua             |  4 ++
> > .../lj-flush-on-trace.test.lua                |  4 ++
> > test/tarantool-tests/utils.lua                | 39 ++++++++++++++++---
> > 4 files changed, 77 insertions(+), 9 deletions(-)
> > 

<snipped>

> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index d2dd71b0..9bdb71ec 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
> > @@ -1,7 +1,12 @@
> > local M = {}
> > 
> > +local ffi = require('ffi')
> > local tap = require('tap')
> > 
> > +ffi.cdef([[
> > +  int setenv(const char *name, const char *value, int overwrite);
> > +]])
> > +
> 
> Why? os.setenv() didn’t work for some reason? Comment it and I’m fine.

Oh, yeah! Sorry, but I am so glad you have faced this by yourself and
produce this precedent! I finally have a good argument for pushing
#4577[1]. BTW, now I believe we need the similar ticket for Lua
interfaces too.

Back to the business: so, you are asking why I didn't use <os.setenv> in
LuaJIT tests? The answer is quite simple: because there is no such
function in Lua! But those guys, who decided to spoil the standard Lua
namespace, believed it is a good idea to store their new functions
alongside with the ones provided by Lua standard library. This is why I
am against using the Lua namespaces for platform-defined functions and
we have introduced <misc> for the new extensions.

So, unfortunately, this can't be done in other way without more efforts.

> 
> tarantool> os.setenv("a","b")
> ---
> ...
> 
> tarantool> os.getenv("a")
> ---
> - b
> ...
> 
> tarantool> os.execute('echo $a')
> b
> ---
> 
> > local function luacmd(args)
> >   -- arg[-1] is guaranteed to be not nil.
> >   local idx = -2
> > @@ -13,6 +18,12 @@ local function luacmd(args)
> >   return table.concat(args, ' ', idx + 1, -1)
> > end
> > 
> > +local function unshiftenv(variable, value, sep)
> > +  local envvar = os.getenv(variable)
> > +  return ('%s="%s%s"'):format(variable, value,
> > +                              envvar and ('%s%s'):format(sep, envvar) or '')
> > +end
> > +
> > function M.selfrun(arg, checks)
> >   -- If TEST_SELFRUN is set, just execute the test payload below
> >   -- <selfrun> call, ...
> > @@ -24,18 +35,22 @@ function M.selfrun(arg, checks)
> > 
> >   test:plan(#checks)
> > 
> > +  local libext = package.cpath:match('?.(%a+);')
> >   local vars = {
> >     LUABIN = luacmd(arg),
> >     SCRIPT = arg[0],
> >     PATH   = arg[0]:gsub('%.test%.lua', ''),
> > -    SUFFIX = package.cpath:match('?.(%a+);'),
> > +    SUFFIX = libext,
> > +    ENV = table.concat({
> > +      unshiftenv('LUA_PATH', '<PATH>/?.lua', ';'),
> > +      unshiftenv('LUA_CPATH', '<PATH>/?.<SUFFIX>', ';'),
> > +      unshiftenv((libext == 'dylib' and 'DYLD' or 'LD') .. '_LIBRARY_PATH',
> > +                 '<PATH>', ':'),
> > +      'TEST_SELFRUN=1',
> > +    }, ' '),
> >   }
> > 
> > -  local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
> > -                          'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
> > -                          'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> > -                          'TEST_SELFRUN=1' ..
> > -                          '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> > +  local cmd = string.gsub('<ENV> <LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> > 
> >   for _, ch in pairs(checks) do
> >     local testf = test[ch.test]
> > @@ -58,4 +73,16 @@ function M.skipcond(condition, message)
> >   os.exit(test:check() and 0 or 1)
> > end
> > 
> > +function M.tweakenv(condition, variable)
> > +  if not condition or os.getenv(variable) then return end
> > +  local testvar = assert(os.getenv('TEST_' .. variable),
> > +                         ('Neither %s nor auxiliary TEST_%s variables are set')
> > +                         :format(variable, variable))
> > +  -- XXX: The third argument of setenv(3) is set to zero to forbid
> > +  -- overwriting the <variable>. Since there is the check above
> > +  -- whether this <variable> is set in the process environment, it
> > +  -- just makes this solution foolproof.
> 
> So, if you check the presence there’s no need in third argument and os.setenv()
> can be used? This can be done instead of comment above - and I’m fine. 

No, this doesn't block me from using <os.setenv>, but rather allows to
tweak the process environment via setenv(3) in a more safe way.

> 
> > +  ffi.C.setenv(variable, testvar, 0)
> > +end
> > +
> > return M
> > -- 
> > 2.25.0
> > 
> 

[1]: https://github.com/tarantool/tarantool/issues/4577

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the patch!

LGTM, except a few nitpicks below.

On 05.04.21, Igor Munkin wrote:
> This patch resolves the issue with running the tests with auxiliary
> dynamically loaded modules in case of out-of-source build.
> 
> The first issue relates to the way modules loaded at runtime are built
> on MacOS. Since the auxiliary libraries are built as a dynamically
> loaded modules on MacOS instead of shared libraries as it is done on
> Linux and BSD, another environment variable should be used to guide
> <ffi.load> while searching the extension. Hence the values collected in
> scope of <BuildTestCLib> macro need to be set to DYLD_LIBRARY_PATH
> variable instead of LD_LIBRARY_PATH on MacOS.
> 
> Unfortunately, this rather small change doesn't resolve the problem at
> all and the root cause lies much deeper than it seems at the beginning.

This paragraph looks uninformative, looks like you can just use
"Moreover, " for the next sentence instead.

> 
> Apple tries their best to "protect their users from malicious software".
> As a result SIP[1] has been designed and released. Now, Apple developers
> are *so protected*, that they can load nothing being not installed in
> the system, since some programs sanitize the environment before they
> start child processes. Specifically, environment variables starting with
> DYLD_ and LD_ are unset for child process started by system programs[2].

Sorry, but this paragraph is too wordy. I propose to reformulate it like
the following:

| Moreover, Apple tries their best to "protect their users from malicious
| software". As a result, SIP[1] has been designed and released. As a part
| of it, environment variables starting with DYLD_ and LD_ are unset
| for child process started by system programs[2].

Feel free to ignore.

> 
> That which does not kill us makes us stronger: fortunately, these

I suppose you can just drop this preamble.

| These environment variables are used by FFI machinery to find the proper
| shared library, hence we can still tweak testing environment before
| calling <ffi.load>.

> environment variables are used by FFI machinery to find the proper
> shared library, hence we can still tweak testing environment before
> calling <ffi.load>. However, the value can't be passed via the standard
> environment variable, so we prepend TEST_ prefix to its name to get
> around SIP magic tricks. Finally, to set the variable required by FFI
> machinery the introduced <utils.tweakenv> routine is used. PROFIT!
> Your move, Cupertino geniuses.

The last two sentences are redundant. Remove them please.

> 
> [1]: https://support.apple.com/en-us/HT204899
> [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> 
> Resolves tarantool/tarantool#5959
> Follows up tarantool/tarantool#4862
> 
> Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
> Co-authored-by: Mons Anderson <mons@cpan.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/tarantool-tests/CMakeLists.txt           | 39 +++++++++++++++++--
>  .../gh-4427-ffi-sandwich.test.lua             |  4 ++
>  .../lj-flush-on-trace.test.lua                |  4 ++
>  test/tarantool-tests/utils.lua                | 39 ++++++++++++++++---
>  4 files changed, 77 insertions(+), 9 deletions(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 3e36ff86..c793ad60 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -69,11 +69,46 @@ set(LUA_PATH
>  )
>  set(LUA_TEST_SUFFIX .test.lua)
>  set(LUA_TEST_FLAGS --failures --shuffle)
> +set(LUA_TEST_ENV
> +  "LUA_PATH=\"${LUA_PATH}\;\;\""
> +  "LUA_CPATH=\"${LUA_CPATH}\;\;\""
> +)
>  
>  if(CMAKE_VERBOSE_MAKEFILE)
>    list(APPEND LUA_TEST_FLAGS --verbose)
>  endif()
>  
> +# XXX: Since the auxiliary libraries are built as a dynamically
> +# loaded modules on MacOS instead of shared libraries as it is
> +# done on Linux and BSD, another environment variable should be
> +# used to guide <ffi.load> while searching the extension.
> +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +  # XXX: Apple tries their best to "protect their users from
> +  # malware". As a result SIP (see the link[1] below) has been
> +  # designed and released. Now, Apple developers are so protected,
> +  # that they can load nothing being not installed in the system,
> +  # since some programs sanitize the environment before they start
> +  # child processes. Specifically, environment variables starting
> +  # with DYLD_ and LD_ are unset for child process started by
> +  # system programs (like /usr/bin/env used for preparing testing
> +  # environment). For more info, see the docs[2] below.
> +  #
> +  # That which does not kill us makes us stronger: fortunately,
> +  # these environment variables are used by FFI machinery to find
> +  # the proper shared library, hence we can still tweak testing
> +  # environment before calling <ffi.load>. However, the value
> +  # can't be passed via the standard environment variable, so we
> +  # prepend TEST_ prefix to its name to get around SIP magic
> +  # tricks. Finally, to set the variable required by FFI machinery
> +  # the introduced <utils.tweakenv> routine is used.

Same comments as for the commit message.

> +  #
> +  # [1]: https://support.apple.com/en-us/HT204899
> +  # [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> +  list(APPEND LUA_TEST_ENV TEST_DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +else()
> +  list(APPEND LUA_TEST_ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +endif()
> +
>  # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
>  # with dependecies are set in scope of BuildTestLib macro.
>  add_custom_target(tarantool-tests
> @@ -83,9 +118,7 @@ add_custom_command(TARGET tarantool-tests
>    COMMENT "Running Tarantool tests"
>    COMMAND
>    env
> -    LUA_PATH="${LUA_PATH}\;\;"
> -    LUA_CPATH="${LUA_CPATH}\;\;"
> -    LD_LIBRARY_PATH="${LD_LIBRARY_PATH}"
> +    ${LUA_TEST_ENV}
>      ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
>        --exec '${LUAJIT_TEST_COMMAND}'
>        --ext ${LUA_TEST_SUFFIX}
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index 64df5dbd..651dc3f4 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -3,6 +3,10 @@ local utils = require('utils')
>  -- Disabled on *BSD due to #4819.
>  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>  
> +-- XXX: Tweak the process environment to get around SIP.
> +-- See the comment in suite CMakeLists.txt for more info.
> +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> +
>  utils.selfrun(arg, {
>    {
>      arg = {
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index edc5cf61..1ad4f832 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -3,6 +3,10 @@ local utils = require('utils')
>  -- Disabled on *BSD due to #4819.
>  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>  
> +-- XXX: Tweak the process environment to get around SIP.
> +-- See the comment in suite CMakeLists.txt for more info.
> +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> +
>  utils.selfrun(arg, {
>    {
>      arg = {
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index d2dd71b0..9bdb71ec 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -1,7 +1,12 @@
>  local M = {}
>  
> +local ffi = require('ffi')
>  local tap = require('tap')
>  
> +ffi.cdef([[
> +  int setenv(const char *name, const char *value, int overwrite);
> +]])

Side note: looks like we need to move `os.setenv()` fucntion from the
Tarantool's built-ins onboard as `misc.setenv()`.

> +

<skipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 07.04.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the patch!
> 
> LGTM, except a few nitpicks below.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> On 05.04.21, Igor Munkin wrote:
> > This patch resolves the issue with running the tests with auxiliary
> > dynamically loaded modules in case of out-of-source build.
> > 
> > The first issue relates to the way modules loaded at runtime are built
> > on MacOS. Since the auxiliary libraries are built as a dynamically
> > loaded modules on MacOS instead of shared libraries as it is done on
> > Linux and BSD, another environment variable should be used to guide
> > <ffi.load> while searching the extension. Hence the values collected in
> > scope of <BuildTestCLib> macro need to be set to DYLD_LIBRARY_PATH
> > variable instead of LD_LIBRARY_PATH on MacOS.
> > 
> > Unfortunately, this rather small change doesn't resolve the problem at
> > all and the root cause lies much deeper than it seems at the beginning.
> 
> This paragraph looks uninformative, looks like you can just use
> "Moreover, " for the next sentence instead.

It could be "Moreover, " if it was another tiny problem, or I missed
another environment variable, but I think everything below is not just
"Moreover, ". BTW, considering your comment I hope this is not a big
problem for you (or, so-called "Moreover, " problem), and our team has
such an expert in MacOS environment!

Re suggestion: Ignoring.

> 
> > 
> > Apple tries their best to "protect their users from malicious software".
> > As a result SIP[1] has been designed and released. Now, Apple developers
> > are *so protected*, that they can load nothing being not installed in
> > the system, since some programs sanitize the environment before they
> > start child processes. Specifically, environment variables starting with
> > DYLD_ and LD_ are unset for child process started by system programs[2].
> 
> Sorry, but this paragraph is too wordy. I propose to reformulate it like
> the following:

Does it make the commit message less clear? I hope no. At the same time,
there is an important notice that some programs sanitize the environment
before starting the child process.

> 
> | Moreover, Apple tries their best to "protect their users from malicious
> | software". As a result, SIP[1] has been designed and released. As a part
> | of it, environment variables starting with DYLD_ and LD_ are unset
> | for child process started by system programs[2].
> 
> Feel free to ignore.

Ignoring.

> 
> > 
> > That which does not kill us makes us stronger: fortunately, these
> 
> I suppose you can just drop this preamble.

Honestly, see no reason for this. Ignoring.

> 
> | These environment variables are used by FFI machinery to find the proper
> | shared library, hence we can still tweak testing environment before
> | calling <ffi.load>.
> 
> > environment variables are used by FFI machinery to find the proper
> > shared library, hence we can still tweak testing environment before
> > calling <ffi.load>. However, the value can't be passed via the standard
> > environment variable, so we prepend TEST_ prefix to its name to get
> > around SIP magic tricks. Finally, to set the variable required by FFI
> > machinery the introduced <utils.tweakenv> routine is used. PROFIT!
> > Your move, Cupertino geniuses.
> 
> The last two sentences are redundant. Remove them please.

This is your opinion. As for me, everything above is written just for
the last one. We implicitly agreed not to use "emotional" wording in
comments, but I don't like this (IMHO such wording makes grepping for
pitfalls easier). There is no strong language in the commit message
also. Again, see no reason for removing. Ignoring.

> 
> > 
> > [1]: https://support.apple.com/en-us/HT204899
> > [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> > 
> > Resolves tarantool/tarantool#5959
> > Follows up tarantool/tarantool#4862
> > 
> > Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
> > Co-authored-by: Mons Anderson <mons@cpan.org>
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  test/tarantool-tests/CMakeLists.txt           | 39 +++++++++++++++++--
> >  .../gh-4427-ffi-sandwich.test.lua             |  4 ++
> >  .../lj-flush-on-trace.test.lua                |  4 ++
> >  test/tarantool-tests/utils.lua                | 39 ++++++++++++++++---
> >  4 files changed, 77 insertions(+), 9 deletions(-)
> > 
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index 3e36ff86..c793ad60 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -69,11 +69,46 @@ set(LUA_PATH
> >  )
> >  set(LUA_TEST_SUFFIX .test.lua)
> >  set(LUA_TEST_FLAGS --failures --shuffle)
> > +set(LUA_TEST_ENV
> > +  "LUA_PATH=\"${LUA_PATH}\;\;\""
> > +  "LUA_CPATH=\"${LUA_CPATH}\;\;\""
> > +)
> >  
> >  if(CMAKE_VERBOSE_MAKEFILE)
> >    list(APPEND LUA_TEST_FLAGS --verbose)
> >  endif()
> >  
> > +# XXX: Since the auxiliary libraries are built as a dynamically
> > +# loaded modules on MacOS instead of shared libraries as it is
> > +# done on Linux and BSD, another environment variable should be
> > +# used to guide <ffi.load> while searching the extension.
> > +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> > +  # XXX: Apple tries their best to "protect their users from
> > +  # malware". As a result SIP (see the link[1] below) has been
> > +  # designed and released. Now, Apple developers are so protected,
> > +  # that they can load nothing being not installed in the system,
> > +  # since some programs sanitize the environment before they start
> > +  # child processes. Specifically, environment variables starting
> > +  # with DYLD_ and LD_ are unset for child process started by
> > +  # system programs (like /usr/bin/env used for preparing testing
> > +  # environment). For more info, see the docs[2] below.
> > +  #
> > +  # That which does not kill us makes us stronger: fortunately,
> > +  # these environment variables are used by FFI machinery to find
> > +  # the proper shared library, hence we can still tweak testing
> > +  # environment before calling <ffi.load>. However, the value
> > +  # can't be passed via the standard environment variable, so we
> > +  # prepend TEST_ prefix to its name to get around SIP magic
> > +  # tricks. Finally, to set the variable required by FFI machinery
> > +  # the introduced <utils.tweakenv> routine is used.
> 
> Same comments as for the commit message.

Removed the line that bothers you by its redundancy. Here is the diff:

================================================================================

diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index c793ad60..475e2e5d 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -93,8 +93,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
   # system programs (like /usr/bin/env used for preparing testing
   # environment). For more info, see the docs[2] below.
   #
-  # That which does not kill us makes us stronger: fortunately,
-  # these environment variables are used by FFI machinery to find
+  # These environment variables are used by FFI machinery to find
   # the proper shared library, hence we can still tweak testing
   # environment before calling <ffi.load>. However, the value
   # can't be passed via the standard environment variable, so we

================================================================================

> 
> > +  #
> > +  # [1]: https://support.apple.com/en-us/HT204899
> > +  # [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> > +  list(APPEND LUA_TEST_ENV TEST_DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> > +else()
> > +  list(APPEND LUA_TEST_ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> > +endif()
> > +
> >  # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> >  # with dependecies are set in scope of BuildTestLib macro.
> >  add_custom_target(tarantool-tests
> > @@ -83,9 +118,7 @@ add_custom_command(TARGET tarantool-tests
> >    COMMENT "Running Tarantool tests"
> >    COMMAND
> >    env
> > -    LUA_PATH="${LUA_PATH}\;\;"
> > -    LUA_CPATH="${LUA_CPATH}\;\;"
> > -    LD_LIBRARY_PATH="${LD_LIBRARY_PATH}"
> > +    ${LUA_TEST_ENV}
> >      ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
> >        --exec '${LUAJIT_TEST_COMMAND}'
> >        --ext ${LUA_TEST_SUFFIX}
> > diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > index 64df5dbd..651dc3f4 100644
> > --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > @@ -3,6 +3,10 @@ local utils = require('utils')
> >  -- Disabled on *BSD due to #4819.
> >  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> >  
> > +-- XXX: Tweak the process environment to get around SIP.
> > +-- See the comment in suite CMakeLists.txt for more info.
> > +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> > +
> >  utils.selfrun(arg, {
> >    {
> >      arg = {
> > diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> > index edc5cf61..1ad4f832 100644
> > --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> > +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> > @@ -3,6 +3,10 @@ local utils = require('utils')
> >  -- Disabled on *BSD due to #4819.
> >  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> >  
> > +-- XXX: Tweak the process environment to get around SIP.
> > +-- See the comment in suite CMakeLists.txt for more info.
> > +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> > +
> >  utils.selfrun(arg, {
> >    {
> >      arg = {
> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index d2dd71b0..9bdb71ec 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
> > @@ -1,7 +1,12 @@
> >  local M = {}
> >  
> > +local ffi = require('ffi')
> >  local tap = require('tap')
> >  
> > +ffi.cdef([[
> > +  int setenv(const char *name, const char *value, int overwrite);
> > +]])
> 
> Side note: looks like we need to move `os.setenv()` fucntion from the
> Tarantool's built-ins onboard as `misc.setenv()`.

I personally don't think this is a vital function to be moved to LuaJIT,
but I'm open for ideas!

> 
> > +
> 
> <skipped>
> 
> > -- 
> > 2.25.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/3] Fix out-of-source testing on MacOS

[Igor Munkin via Tarantool-patches]

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in 1.10, 2.6, 2.7 and master.

On 05.04.21, Igor Munkin wrote:
> This series consists of three patches, but only one is related to the
> problem per se. So I ought to explain the rationale for two others.
> 
> The reason is quite clear: I finally got a round tuit[1]. The first
> patch removes an excess artefact left after converting all testing
> targets to the corresponding .PHONY rules in scope of #4862. The
> changeset was so huge, so it's no wonder such minor thing has been left
> unnoticed. The second patch enhances the testing "framework" used in
> tarantool-tests set. As a result, one can implement tests for JIT
> machinery in a simple way via <utils.selfrun>. The third patch of the
> series is the one fixin #5959: there are many words left for posterity
> in the commit message (hope you'll have a good time reading it and the
> blogpost[2] found by Mons).
> 
> Issue: https://github.com/tarantool/tarantool/issues/5959
> Branch: https://github.com/tarantool/luajit/tree/imun/gh-5959-fix-dynamic-modules-loading-on-macos
> CI: https://github.com/tarantool/tarantool/tree/imun/gh-5959-fix-dynamic-modules-loading-on-macos
> 
> [1]: https://en.wiktionary.org/wiki/round_tuit
> [2]: https://briandfoy.github.io/macos-s-system-integrity-protection-sanitizes-your-environment/
> 
> Igor Munkin (3):
>   test: remove excess dependency for tests target
>   test: make utils.selfrun usage easier
>   test: fix dynamic modules loading on MacOS
> 
>  test/tarantool-tests/CMakeLists.txt           | 42 ++++++++++++--
>  .../gh-4427-ffi-sandwich.test.lua             | 55 ++++++++++---------
>  .../lj-flush-on-trace.test.lua                | 55 ++++++++++---------
>  test/tarantool-tests/utils.lua                | 44 +++++++++++++--
>  4 files changed, 134 insertions(+), 62 deletions(-)
> 
> -- 
> 2.25.0
> 

-- 
Best regards,
IM