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/win32] fixes for building with abnormal configuration #10946

Merged
merged 5 commits into from
Dec 9, 2016

Conversation

Kwiboo
Copy link
Member

@Kwiboo Kwiboo commented Nov 16, 2016

Description

Building fails (on windows) when version tag is empty, app name is changed or when optical/upnp/airtunes is disabled

Motivation and Context

Fixes building of abnormal configuration on windows

How Has This Been Tested?

Compile tested with cmake 3.6.3 and visual studio 2015 on windows 10

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

@hudokkow
Copy link
Member

Thanks for looking into this. CMake changes look sane. As for the rest, we need an win dev.

ping @Paxxi
jenkins build this please

@hudokkow hudokkow added CMake Type: Fix non-breaking change which fixes an issue Platform: Windows labels Nov 16, 2016
@razzeee
Copy link
Member

razzeee commented Nov 27, 2016

jenkins build failed last time with this PR.
Let's do it again and see why.
jenkins build this please

@Kwiboo
Copy link
Member Author

Kwiboo commented Nov 28, 2016

If it helps any review of this PR:
HAS_AIRTUNES and HAS_UPNP removed from xbmc/system.h is defined earlier in the same file depending on cmake config and should not cause any changes in defines with normal cmake config.
HAS_AUDIO was removed because I could not find it used anywhere inside Kodi: https://github.com/xbmc/xbmc/search?q=HAS_AUDIO

@hudokkow
Copy link
Member

Anyone has a reason not to merge this? If so, speak in the next 24h or I'll push it in.
@afedchin, do you have time for a quick look?

@afedchin
Copy link
Member

what should I take a look on? I know nothing about cmake.

@hudokkow
Copy link
Member

The ifdeferry? It's not a CMake invention... ;)

@@ -96,6 +98,7 @@ CDetectDisc::CDetectDisc(const std::string &strPath, const bool bautorun)
bool CDetectDisc::DoWork()
{
CLog::Log(LOGDEBUG, "%s: Optical media found in drive %s", __FUNCTION__, m_strPath.c_str());
#ifdef HAS_DVD_DRIVE

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@Kwiboo
Copy link
Member Author

Kwiboo commented Nov 30, 2016

Rebased and a moved ifdef in CDetectDisc::DoWork()

Completely removing CDetectDisc was a rather obtrusive change as it also required ifdef in https://github.com/xbmc/xbmc/blob/master/xbmc/windowing/windows/WinEventsWin32.cpp#L790 so I just moved the ifdef in CDetectDisc::DoWork()

@MartijnKaijser
Copy link
Member

jenkins build this please
any further comments/objections?

@Paxxi
Copy link
Member

Paxxi commented Dec 3, 2016

Looks good to me

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Dec 3, 2016
@hudokkow
Copy link
Member

hudokkow commented Dec 4, 2016

jenkins build and merge

@phil65 phil65 merged commit bfd9d00 into xbmc:master Dec 9, 2016
@Kwiboo Kwiboo deleted the cmake-win32 branch December 10, 2016 08:43
@Kwiboo Kwiboo mentioned this pull request Jan 14, 2017
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Platform: Windows Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants