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 VERSION_MIN for automated generated dependency requirements #16588

Merged
merged 1 commit into from Sep 8, 2019

Conversation

peak3d
Copy link
Contributor

@peak3d peak3d commented Sep 8, 2019

Description

kodi cmake process replaces @ADDON_DEPENDS@ inside addon.xml.in files of binary addons with a list of kodi API dependencies + version. Currently the interface version is taken instead interface min version what is wrong (https://github.com/peak3d/inputstream.adaptive/blob/master/inputstream.adaptive/addon.xml.in#L7).

When version of addon is increased (https://github.com/xbmc/xbmc/blob/master/xbmc/addons/kodi-addon-dev-kit/include/kodi/versions.h) but VERSION_MIN stays untouched, the binary addon should be still compatible with kodi versions which support MIN_VERSION

Motivation and Context

After bumping ADDON_INSTANCE_VERSION_INPUTSTREAM, building addon with this new version, kodi with previous version is unable to load the addon (incompatible). This is incorrect because VERSION_MIN was not changed.

How Has This Been Tested?

  • Bump ADDON_INSTANCE_VERSION_INPUTSTREAM
  • build binary addon
  • revert ADDON_INSTANCE_VERSION_INPUTSTREAM
  • Launch kodi

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

@peak3d peak3d added Type: Fix non-breaking change which fixes an issue Component: Binary add-ons v19 Matrix labels Sep 8, 2019
@peak3d peak3d added this to the Matrix 19.0-alpha 1 milestone Sep 8, 2019
@peak3d peak3d changed the title [Binary addons] Use VERSION_MIN for automated generaed dependency requirements [Binary addons] Use VERSION_MIN for automated generated dependency requirements Sep 8, 2019
@peak3d peak3d merged commit 36dc4b0 into xbmc:master Sep 8, 2019
@peak3d peak3d deleted the minversion branch September 8, 2019 17:03
@ksooo
Copy link
Member

ksooo commented Sep 8, 2019

@peak3d you should not have merged this PR without review from at least one of the bin-addon guys, like @AlwinEsch. I think this PR makes things worse than before.

  1. Kodi version 1.0, min version 1.0

  2. Somebody adds a new struct member at end of a struct that is always allocated by Kodi and handed over to the addon, then. This is an ABI comaptible change, thus
    Kodi version increases to 1.1, min version stays 1.0

  3. you do not modify your addon source code
    => you recompile your addon against Kodi v 1.1 + mv 1.0
    => without your PR Addon refuses to load in Kodi v 1.0 + mv 1.0. That's what you want to fix with your PR. So far so good.

  4. You modify your addon source and use the new struct member introduced with v 1.1
    => You recompile your addon against Kodi v 1.1 + mv 1.0
    => with your PR addon.xml still has a depends to 1.0 (which still min version of Kodi) and the addon will crash the moment it want to access the v1.1 struct member if you use it in Kodi v1.0 + mv 1.0

As I told @AlwinEsch several times in the past it does not really work to automatically add the depends to addon.xml.

What we had before your PR was not correct, but your PR mkes it even worse, because it now can crash.

@peak3d
Copy link
Contributor Author

peak3d commented Sep 8, 2019

From techical POV you can do everything you want, because kodi and addon knows the interface api versions of each other (runtime). If you don't want to add the necessary source version checks, you simply have to increase VERSION_MIN. This has not changed at all.

Runtime information addon get from kodi: https://github.com/peak3d/inputstream.adaptive/blob/master/src/main.cpp#L3651
This can be used for example here: https://github.com/peak3d/xbmc/blob/chapter/xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/Inputstream.h#L710
Note that this file is during compilation in scope of the addon (copied), and m_instanceData is allocated by kodi, but filled from addon, therefore it has to be checked

Runtime information kodi get from addon:
https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDInputStreams/InputStreamAddon.cpp#L360

Passing versions was introduced ~ 1 year ago, with the goal to have the ability to implement new features which are compatible to older kodi versions. By using VERSION instead MIN_VERSION this concept does not work.

Edit: For your point 4), you have 2 solutions:
1.) Increase VERSION_MIN to 1.1 (means that kodi has to be compiled with min 1.1 api interface version
2.) Put the version checks in your code as described above

@AlwinEsch
Copy link
Member

I know your reason, but this is not correct enough, there must be something inside Kodi changed and maybe use both on addon.xml (the MIN and the CURRENT version).

With this your Version check here does no more work: https://github.com/xbmc/xbmc/blob/master/xbmc/cores/VideoPlayer/DVDInputStreams/InputStreamAddon.cpp#L360

The DependencyVersion() now returns always only the Min but not the current.

@peak3d
Copy link
Contributor Author

peak3d commented Sep 8, 2019

@AlwinEsch Ok, this is not what I have expected -> I'll revert now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Binary add-ons Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants