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] Rename DEPENDS_PATH to ADDON_DEPENDS_PATH #10865

Merged
merged 1 commit into from Nov 18, 2016

Conversation

@fetzerch
Copy link
Member

commented Nov 5, 2016

Description

A variable called DEPENDS_PATH is used twice in the build system:

  • In the core build system: It's set to the system depends path (via toolchain file)
  • In the binary-addon build system it's used to point to the place where addon dependencies should be built and can be found.

This change renames the variable for binary-addons to ADDON_DEPENDS_PATH. Alternative would be to rename the variable in the toolchain file. While this would be less effort, I think it's cleaner if DEPENDS_PATH is used for core depends, not addon depends.

Possible impact:
If addon devs set this var in their custom scripts, they'll have to adapt it. (@AchimTuran was doing that afaik)

Ping: @wsnipex, @notspiff, @hudokkow.

Motivation and Context

This can cause very hard to debug issues. I've ran into it while trying to compile a binary addon with our normal Toolchain file. The toolchain file accidentally tells the addon build system to compile addon depends into the system depends and even worse: to pick up depends from there.

How Has This Been Tested?

Locally on Linux and osx.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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
DEPENDS_PATH is used twice in the code build system it's set to the
system depends path (via toolchain file). In the addon system it's
used to point to the place where addon dependencies should be built.

This change renames the variable for binary-addons to
ADDON_DEPENDS_PATH. The toolchain file can then be used for local
addon builds.
@wsnipex

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

I agree in principle, lets just hope not too many addons use DEPENDS_PATH

@hudokkow

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

lgtm
I have a feeling this is going to kill win32 retroplayer builds. 😉
jenkins build this with addons please

@hudokkow

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

jenkins build this with addons please

@hudokkow

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

Working as expected, except for the usual suspects failing.

The only add-on I could find using DEPENDS_PATH on my (somewhat outdated) add-ons build env is screensavers.rsxs.

@garbear, you might want to cherry-pick hudokkow@af5326d before rebasing for beta6/RC if this goes in. Should make your life easier. 😅

jenkins retroplayer-17beta5 build with this PR + missing bits: http://jenkins.kodi.tv/view/Windows/job/WIN-32/10567/

@fetzerch fetzerch removed the RFC label Nov 16, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2016

Thanks much @hudokkow (especially for thinking about retroplayer ;))
Any objections getting this in?

@hudokkow

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

Not from me.

@wsnipex

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

fine for me

@fetzerch fetzerch merged commit d3e18c7 into xbmc:master Nov 18, 2016
1 of 4 checks passed
1 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
default You are a failure. Fix the code and try again......
Details
jenkins4kodi Yeah yeah I'll get to it when i have some time
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@fetzerch fetzerch deleted the fetzerch:cmake_addon_depends branch Nov 18, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2016

@garbear: please see @hudokkow's comment now that this is merged

@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2016

@garbear, @hudokkow: a few places got forgotten in the retroplayer patch, please pick fetzerch@690379e

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