[PATCH wayland v2 3/4] wayland-util: build/ship as separate shared library

[Emil Velikov]

On 17 March 2017 at 13:32, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri, 17 Mar 2017 12:07:45 +0000
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
>> On 17 March 2017 at 11:10, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > On Thu, 16 Mar 2017 15:32:46 +0000
>> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> >
>> >> On 16 March 2017 at 12:40, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
>> >> > Hi,
>> >> >
>> >> > since I have so much trouble making my mind on this patch, how about a
>> >> > silly counter-proposal?
>> >> >
>> >> > Let's squash libwayland-server.so and libwayland-client.so into a
>> >> > single libwayland.so.
>> >> >
>> >> > That would take care of the duplicate exported symbols issue not just
>> >> > for the util functions, but also for the exported variables produced by
>> >> > wayland-scanner. Since we have many programs (libEGL!) that necessarily
>> >> > already link to both, there cannot be problems from linking to
>> >> > everything always.
>> >> >
>> >> > We would still need to install dummy libwayland-server.so and
>> >> > libwayland-client.so just for pulling in libwayland.so, but that's no
>> >> > worse than Emil's proposition.
>> >> >
>> >> > Whether we have the existing .pc files telling to link to server/client
>> >> > or just libwayland.so would be up for debate. I'm not suggesting to
>> >> > merge the .pc files.
>> >> >
>> >> > libwayland-server.so and libwayland-client.so have exactly the same set
>> >> > of dependencies.
>> >> >
>> >> > Any downsides to this approach vs. doing nothing vs. Emil's
>> >> > libwayland-{client,server,util}.so?
>
>> How exactly do you see libwayland.so relative to the client/server one
>>  - which provides what symbols, etc.
>>  - are the client/server DSOs - symlinks/other ?
>
> This is how I imagined it:
>
> - libwayland.so contains all the code, and exports both server and
>   client symbols.
>
> - libwayland-server.so and libwayland-client.so are normal DSOs that
>   export essentially no symbols, they only depend on libwayland.so and
>   cause it to be loaded.
>
> In other words, you proposed to move util functions into a third
> library. I proposed to move everything into a third library.
>
I'm yet to see/hear of such a DSO, and I'd imagine it will be rather
confusing/misleading.

>
> One thing now coming to my mind is this. With your
> https://github.com/evelikov/wl_link_test experiments, did you try the
> following?
> 1. build and install old wayland
> 2. build link test
> 3. build and install your patched wayland
> 4. run link test (do not rebuild)
>
> The point here is that the application executable nor library does not
> have libwayland-util.so as NEEDED themselves. Still, they need to
> continue working fine.
>
Dan pointed out the same concert - bumping the -version-info will handle that.

> Then there is the case of dlopen()'ing these libraries. I think there
> might be something unexpected to expect because we have this:
> https://cgit.freedesktop.org/wayland/weston/tree/libweston/compositor-drm.c?id=2.0.0#n1575
> introduced in commit 97f2952b.
>
This one works perfectly fine with or w/o a version-info change - try
the wl-dl-test.

> Does that mean that if someone writes a plugin for his app, and that
> plugin uses wl_list, your patch would require them to at least rebuild
> the plugin?
>
There's no requirement to rebuild anything. If we're concerned about
using FOO build against wayland X+1, while running on wayland X -
version-info will make that an explicit error, as it should be.
Similar thing has been done already twice, afaict.

If that's not a concern one need not bother changing version-info.

> ISTR libSDL dlopen()'s libwayland-client.so which is a slightly
> different case again.
>
Yes, dlopen/dlsym is safe, regardless of who is doing it.
To explain why/who - as one does dlsym(libwayland-client.so.0_handle,
"wl_list_goo") the linker will propagate to the direct dependencies
and if any of those provide the symbol a pointer to it will be
returned.

> The case where one does rebuild the application is what you at least
> tested, as I understand.
>
Correct.

FWIW: I think it would have saved us all time, if some/most/all of the
above were asked much earlier.
If you have any other concerns/issues please shout, pretty please ?

>> > Indeed, that's what I need: Reviewed-by's and Acked-by's from people
>> > with proper reputation.
>> >
>> > Just like for any patch really, the strength of reviews needs to
>> > reflect the impact of the patch.
>> >
>> How about we do something like Daniel ?
>>  - he pointed out some possible flaws
>>  - I dismissed those and provided a simple test case to justify my claim
>>
>> If every 'issue' is refuted, shout for X weeks slap an Ack and let us
>> carry on with out lives ?
>
> I do not see Daniel's Reviewed-by's or Acked-by's on this revision of
> this patch, nor the earlier one.
>
> I will not give my R-b or A-b for this patch, because I don't feel
> qualified. The same goes for my counter-proposal, too. The
> counter-proposal would need your R-b and someone else's A-b as well.
>
Ack - your previous email was perfectly clear on these.

I hope that one day you'll reconsider wrt Acked-by and I'd prefer if
that covers someone else's work than mine.

Some food for thought:
-  Do you have many experiences with bugs caused by symbol collision ?
 - Have you considered issues, that you don't have personal experience
with, worth-while ? Where there many ?

Thanks
Emil