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

[cmake] Win-Binary-Addon fixes + Handle "select" as setting type for binary addons #10875

Merged
merged 3 commits into from Nov 30, 2016

Conversation

Projects
None yet
9 participants
@fetzerch
Member

fetzerch commented Nov 6, 2016

A couple of changes from the retroplayer branch.

Description

  • Two changes related to binary addons. Ping @hudokkow, @wsnipex or @Paxxi.
  • One change for an addon callback. Not sure whom to ping here.

Motivation and Context

Upstreaming them, to reduce rebase work for @garbear.

How Has This Been Tested?

Locally and on jenkins.

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

Jenkins build (retroplayer branch): http://jenkins.kodi.tv/job/WIN-32/10573/

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi Nov 6, 2016

Member

Will this change the setting for the Kodi repo or just some addon?

*edit I was referring to the git config core.autocrlf setting

Member

Paxxi commented Nov 6, 2016

Will this change the setting for the Kodi repo or just some addon?

*edit I was referring to the git config core.autocrlf setting

@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser

MartijnKaijser Nov 6, 2016

Member

it's a setting option that we already have for python addons but it seems binary didn't had it added yet?

edit:
iirc we also have multiselect

Member

MartijnKaijser commented Nov 6, 2016

it's a setting option that we already have for python addons but it seems binary didn't had it added yet?

edit:
iirc we also have multiselect

@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 6, 2016

Member

cdac36e is only for binary addon dependencies. Those are downloaded as external projects and usually you don't ever change anything in there. that's also why we need only the last change.

Member

fetzerch commented Nov 6, 2016

cdac36e is only for binary addon dependencies. Those are downloaded as external projects and usually you don't ever change anything in there. that's also why we need only the last change.

@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 6, 2016

Member

@MartijnKaijser: Yes exactly. It's mentioned in the wiki http://kodi.wiki/view/Add-on_settings#type.3D.22select.22 but didn't work for binary addons.

Member

fetzerch commented Nov 6, 2016

@MartijnKaijser: Yes exactly. It's mentioned in the wiki http://kodi.wiki/view/Add-on_settings#type.3D.22select.22 but didn't work for binary addons.

@hudokkow

This comment has been minimized.

Show comment
Hide comment
@hudokkow

hudokkow Nov 6, 2016

Member

cmake changes look good.

btw, 5+ hours to build win32 with retroplayer addons? ouch!

Member

hudokkow commented Nov 6, 2016

cmake changes look good.

btw, 5+ hours to build win32 with retroplayer addons? ouch!

@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 6, 2016

Member

Yep because it uses only one core. #10753 should speed it up.

Member

fetzerch commented Nov 6, 2016

Yep because it uses only one core. #10753 should speed it up.

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi Nov 6, 2016

Member

No, just no, doing ugly tricks in our build system to work around a broken application (patch) to try and patch something from a git repo is just begging for trouble.

Fork the repo, patch it, upstream it or whatever but if we start down this road it will only end in tears and gothic synth pop while eating ice cream.

Member

Paxxi commented Nov 6, 2016

No, just no, doing ugly tricks in our build system to work around a broken application (patch) to try and patch something from a git repo is just begging for trouble.

Fork the repo, patch it, upstream it or whatever but if we start down this road it will only end in tears and gothic synth pop while eating ice cream.

@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 6, 2016

Member

What we currently have is just broken and only works accidentally if you don't have autocrlf=true in your global gitconfig. So if we want to keep the possibility of applying patches, this is necessary.

And I don't think anyone wants to maintain local forks of repos for small patches like this: https://github.com/notspiff/inputstream.rtmp/tree/master/depends/common/librtmp
Because you'll have to bump the repo manually.

Member

fetzerch commented Nov 6, 2016

What we currently have is just broken and only works accidentally if you don't have autocrlf=true in your global gitconfig. So if we want to keep the possibility of applying patches, this is necessary.

And I don't think anyone wants to maintain local forks of repos for small patches like this: https://github.com/notspiff/inputstream.rtmp/tree/master/depends/common/librtmp
Because you'll have to bump the repo manually.

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi Nov 6, 2016

Member

We still have to bump the repo manually for new versions so it's no more work.

Member

Paxxi commented Nov 6, 2016

We still have to bump the repo manually for new versions so it's no more work.

@garbear

This comment has been minimized.

Show comment
Hide comment
@garbear

garbear Nov 6, 2016

Member

i've forked a few cores and it's more work that adding a .patch file because you have to manually rebase often, whereas the patch is always applied to master

Member

garbear commented Nov 6, 2016

i've forked a few cores and it's more work that adding a .patch file because you have to manually rebase often, whereas the patch is always applied to master

@wsnipex

This comment has been minimized.

Show comment
Hide comment
@wsnipex

wsnipex Nov 6, 2016

Member

the option to apply patches is an integral part of the addons build system, we need this to work.
Note that upstream sources are often not repositories but simple tarballs. We're definitely not going to put those into repos just to apply minimal patches that are only needed for our build system.

Member

wsnipex commented Nov 6, 2016

the option to apply patches is an integral part of the addons build system, we need this to work.
Note that upstream sources are often not repositories but simple tarballs. We're definitely not going to put those into repos just to apply minimal patches that are only needed for our build system.

@hudokkow

This comment has been minimized.

Show comment
Hide comment
@hudokkow

