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

Optional addons #9706

Merged
merged 2 commits into from Apr 30, 2016

Conversation

@garbear
Copy link
Member

commented Apr 29, 2016

Adds optional add-ons to addon-manifest.xml

Optional add-ons are only used to set the enabled bit when add-ons are synced from the filesystem to the database. They do not participate in the other functions of system add-ons, such as preventing Kodi from starting or blocking being disabled in the GUI.

Replaces #9700

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

stupid question: what happens on further upgrade, if user has disabled one or more optional addons, but those are set to non-optional next release ? is it forced enabled in db or left as-is ?

@garbear

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

they are left as is. the only time Kodi considers the optional add-ons is when syncing new add-ons to the database. If the add-on is already in the database, it doesn't matter if it's optional or not.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

then that will bite us back soon or later

@garbear

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

I might be wrong. I don't know this system as well as @tamland

std::set<std::string> addonsToEnable(m_systemAddons);
addonsToEnable.insert(m_optionalAddons.begin(), m_optionalAddons.end());

m_database.SyncInstalled(installed, addonsToEnable);

This comment has been minimized.

Copy link
@tamland

tamland Apr 29, 2016

Member

This will set them to enabled in db. I.e your disabled optional addons will be re-enabled on every sync.

This comment has been minimized.

Copy link
@garbear

garbear Apr 29, 2016

Author Member

You're right. I'll fix this

@garbear garbear force-pushed the garbear:optional-addons branch from 40a9202 to e834745 Apr 29, 2016
@garbear

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

@tamland I updated the PR. optional add-ons will stay disabled now, I have tested this

{
int enable = 0;

if (system.find(id) != system.end() || optional.find(id) != optional.end())

This comment has been minimized.

Copy link
@garbear

garbear Apr 29, 2016

Author Member

system add-ons are enabled below, so should this be just if (optional.find(id) != optional.end())?

This comment has been minimized.

Copy link
@garbear

garbear Apr 30, 2016

Author Member

I guess this should stay, let's avoid having system add-ons in a disabled state to begin with

This comment has been minimized.

Copy link
@tamland

tamland Apr 30, 2016

Member

It's covered below, in the same transaction, so it doesn't really do anything.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2016

@tamland ok to merge? we should get this in before 17 alpha 1 drops

void SyncInstalled(const std::set<std::string>& ids, const std::set<std::string>& enabled);
void SyncInstalled(const std::set<std::string>& ids,
const std::set<std::string>& system,
const std::set<std::string>& optional);

This comment has been minimized.

Copy link
@tamland

tamland Apr 30, 2016

Member

Please try avoid vertical alignment in the future as it breaks formatting and vcs on every change. it's in our guidelines as well now.

@tamland

This comment has been minimized.

Copy link
Member

commented Apr 30, 2016

@garbear Fine with me if you want it in alpha 1, but let's be perfectly clear here: the way the joystick addon is currently handled really needs to be rethinked. We now have a dependency on a third party addon in core and special handling for one addon.

@tamland

This comment has been minimized.

Copy link
Member

commented Apr 30, 2016

jenkins build this please

@garbear

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2016

peripheral.joystick isn't a third party add-on. it contains the code removed by #8807 and is hosted by the organization https://github.com/kodi-game which, like https://github.com/kodi-pvr, is a kodi organization to handle game-related extensions. it is perfectly acceptable to have peripheral.joystick be an optional add-on.

@garbear garbear merged commit a09f146 into xbmc:master Apr 30, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
default Merged build finished.
Details
@garbear

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2016

should the stewardship of this add-on be moved to the xbmc org? it kinda feels out of place at kodi-game, because although controllers are used for games they can be used without games as well

@garbear garbear deleted the garbear:optional-addons branch Apr 30, 2016
@tamland

This comment has been minimized.

Copy link
Member

commented May 1, 2016

The point wasn't where it's hosted but that it's external, not part of the build/packaging or tree like other addons. I.e there's now a dependency/special handling in core code for it because it's in this weird state where it's optional, yet at the same time it's sort of required (hence why you wanted it to be excepted from the disable rule right?).

@garbear

This comment has been minimized.

Copy link
Member Author

commented May 1, 2016

I understand. This PR was meant to address the issues that the OE/LE packagers brought up, where the system vs non-system add-on dichotomy left no room for shipping add-ons "in between" these two extremes, peripheral.joystick being one but they had many others such add-ons. You suggested rethinking this PR, what approach would you take?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 1, 2016

Just to say this PR doesn't really address those issues - in fact peripheral.joystick was never really an issue for OE/LE as OE/LE builders can and do include it in the addon-manifest.xml because they know they're including ("shipping") the add-on within the build. Consequently when Addons*.db is built from scratch peripheral.joystick is correctly enabled, with or without this PR, at least in OE/LE (but thanks anyway!)

The issue that still exists even after this PR is that of non-system addons that the builder includes/ships for convenience, but which he/she may not want to be enabled by default (eg. PVR/AudioDecoder addons). Or addons that the builder does want enabled by default, but which can still be disabled by the user (eg. inputstream.*).

Right now there's no way for the builder to have this fine grained control over non-system addons that are shipped as part of a distribution, unless the builder adds the addons to addon-manifest.xml in which case the addons are always and permanently enabled, because they're now treated as "system addons" even though they are nothing of the sort (an abuse of addon-manifest.xml).

And with the current addons system, all non-system addons (shipped with the build, or user installed) will be disabled by default when rebuilding the Addons*.db (ie. first boot, or to resolve an issue). One could argue this is the correct status for the user installed addons, after all a user-installed addon may have been the reason for the db rebuild, but for those curated built-in addons perhaps the builder should have the final word.

Actually, another nice feature (in addition to being able to control the default enabled/disabled state) would be for the builder to be able to configure a built-in addon such that it never updates from an upstream repository - ie. locking the built-in addon version for the lifetime of the build in which it shipped. Or maybe that's just a feature I'd like to see, from a nightly testing perspective... :)

Imagine an appliance-manifest.xml where two PVR and two inputstream addons have been shipped as part of a build:

<addons>
  <addon allowupdate=false enabled=false>pvr.argustv</addon>
  <addon allowupdate=false enabled=false>pvr.demo</addon>
  <addon allowupdate=false>inputstream.mpd</addon>
  <addon allowupdate=false>inputstream.smoothstream</addon>
</addons>  

Defaults for allowupdate and enabled would both be true.

None of these addons would be able to update to any newer version that may be found out on the internet. The PVR addons would be disabled at first boot (or db rebuild), while the inputstream addons would both be enabled.

Sorry, this turned into a bit of a ramble... :)

@Razzeee

This comment has been minimized.

Copy link
Member

commented May 2, 2016

Btw has this been documented or does this need the documentation needed tag?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 5, 2016

@tamland @garbear not sure if this PR or another however on a fresh install on Android every single PVR addon is enabled. This doesn't happen on a fresh Windows install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.