-
Notifications
You must be signed in to change notification settings - Fork 304
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
pkgconfig: Correctly rename libplist*.pc to libplist*-2.0.pc
- Loading branch information
Showing
4 changed files
with
3 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
File renamed without changes.
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I'm currently looking at updating the Debian packaging in preparation for a release of the libimobiledevice stack. What would be the reason for renaming this file, since the lib itself has not been renamed (nor the soname changed). Even the internal configure file still looks for libplist.pc and not libplist-2.0.pc
CYTHON_PLIST_INCLUDE_DIR=$($PKG_CONFIG --variable=includedir libplist)/plist/cython137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/libimobiledevice/libimobiledevice/blob/master/configure.ac#L129 still uses libplist.pc as well
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corsac-s oh good catch about the configure.ac
So the rename is according to the pkg-config conventions that suggest to include an API version in the .pc filename, thus allowing to install multiple incompatible versions alongside each other. We forgot to add it when libplist 1.0 was released and I forgot it when I released 2.0 and we now wanted to correct this 'mistake'.
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense, except that inside the .pc file (and on the filesystem) the library name is not versioned, so in the end the Libs flag you get is still
-lplistand you can't really have multiple incompatible versions.I'm not a huge expert in library packaging but I'd say at that point that libplist.pc/libplist.so are all for libplist-2.0 and if/when you want to add an incompatible libplist-3.0 (or something like that) then you introduce libplist-3.0.pc / libplist-3.0.so / -lplist.
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I noticed you did the same change (updating the library name to include a version number) for all the library. I'm unsure what's the real point here since I don't think there were incompatible API changes in these releases (and they should usually be handled by updating soname). It actually makes it difficult for packaging, because suddenly the library the depending packages (or libraries) are linked against will disappear (for example here: kodi, upower, gvfs-backends, to only check “external” projects). Every one of these projects build system need to be updated to look for and link against the new name, while there is no source incompatibility, which is more work for them and the packagers.
Can you confirm it was a one-time change and you don't intend to raise the version number at each release?
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a one-time change until the API actually changes (i.e. the semantic version major number will be increased). Since we had this huge gap between the releases, we thought now and only now would be the time with such a change because everyone needs to adapt to the new packages now anyway.
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change also seems to cause this issue for a package I maintain.
My package works with libplist all the way back to 0.16, and the current pkgconfig check is for "libplist", so configure fails now that it apparently needs to search for both "libplist" and "libplist-2.0". For the other library depencies (e.g. libcurl, libwebsockets, libsodium, libav) there are no versions in the pkconfig names, and I'm thankful for that because otherwise it would be a pain to maintain. Could you reconsider your approach?
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be too late at that point because some packages might have started requesting libplist-2.0. What I did for Debian packaging is shipping a symlink libplist.pc -> libplist-2.0.pc (and same thing for libplist.so, libplist.so.3 etc.) since there are no ABI/API break. That means packages requesting libplist will still work (but they should still be updated to libplist-2.0 at one point)
137716dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s very good for Debian packages, I’ll see if the same can be done in OpenWrt. But it still leaves other distros with broken builds. And the same for people building from release tarballs. So I hope upstream will make a new release that also ships a regular “libplist.pc”. I suppose a Github issue is in order here, so I will add that.