hudokkow Nov 6, 2016

Member

Right now my github account holds 180+ repos. 180 are Kodi related.
Keeping local forks of every repo out there in need of a patch is crazy.

Member

hudokkow commented Nov 6, 2016

Right now my github account holds 180+ repos. 180 are Kodi related.
Keeping local forks of every repo out there in need of a patch is crazy.

@akva2

This comment has been minimized.

Show comment
Hide comment
@akva2

akva2 Nov 7, 2016

Contributor

look to peeps who have been doing this for a long time. look at any /random linux distro. they have been doing this for years on end. and they all use orig tarballs and patchery in the packaging. look no further than your own repo. depends is all tarballs and patchery (not counting the windows stuff which is just meh all over). do you really think it's a good idea to go for a model different from what every single linux distro out there does?

you can use whatever tool you want locally to generate them patches. such as git. i for one often do; import tarball1 in a git, apply patches, import tarball2 (the new tarball), rebase patch branch on that and voila patches. but that should not affect deployment.

Contributor

akva2 commented Nov 7, 2016

look to peeps who have been doing this for a long time. look at any /random linux distro. they have been doing this for years on end. and they all use orig tarballs and patchery in the packaging. look no further than your own repo. depends is all tarballs and patchery (not counting the windows stuff which is just meh all over). do you really think it's a good idea to go for a model different from what every single linux distro out there does?

you can use whatever tool you want locally to generate them patches. such as git. i for one often do; import tarball1 in a git, apply patches, import tarball2 (the new tarball), rebase patch branch on that and voila patches. but that should not affect deployment.

@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 7, 2016

Member

I'll try to upstream a feature for the autocrlf change as well: https://gitlab.kitware.com/cmake/cmake/merge_requests/240

@MartijnKaijser: Want me to add all missing addon config options then?

Member

fetzerch commented Nov 7, 2016

I'll try to upstream a feature for the autocrlf change as well: https://gitlab.kitware.com/cmake/cmake/merge_requests/240

@MartijnKaijser: Want me to add all missing addon config options then?

fetzerch added some commits Nov 5, 2016

[game-settings] Add 'select' as setting type
The newly added settings generator generates settings with type
'select'. These were then not loaded correctly.
[addon/depends] Handle autocrlf for depends
On Windows, if we have addon dependencies, we need to forcefully disable
autocrlf because otherwise the patches will not apply.

Patches are typically in LF format (and the build system automatically adds
--binary if necessary). But the patches will also not apply if the source files
are in CLRF which can happen if the user has core.autocrlf=true in the
global git config.

A better way of fixing would be to pass the config already to clone:
https://gitlab.kitware.com/cmake/cmake/issues/15799
[addon/depends] Clone only the last commit of dependencies
This has been introduced with CMake 3.6 but is ignored on older versions.
@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 30, 2016

Member

jenkins build this with addons please

Member

fetzerch commented Nov 30, 2016

jenkins build this with addons please

@fetzerch

This comment has been minimized.

Show comment
Hide comment
@fetzerch

fetzerch Nov 30, 2016

Member

android build error unrelated

Member

fetzerch commented Nov 30, 2016

android build error unrelated

@fetzerch fetzerch merged commit f0bc0a1 into xbmc:master Nov 30, 2016

2 of 4 checks passed

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
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fetzerch fetzerch deleted the fetzerch:cmake_retroplayer_upstream branch Nov 30, 2016

@hudokkow hudokkow added the v18 Leia label Nov 30, 2016

@hudokkow hudokkow added this to the L 18.0-alpha1 milestone Nov 30, 2016

@ronie

This comment has been minimized.

Show comment
Hide comment
@ronie

ronie Dec 3, 2016

Member

@fetzerch
building of binary addons now fails on ubuntu 16.10, which uses cmake 3.5.2
/bin/sh: 1: GIT_SHALLOW: not found

afaik GIT_SHALLOW was introduced in cmake 3.6
if so, this check needs to be adjusted i think?:
f095fe3#diff-792731e8f7b3b5e722d1697dd3998296R164

Member

ronie commented Dec 3, 2016

@fetzerch
building of binary addons now fails on ubuntu 16.10, which uses cmake 3.5.2
/bin/sh: 1: GIT_SHALLOW: not found

afaik GIT_SHALLOW was introduced in cmake 3.6
if so, this check needs to be adjusted i think?:
f095fe3#diff-792731e8f7b3b5e722d1697dd3998296R164

@@ -161,6 +161,10 @@ function(add_addon_depends addon searchpath)
PATCH_COMMAND ${PATCH_COMMAND}
"${INSTALL_COMMAND}")
if(CMAKE_VERSION VERSION_GREATER 3.5)
list(APPEND EXTERNALPROJECT_SETUP GIT_SHALLOW 1)

This comment has been minimized.

@Rechi

Rechi Jun 4, 2017

Member

@fetzerch this conflicts with https://github.com/xbmc/xbmc/blob/a18701e/cmake/addons/depends/README#L17-L18
<git-revision> can't be a commit which is not the last commit for a branch

@Rechi

Rechi Jun 4, 2017

Member

@fetzerch this conflicts with https://github.com/xbmc/xbmc/blob/a18701e/cmake/addons/depends/README#L17-L18
<git-revision> can't be a commit which is not the last commit for a branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment