[Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS
Igor Munkin
imun at tarantool.org
Wed Apr 7 20:33:10 MSK 2021
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 at 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 at tarantool.org>
> > Co-authored-by: Mons Anderson <mons at cpan.org>
> > Signed-off-by: Igor Munkin <imun at 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
More information about the Tarantool-patches
mailing list