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 set of APP_RENDER_SYSTEM on KodiConfig.cmake #15642

Merged
merged 1 commit into from Mar 21, 2019

Conversation

Projects
None yet
4 participants
@AlwinEsch
Copy link
Member

commented Mar 1, 2019

Description

In the implementation of GLES under Windows found the foolish finding of OpenGL. This seems to exist systematically and makes it a little harder. Then found that "APP_RENDER_SYSTEM" of "KodiConfig.cmake" is not defined correctly (empty defined).

This request fixes this and uses the "platform" folder.
Parts of "Platform.cmake" are also included in PrepareEnv.cmake.

But good to include them also in PrepareEnv.cmake to fix for addons?

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

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
file(GLOB _platformnames RELATIVE ${CORE_SOURCE_DIR}/cmake/platform/${CORE_SYSTEM_NAME}/
${CORE_SOURCE_DIR}/cmake/platform/${CORE_SYSTEM_NAME}/*.cmake)
string(REPLACE ".cmake" " " _platformnames ${_platformnames})
message(FATAL_ERROR "invalid CORE_PLATFORM_NAME: ${CORE_PLATFORM_NAME_LC}\nValid platforms: ${_platformnames}")

This comment was marked as outdated.

Copy link
@AlwinEsch

AlwinEsch Mar 1, 2019

Author Member

With this c&p part from Platform.cmake I'm not sure, are still all supported OS types defined in ./cmake/platform/ folder?

@AlwinEsch AlwinEsch force-pushed the AlwinEsch:fix-addon-cmake branch 3 times, most recently from 98fa02f to e77f7c3 Mar 1, 2019

@AlwinEsch AlwinEsch added this to the Leia 18.2-rc1 milestone Mar 2, 2019

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

How is the state about them, OK to bring in?

set(PLATFORM_USED_SOURCE_DIR ${CORE_SOURCE_DIR})
else()
set(PLATFORM_USED_SOURCE_DIR ${CMAKE_SOURCE_DIR})
endif()

This comment has been minimized.

Copy link
@wsnipex

wsnipex Mar 10, 2019

Member

Please explain this block. Both kodi and addons have CORE_SOURCE_DIR defined and pointing to the same main kodi source root, so we should be able to use CORE_SOURCE_DIR directly

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Mar 10, 2019

Author Member

Is a bit a hen and egg problem.

On Kodi build it must be included on begin and CORE_SOURCE_DIR not set:
https://github.com/xbmc/xbmc/blob/master/CMakeLists.txt#L32
This then becomes set by https://github.com/xbmc/xbmc/blob/master/CMakeLists.txt#L36, and I think https://github.com/xbmc/xbmc/blob/master/cmake/scripts/common/Macros.cmake#L8 need a bit the defines from Platform.cmake.

EDIT: Am unsure to change the place and not inadvertently bring in a hidden mistake

This comment has been minimized.

Copy link
@wsnipex

wsnipex Mar 11, 2019

Member

from a quick look it shouldn't be a problem to set CORE_SOURCE_DIR on top of the main CMakeLists.txt

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Mar 11, 2019

Author Member

Have checked, it does not work in other way without a bigger rewrite.

  • Platform.cmake define the CORE_SYSTEM_NAME
  • CORE_SYSTEM_NAME required by Macros.cmake
  • CORE_SOURCE_DIR on common/Macros.cmake not allowed to set before, otherwise ${CORE_SYSTEM_NAME}/Macros.cmake is not included

Not sure if this request fix something on my screensavers.rsxs Jenkins problem, but maybe it bring something :-)

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Mar 19, 2019

Author Member

@wsnipex what you think about?

This comment has been minimized.

Copy link
@wsnipex

wsnipex Mar 20, 2019

Member

I still think this should be refactored, but since that needs deeper changes we can leave it as is for v18. But please add a TODO comment that this should be fixed for v19

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Mar 20, 2019

Author Member

Also heard that v19 is planning to restructure the CMake for addons. Maybe we could integrate it with it (or maybe it's already ;-)). Otherwise, I will see what can be done.

@AlwinEsch AlwinEsch force-pushed the AlwinEsch:fix-addon-cmake branch from e77f7c3 to 3f0b2e7 Mar 14, 2019

@AlwinEsch AlwinEsch force-pushed the AlwinEsch:fix-addon-cmake branch from b08b6a9 to 84d607d Mar 19, 2019

[cmake] fix set of APP_RENDER_SYSTEM on KodiConfig.cmake
In the implementation of GLES under Windows found the foolish finding of OpenGL. This seems to exist systematically and makes it a little harder. Then found that "APP_RENDER_SYSTEM" of "KodiConfig.cmake" is not defined correctly (empty defined).

This request fixes this and uses the "platform" folder.
Parts of "Platform.cmake" are also included in PrepareEnv.cmake.

TODO: Refactor on v19 to remove the if/else usage

@AlwinEsch AlwinEsch force-pushed the AlwinEsch:fix-addon-cmake branch from 84d607d to 34d0c4a Mar 20, 2019

@AlwinEsch AlwinEsch merged commit 36dad43 into xbmc:master Mar 21, 2019

1 check passed

default You're awesome. Have a cookie
Details

@AlwinEsch AlwinEsch deleted the AlwinEsch:fix-addon-cmake branch Mar 21, 2019

@Rechi

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@AlwinEsch This broke compiling binary addons via depends for GBM and Wayland platform.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2019

Can you give the error output, if not easy to fix this request becomes reverted from me.

@Rechi

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

CMake Error at /home/jenkins/workspace/LINUX-64-GBM/cmake/platform/linux/gbm.cmake:13 (message):
  You need to decide whether you want to use GL- or GLES-based rendering in
  combination with the GBM windowing system.  Please set GBM_RENDER_SYSTEM to
  either "gl" or "gles".  For normal desktop systems, you will usually want
  to use "gl".
Call Stack (most recent call first):
  /home/jenkins/workspace/LINUX-64-GBM/cmake/scripts/common/Platform.cmake:37 (include)
  /home/jenkins/workspace/LINUX-64-GBM/tools/depends/target/binary-addons/x86_64-linux-gnu-debug/build/depends/lib/kodi/PrepareEnv.cmake:3 (include)
  CMakeLists.txt:186 (include)

AlwinEsch added a commit to AlwinEsch/kodi that referenced this pull request Mar 23, 2019

[cmake] partly revert xbmc#15642 to have wayland and GBM working
With the include of Platform.cmake to have for addons is the
GBM_RENDER_SYSTEM not defined, this becomes set from
"./tools/depends/target/Toolchain.cmake" but on a other place.

This remove the change, but after the release must be the cmake
workflow cleaned up. There are some hen and egg problems and
maybe some hidden faults where a value becomes checked which
becomes never available.

AlwinEsch added a commit that referenced this pull request Mar 23, 2019

Merge pull request #15804 from AlwinEsch/partly-fix-cmake
[cmake] partly revert #15642 to have wayland and GBM working

pkerling added a commit to pkerling/xbmc that referenced this pull request Mar 28, 2019

[cmake] partly revert xbmc#15642 to have wayland and GBM working
With the include of Platform.cmake to have for addons is the
GBM_RENDER_SYSTEM not defined, this becomes set from
"./tools/depends/target/Toolchain.cmake" but on a other place.

This remove the change, but after the release must be the cmake
workflow cleaned up. There are some hen and egg problems and
maybe some hidden faults where a value becomes checked which
becomes never available.

fuzzard added a commit to fuzzard/xbmc that referenced this pull request Apr 13, 2019

[cmake] partly revert xbmc#15642 to have wayland and GBM working
With the include of Platform.cmake to have for addons is the
GBM_RENDER_SYSTEM not defined, this becomes set from
"./tools/depends/target/Toolchain.cmake" but on a other place.

This remove the change, but after the release must be the cmake
workflow cleaned up. There are some hen and egg problems and
maybe some hidden faults where a value becomes checked which
becomes never available.

fuzzard added a commit to fuzzard/xbmc that referenced this pull request Apr 15, 2019

[cmake] partly revert xbmc#15642 to have wayland and GBM working
With the include of Platform.cmake to have for addons is the
GBM_RENDER_SYSTEM not defined, this becomes set from
"./tools/depends/target/Toolchain.cmake" but on a other place.

This remove the change, but after the release must be the cmake
workflow cleaned up. There are some hen and egg problems and
maybe some hidden faults where a value becomes checked which
becomes never available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.