Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[binary addons] use TARGET_LINKER_FILE_NAME as library filename #7783

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@stefansaraev
Copy link
Contributor

stefansaraev commented Aug 10, 2015

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 11, 2015

Gave it a quick try on WIN32 and TARGET_LINKER_FILE_NAME works fine here as well. But while testing I noticed a general problem with using those generators, see #7570 (comment) so maybe we should wait with merging this until the other issue is resolved.

@Montellese Montellese self-assigned this Aug 11, 2015

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 12, 2015

I don't mind either way, but the initial change to use the full lib name was intended. This way you could potentially have multiple addon versions installed (e.g. for stable and master kodi)

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 12, 2015

@wsnipex: I'm not really knowledgable in this area. So when packaging/installing a binary addon you take the full library name (including the version) for the actual .so file and then create a link with the library name without full version?
While our addon manager doesn't support different versions of the same addon who knows what will be possible in the future ;-)

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 12, 2015

yup. thats standard in linux. For libraries, the libname.so is normally only installed with the corresponding -dev package. We don't do that for our addons though.
this is how it looks like:

lrwxrwxrwx 1 root root 16 Jul 26 22:20 /usr/lib/kodi/addons/pvr.hts/pvr.hts.so.16.0 -> pvr.hts.so.2.2.5
lrwxrwxrwx 1 root root 15 Jul 26 22:20 /usr/lib/kodi/addons/pvr.hts/pvr.hts.so -> pvr.hts.so.16.0
-rw-r--r-- 1 root root 285728 Jul 26 22:20 /usr/lib/kodi/addons/pvr.hts/pvr.hts.so.2.2.5

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

stefansaraev commented Aug 12, 2015

@wsnipex I would like to see contents of android or ios addon (generated by kodi build system). before it was supposed to be plain .so without version info. now if it's still plain .so but versioned library name is referenced in addon.xml .... - you get my point ;)

also, symlinks are not so good when you are packaging addons as zips

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 12, 2015

ok, Droid is really broken atm, cause the libs are renamed somewhere(IIRC in apk packaging) to libname.so. Seems nobody ever tested this.
So this PR is valid for droid, but I'd like to keep current behavior for linux.
@stefansaraev the libname.so IS a symlink, the real file is the versioned one.

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

stefansaraev commented Aug 12, 2015

as every single addon, before now, referenced a .so without version info. on OpenELEC I just dereference-copy the so to target zip.

now, we have some addons that reference .so (3rdparties) and others that reference .so.*. I have two options: 1) copy everything (I dont really like this) or 2) xmlstarlet

I dont really mind if you prefer versioned libs in zips, I would like to have some consistency, and if possible - not force me to use tools like xmlstarlet to decide what to package :)

EDIT: how about ios/osx btw ?

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 12, 2015

what is the problem with retaining the symlinks in the zip?

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

stefansaraev commented Aug 12, 2015

what is the problem with retaining the symlinks in the zip?

when installing an addon, kodi will dereference symlinks

but the initial change to use the full lib name was intended

are you sure? because #7780 (comment)

I think we should revert it like before, but as said, I dont mind if you want to overcomplicate it a bit. up to you :)

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 12, 2015

I didn't know that the change was intentional, I only tested if @wsnipex's adjustments in that PR also work on WIN32.

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 12, 2015

then merge it as is, because its much better to keep dirty workarounds/hacks like dereferencing symlinks and renaming libs instead of doing it correctly.

I'm out here.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 14, 2015

You could use CORE_SYSTEM_NAME to differentiate between Android and Linux. Not sure how it should be handled on OSX, iOS, FreeBSD and RBPi. ON WIN32 it doesn't matter and on Android we need to use TARGET_LINKER_FILE_NAME.

@akva2

This comment has been minimized.

Copy link
Contributor

akva2 commented Aug 18, 2015

the real solution here is to fix the zip code to extract symlinks properly. i have attempted it but the zip files i get from cpack do not correspond to (my understanding) of the zip spec so i'm stoked atm. files are properly flagged as a symlink, but when i try to dig into the extra field to get the target name, there is unexpected garbage at the start of the block :/ using a hexeditor i can see the expected data is in there, but the headers do not lead me to it :(

the files are not broken as such since unzip extracts them just fine.

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 24, 2015

time to get something in, since droid nightlies have broken addons

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

stefansaraev commented Aug 24, 2015

jenkins build this please

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

stefansaraev commented Aug 24, 2015

android apks look fine. your button.

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 24, 2015

I will not merge this, I think I made my point clear above. If you want it as is, someone else will have to press the button.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 24, 2015

@wsnipex: Would you be fine with my previous proposition to special case android?

@stefansaraev stefansaraev deleted the stefansaraev:binaddons branch Aug 24, 2015

@wsnipex

This comment has been minimized.

Copy link
Member

wsnipex commented Aug 24, 2015

stefansaraev referenced this pull request in stefansaraev/TB Jan 12, 2017

[fix] kodi addons: do not install and use versioned .so libs
year and half late, versioned addon libs are still trouble for no gain.
pvrmanager does not give a f*** about soname changes after updating an addon:

ERROR: ADDON: Could not locate pvr.hts.so.x.y.someoldversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.