xdo_get_keysym_charmap symbol removed without SONAME bump

[Daniel Kahn Gillmor]

Hi Jordan and other libxdo users--

I'm looking at updating the version of xdotool that we have in debian to
be built off the head of the git development, since it seems fairly
stable and has some apparent improvements in handling multiple keyboard
layouts.

However, i just noticed that bbf0e70ab614852cff8111cabeed8d5ffa9554e0
removed the function xdo_get_keysym_charmap, which had previously been
exported in libxdo.so.3

From what i can see of the change, the symbol is removed because the
function doesn't make sense any more with the new use of libxkbcommon.

But: removing a symbol from a library is the sort of change which the
library is expected to bump the SONAME for (making it libxdo.so.4).

However, I don't know of any code in the wild that uses this symbol (and
i'm certain that nothing in debian does [0]), so i'm kind of inclined to
go ahead and just ship with the missing symbol and without a bump of the
SONAME.

If you have any concerns about that plan, or if you'd rather go ahead an
bump the SONAME to 4, please let me know. (note that bumping the SONAME
might mean that the updated package won't make it into the upcoming
debian jessie release, since library transitions are being rejected at
the moment).

All the best,

--dkg

[0] http://codesearch.debian.net/search?q=xdo_get_keysym_charmap

[Jordan Sissel]

Hey dkg!

I am inclined to agree with your assessment. I don't know if anything uses that function outside of libxdo itself. It's primary use (from what I remember) was internal for libxdo and may have only accidentally been exported, not intentionally.

+1 on not bumping SONAME. However, I am willing to bump SONAME version if others demand it.

-Jordan

[Daniel Kahn Gillmor]

Hi Jordan--

On Fri 2014-10-03 15:40:20 -0400, Jordan Sissel wrote:

> I am inclined to agree with your assessment. I don't know if anything uses
> that function outside of libxdo itself. It's primary use (from what I
> remember) was internal for libxdo and may have only accidentally been
> exported, not intentionally.

hm, looking further with help from Jakub Wilk
(https://bugs.debian.org/764076) i see that the xdo_t struct itself has
changed its form and size (the modmap and keymap members were removed).

And charcodemap_t has changed its semantics (but not its size):

typedef struct charcodemap {
wchar_t key; /** the letter for this key, like 'a' */
KeyCode code; /** the keycode that this key is on */
KeySym symbol; /** the symbol representing this key */
- int index; /** the index in the keysym-per-keycode list that is this key */
- int modmask; /** the modifiers activated by this key */
+ int group; /** the keyboard group that has this key in it */
+ int modmask; /** the modifiers to apply when sending this key */
/** if this key need to be bound at runtime because it does not
* exist in the current keymap, this will be set to 1. */

i confess i don't fully understand the difference between "index" and
"group" -- if they the same thing with different names, and if the
modmask comment change is just a clarification, then this actually isn't
an ABI change (though it is an API change, since the member was renamed)

lastly, the struct keysymcharmap (keysymcharmap_t) was removed, but i
think that's functionally the same API change as the removal of
xdo_get_keysym_charmap().

All of these changes were introduced in
bbf0e70ab614852cff8111cabeed8d5ffa9554e0, which appears to be the major
API and ABI breakage.

> +1 on not bumping SONAME. However, I am willing to bump SONAME version if
> others demand it.

So to fix the immediate problem for debian packages, i'm looking at
a couple options:

0) bump the SONAME -- i'm not sure what that would do to xdotool in
debian, since it would be a library transition. This would also
make debian's libxdo's SONAME diverge from upstream's, which i don't
like.

1) change the structs back: reintroduce the missing elements for xdo_t,
and just let them be unused -- i'm not entirely sure how to fix
charcodemap_t if those changes are semantic, since (a) we don't want
to change the size of the struct, and (b) we don't want one value to
have two different meanings. Maybe we punt on the charcodemap
changes? Note that this change would make things compiled against
debian's 3.20140805.1 of libxdo be binary incompatible with
unpatched versions of the same checkout of libxdo. (this same
problem already exists between 3.2013* and 3.20140805, though)


