Mir

Code review comment for lp:~alan-griffiths/mir/fix-1415321

Revision history for this message
Chris Halse Rogers (raof) wrote :

>> You don't need to bump the SONAME of mirclient-debug-extension.
...
>> I don't think you need to bump mirclient SONAME? The only loss I can see is
>> the symbols exposed purely for the tests and PrivateProtobuf, neither of which
>> are public. abi-compliance-checker will complain, but will be wrong in this
>> case.

> I thought that rewriting the symbol maps as I have would require the SONAME bump? As xxx@MIR_CLIENT_8 etc won't resolve?

Rewriting the symbol maps as you had done *would* require the SONAME bump, but as you've now listed all the xxx@MIR_CLIENT_8 symbols in the MIR_CLIENT_8 block they'll all resolve correctly (other than the ones we've always said are private).

Needs fixing: You don't need to postfix all the C symbols with “*”; we do that for the C++ symbols because the parameter signature is a part of the function name in C++. That's obviously not relevant for C symbols, and is potentially dangerous if one of our functions ends up being named a prefix of another one.

Needs fixing: The changelog should no longer document a mirclient ABI bump :)

> libmircommon is "common" between by libmirplatform, libmirclient and libmirserver - it should contain symbols that > are used by all three. (There's a follow-up MP that removes some symbols used only by libmirplatform and
> libmirserver.)

I'm not sure why that is the definition of mircommon; I don't see why symbols shared by mirclient and mirserver shouldn't be in there.

> libmirclient is used by libmirserver so why shouldn't it contain symbols shared with the latter?

Two reasons:
1) I'd ideally like libmirclient to export only a C ABI,
2) I expect in the not too distant future that libmirserver will *not* depend on libmirclient; that it currently does is a strange arrangement forced on us by the nested platform not being loadable.

While I agree it makes sense for symbols shared only between mirserver and mirplatform to be in mirplatform, this is because mirplatform exists only to provide infrastructure for mirserver components. mirclient and mirserver should both be top-level objects.

I won't block on the somewhat philosophial issue of mirserver → mirclient dependency.

review: Needs Fixing

« Back to merge proposal