Re: [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Fri, Oct 02, 2020 at 03:23:17PM +0300, Timur Safin wrote:
> Before getting into all gory details and start to actively participate in discussion 
> I want to highlight some basic principles which we used while preparing these simple 
> patches (some are obvious, but some not that much):
> 
> - I try to offload functionality to the code existing inside of Tarantool as much as 
> possible. If there are some structures or functions - refer to them. And this principle 
> better be applied not only to Tarantool core specific functionality, but also to all 3rd 
> party functions it uses. E.g. small, msgpuck, or even heap.
> 
> - And current principle to compile against them is simple - we use these 3rd party headers 
> for compilation of functions in the module, but ignore libraries for a moment. We delay 
> binding till run-time where system loader would substitute symbol from Tarantool executable 
> which eventually called us as external module. If we could use msgpuck or small in this 
> manner then we almost entirely avoiding most ABI incompatibility problems. 
> 
> - The question is: where to get those 3rd party headers? We may use several approaches 
>   in modules:
> 
> 1. via cmake ExternalProject_Add to github repo;
> 2. via git submodule to github repo;
> 3. via systems package manager dependency and binary dependency (e.g. apt or rpm).
> 
> So happened, that in merger we used several of them, e.g. submodule for small https://github.com/tsafin/tarantool-merge/tree/master/src
> tarantool-dev system package for msgpuck. And simply copy-pasted heap.h from salad.
> [IMVHO, salad worth to be treated similarly as small. For better reuse we may export
> It to external repository]
> 
> 3 different approaches for quite similar cases of code reuse. :) Eventually we may 
> develop the consistent approach here - how to reuse 3rd party Tarantool library 
> in ABI stable way, which may be originating of different versions of Tarantool? 
> (e.g. small in 2 its incarnations from 2.x and 1.10)
> 
> Would it be always enough to use older version of headers, and run-time linking with 
> parent executable? At the moment I see no reason to not do so. As far as I can see 
> small is quite similar from ABI prospective between 1.10 and 2.x at the moment, 
> we could start away as is, but certainly should add binary compatibility check somewhere 
> into CI (to small repo, to Tarantool repo, and/or to merger repo) for future 
> compatibility guarantees. I do assume that small repository was created just for such
> usage scenarios like here (module reusing some useful functionality), and it's 
> correct way to use it as submodule or ExternalProject_ in cmake. We just need to
> make sure it will not broken later.
> 
> Same for msgpuck - it's already created for better code reuse - we just need to 
> Make sure it's possible to be used in future compatible way. (Via extra CI here 
> and there).
> 
> Having said all that, now we could return to patches discussions - see my 
> other emails...
> 
> Regards,
> Timur

You cannot lean on API / ABI compatibility now and implement it later.
You should hold layout of non-opaque structures, think how they will be
extended in a future (introduce padding, maybe), probably introduce some
level of indirectness (public + internal structures, calls instead of
inline functions) and estimate performance impact. You should try to
defend your approach and maybe reimplement it two-three times.

Almost the whole task was about porting the internal modules to the
stable API (the rest is trivial). Either expose something from
tarantool, or provide stable API from small and msgpuck. I said this a
lot of times during the last quarter. I sent you draft of [1] August, 3.
Two months later, a week before planned feature freeze, you say
'eventually we'll do it' and continue attempts to push the team to agree
on the fragile approach.

You say 'I assume it will work' ignoring past breakage cases [1]. You
say 'we can just add binary layout checks later', however you're surely
saw [2] and know that extra work is necessary before freezing ABI.

Optimistic ABI compatibility? I just don't understand that all.

[1]: https://lists.tarantool.org/pipermail/tarantool-discussions/2020-September/000095.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019592.html