For the longer-term future of libxdo, i think the right thing would be
to move the declarations of the structs into internal header files, and
just refer to them as generic pointers outside the library.

What do you think? I'm likely to go ahead with option (1) for now, but
if you have other proposals, i'd be happy to take guidance.

Regards,

--dkg

[Jordan Sissel]

Replies below -

On Mon, Oct 6, 2014 at 1:51 PM, Daniel Kahn Gillmor <d...@fifthhorseman.net> wrote:

Hi Jordan--

On Fri 2014-10-03 15:40:20 -0400, Jordan Sissel wrote:

<snip>

i confess i don't fully understand the difference between "index" and
"group" -- if they the same thing with different names, and if the
modmask comment change is just a clarification, then this actually isn't
an ABI change (though it is an API change, since the member was renamed)

It's been quite some time since I've looked at the code deeply. I'm not sure I'll have energy to correctly identify any ABI breakages. It may be most wise to simply bump the SONAME version and move on.
 
<snip>

 

For the longer-term future of libxdo, i think the right thing would be
to move the declarations of the structs into internal header files, and
just refer to them as generic pointers outside the library.

 

What do you think?  I'm likely to go ahead with option (1) for now, but
if you have other proposals, i'd be happy to take guidance.

Regards,

Testing ABI compat could be done by testing old xdotool against new libxdo. If it fails to compile or the test suite fails, then I think we broke it and we'll know that either a compatibility patch is required (time permitting) or an SONAME version bump is required (assuming nobody has time to do this).

I don't know how much time I will have to test this, but if you don't hear from me within a week or two (or by whatever deadline you have, if earlier), then I think SONAME bump is what will be needed.

-Jordan

[Daniel Kahn Gillmor]

Thanks for the quick response, Jordan--

On 10/06/2014 07:11 PM, Jordan Sissel wrote:
> It's been quite some time since I've looked at the code deeply. I'm not
> sure I'll have energy to correctly identify any ABI breakages. It may be
> most wise to simply bump the SONAME version and move on.

My only concern about bumping the ABI is that it may cause xdotool to
not make it into debian jessie, which i'd like to avoid.

I've uploaded a modified version of 3.20140805.1 that reinstates the old
(unused) fields for xdo_t, and doesn't touch the charcodemap_t.

Anyway, seems to work against older versions, at least for non-fancy
forms of interaction :/

> Testing ABI compat could be done by testing old xdotool against new libxdo.
> If it fails to compile or the test suite fails, then I think we broke it
> and we'll know that either a compatibility patch is required (time
> permitting) or an SONAME version bump is required (assuming nobody has time
> to do this).
>
> I don't know how much time I will have to test this, but if you don't hear
> from me within a week or two (or by whatever deadline you have, if
> earlier), then I think SONAME bump is what will be needed.

I agree that an SONAME bump would be useful in the longer-term. If
we're going to do that, though, i think we should do something more
radical, like removing xdo_t entirely from xdo.h, and just leaving the
struct as an opaque pointer. We'd want to add getter and setter
functions for the parameters we want users to access (presumably the
ones without @internal)

I note that charcodemap_t is itself marked as @internal though -- is
that right? the only place it seems to be used other than as a way to
store and restore the modifier state is in t/showmodifiers.c. So maybe
we could leave that as an opaque struct as well?

That still leaves xdo_search_t as a non-opaque struct, which i think is
fair, because that struct really is part of the API for doing a search.

What do you think of this approach?

--dkg

[Jordan Sissel]

What's the time frame on solving this? I've been busy with work and parenting and haven't had a chance to address this.

I'd like to get a new xdotool release published before whatever Debian's deadline is so you can include it.

-Jordan