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

Improve rebranding situation for binary addon building #17810

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

a1rwulf
Copy link
Member

@a1rwulf a1rwulf commented May 6, 2020

Description

This improves the ability to use binary addons in rebrands.

How Has This Been Tested?

Tested by building a rebranded version for Android.
See following repos for an example:
https://github.com/a1rwulf/xbmc/commits/addons-rebrand-testing
https://github.com/a1rwulf/pvr.iptvsimple/commits/Playa

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)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@a1rwulf a1rwulf added Type: Improvement non-breaking change which improves existing functionality CMake v19 Matrix labels May 6, 2020
@a1rwulf a1rwulf added this to the Matrix 19.0-alpha 1 milestone May 6, 2020
Copy link
Member

@AlwinEsch AlwinEsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is OK for me 👍.

As long there the addons manually becomes changed for 😄.
To have it here is OK, only to use then hack ways on addons to allow it without changes, would not good for me.

@@ -156,8 +156,11 @@ if(ADDON_SRC_PREFIX)
message(STATUS "Overriding addon source directory prefix: ${ADDON_SRC_PREFIX}")
endif()

include(../../cmake/scripts/common/Macros.cmake)
Copy link
Member

@AlwinEsch AlwinEsch May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use then here also like on line 83, with:
include(${CORE_SOURCE_DIR}/cmake/scripts/common/Macros.cmake)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, I tried different other variables but somehow missed this one...

@a1rwulf
Copy link
Member Author

a1rwulf commented May 7, 2020

Is OK for me .

As long there the addons manually becomes changed for .
To have it here is OK, only to use then hack ways on addons to allow it without changes, would not good for me.

Yeah - no plans to go any further with it.
I just think that our foundation should fully support changes of the app name.
Rebranders need to take care of getting their supported addons to work.

@a1rwulf
Copy link
Member Author

a1rwulf commented May 7, 2020

jenkins build this with addons please

@a1rwulf
Copy link
Member Author

a1rwulf commented May 7, 2020

@AlwinEsch most addons worked fine, so I assume my PR doesn't cause any breakage:
https://jenkins.kodi.tv/job/BuildMultiWithAddons-PR/60/

@a1rwulf
Copy link
Member Author

a1rwulf commented May 12, 2020

I'll merge this soon if there are no objections.

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't improve the rebranding situation, it makes it worse. With this PR you have to change every binary add-on.

@a1rwulf
Copy link
Member Author

a1rwulf commented May 12, 2020

This doesn't improve the rebranding situation, it makes it worse. With this PR you have to change every binary add-on.

That's not correct, you have to change every binary addon already.
But at the moment you need to patch our binary addon build system additionally because it's not prepared for rebranding at all.

@a1rwulf
Copy link
Member Author

a1rwulf commented May 12, 2020

Example:
You change the APP_NAME in version.txt and build your rebrand.
On Linux this will install into /usr/local/lib/rebrandname and /usr/local/share/rebrandname.
However binary-addons look for /usr/local/lib/kodi and /usr/local/share/kodi.
So you end up with 2 situations:

  • Build of binary addons complains cause it can't find the kodi package
  • It accidently picks up all the items from your kodi installation, which is even worse

This PR makes sure it finds everything from your rebrand instead of mixing everything from kodi into the builds, which is an improvement already!

@wsnipex
Copy link
Member

wsnipex commented May 13, 2020

with this change every call to find_package(kodi) in addons will fail

@a1rwulf
Copy link
Member Author

a1rwulf commented May 13, 2020

with this change every call to find_package(kodi) in addons will fail

Why?
If that would be the case, then the build with addons would have failed.
I've tested building kodi with an addon for android and the rebranded sample.
Both work.

@a1rwulf
Copy link
Member Author

a1rwulf commented May 13, 2020

After internal discussion and some more digging.
IMHO searching for KodiConfig.cmake by binary addons is broken for linux because of:
https://github.com/xbmc/xbmc/blob/master/cmake/scripts/linux/Install.cmake#L39

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 3, 2023
@jenkins4kodi
Copy link
Contributor

@a1rwulf this needs a rebase

Copy link

github-actions bot commented Aug 5, 2024

This pull request is now marked stale because it has been open over a year without activity. Remove the stale label or add a comment to reset the stale state.

@github-actions github-actions bot added the Stale label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Rebase needed PR that does not apply/merge cleanly to current base branch Stale Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants