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

[cmake] Fix double install of skins #10656

Merged
merged 3 commits into from Oct 11, 2016

Conversation

@fetzerch
Copy link
Member

commented Oct 8, 2016

Skins were installed twice on linux (to the same place) so that make uninstall reported errors.

#10398 contained a commit that ensured that skins are not installed twice on linux. The PR was reverted in #10454 because it broke skin packaging on windows.

After investigation, the second "install" on windows covered the issue, that the windows build script created empty xbt files. Those were overridden by cmake again so that it wasn't noticed.

Ping: @Paxxi

@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2016

jenkins build this please

@hudokkow

This comment has been minimized.

Copy link
Member

commented Oct 8, 2016

win32 error: Error in script "genNsisInstaller.nsi" on line 179 -- aborting creation process
https://github.com/xbmc/xbmc/blob/master/project/Win32BuildSetup/genNsisInstaller.nsi#L179

@Paxxi

This comment has been minimized.

Copy link
Member

commented Oct 8, 2016

GenNsisInstaller expects the skin files in win32buildsetup/BUILD_WIN32 so it likely needs to be modified to find the ones built by cmake

@fetzerch fetzerch force-pushed the fetzerch:cmake_install_cleanup branch from 438d242 to 0429214 Oct 9, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2016

Next round. http://jenkins.kodi.tv/job/WIN-32/10218/

I'm not sure if the change to genNsisIncludes.bat is correct, because also the current installer doesn't show skin related options.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 9, 2016

@fetzerch nevermind

@fetzerch fetzerch force-pushed the fetzerch:cmake_install_cleanup branch 2 times, most recently from fe5d74a to b4642b8 Oct 9, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2016

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 9, 2016

APK size looks normal

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta4 milestone Oct 9, 2016
fetzerch added 3 commits Oct 8, 2016
Skins are already handled in main CMakeLists.txt. Audioencoders
are listed twice in installdata.
CMake's pack-skin target already packages the skin (builds the xbt files).
The current scripts produced empty xbt files, and the final package only
worked because of an error in CMake which was overriding the xbt file
again with a working version.
@fetzerch fetzerch force-pushed the fetzerch:cmake_install_cleanup branch from b4642b8 to 475b7ba Oct 9, 2016
@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2016

good to go? quite surprisingly it didn't break android skin or binary addon packaging ;)

@fetzerch fetzerch merged commit 01c5bc9 into xbmc:master Oct 11, 2016
2 checks passed
2 checks passed
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_install_cleanup branch Oct 11, 2016
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.