From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Ostanevich <sergos@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS Date: Tue, 6 Apr 2021 21:05:59 +0300 [thread overview] Message-ID: <20210406180559.GF29703@tarantool.org> (raw) In-Reply-To: <D61EE8AD-0195-44AF-B50E-3C6D81144B97@tarantool.org> 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
next prev parent reply other threads:[~2021-04-06 18:06 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-05 17:11 [Tarantool-patches] [PATCH luajit 0/3] Fix out-of-source testing " Igor Munkin via Tarantool-patches 2021-04-05 17:11 ` [Tarantool-patches] [PATCH luajit 1/3] test: remove excess dependency for tests target Igor Munkin via Tarantool-patches 2021-04-06 7:38 ` Sergey Kaplun via Tarantool-patches 2021-04-06 8:02 ` Igor Munkin via Tarantool-patches 2021-04-06 9:51 ` Sergey Kaplun via Tarantool-patches 2021-04-06 15:38 ` Sergey Ostanevich via Tarantool-patches 2021-04-06 16:19 ` Igor Munkin via Tarantool-patches 2021-04-05 17:11 ` [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier Igor Munkin via Tarantool-patches 2021-04-06 13:01 ` Sergey Kaplun via Tarantool-patches 2021-04-06 13:35 ` Igor Munkin via Tarantool-patches 2021-04-06 16:22 ` Sergey Ostanevich via Tarantool-patches 2021-04-06 18:32 ` Igor Munkin via Tarantool-patches 2021-04-07 12:16 ` Sergey Ostanevich via Tarantool-patches 2021-04-05 17:11 ` [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS Igor Munkin via Tarantool-patches 2021-04-06 17:02 ` Sergey Ostanevich via Tarantool-patches 2021-04-06 18:05 ` Igor Munkin via Tarantool-patches [this message] 2021-04-07 16:38 ` Sergey Kaplun via Tarantool-patches 2021-04-07 17:33 ` Igor Munkin via Tarantool-patches 2021-04-07 21:17 ` [Tarantool-patches] [PATCH luajit 0/3] Fix out-of-source testing " Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210406180559.GF29703@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=sergos@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox