sy6sy2 commented 5 years ago

ATV-cmake-generation branch

Branch: https://github.com/SylvainCecchetto/xbmc/tree/ATV-cmake-generation
PR: https://github.com/xbmc/xbmc/pull/15956

Description

Branch to use for the next PR to upstream/master.

This branch must be kept up to date with upstream/master by using:

git fetch upstream
git rebase upstream/master
git push --force

The only purpose of the modifications made by this branch is to be able to generate without error the project Xcode with Cmake for the Apple TV. Here we do not care if we can compile or not this project with Xcode

Commits

[tvOS] Toolchain: Only tools/depends/target/Toolchain.cmake.in file changes
[tvOS] cmake/platform
[tvOS] cmake/scripts
[tvOS] cmake/modules/FindOpenGLES.cmake
[tvOS] tools/darwin/packaging
[tvOS] xbmc/platform/darwin
At this point I am able to generate Xcode project for tvOS without error

phunkyfish commented 5 years ago

Is the plan to add each of the logical pieces from here?

Or to submit this PR up to being able to generate the Xcode project?

sy6sy2 commented 5 years ago

Good question, I don't which of the two solutions is the best.

phunkyfish commented 5 years ago

Either do I. I guess smaller is better and if anything fundamental changes in review there would be less rework.

sy6sy2 commented 5 years ago

I think you are right. So the plan would be to submit a PR having as only goal to be able to generate the kodi Xcode project for tvOS with tvOS toolchain. Then we we do not care if we can compile or not this project with Xcode.

If it is right I can:

Rename this branch to ATV-cmake-generation
Only keep commits from 1 to 6 (the minimal changes to be able to generate the Xcode project)
Open a PR if nothing is missing.

Sounds good? :-)

phunkyfish commented 5 years ago

Perfect

sy6sy2 commented 5 years ago

PR open!

kambala-decapitator commented 5 years ago

21 would also fit here

sy6sy2 commented 5 years ago

You are right kambala! Never used sips but I can look at that

kambala-decapitator commented 5 years ago

shouldn't tvOS condition be added here as well? https://github.com/SylvainCecchetto/xbmc/blob/ATV-cmake-generation/cmake/scripts/osx/Macros.cmake#L23

sy6sy2 commented 5 years ago

Oh you are right, I miss this one :-/ Thank you! I will apply changes tomorrow.

kambala-decapitator commented 5 years ago

https://github.com/SylvainCecchetto/xbmc/blob/ATV-cmake-generation/cmake/scripts/tvos/ArchSetup.cmake#L38

this must be set(CMAKE_XCODE_ATTRIBUTE_TVOS_DEPLOYMENT_TARGET "11.0") (yes, I'm lazy to create a PR)

kambala-decapitator commented 5 years ago

also this: https://github.com/SylvainCecchetto/xbmc/blob/ATV-cmake-generation/CMakeLists.txt#L342

sy6sy2 commented 5 years ago

Done, thank you for your review

sy6sy2 commented 5 years ago

Rechi comment: "I still think making tvos a complete separate platform is the wrong approach, but I would have to see the whole code to make a final decision." about set(CORE_SYSTEM_NAME tvos) in the Toolchain.

Do you think that we can keep set(CORE_SYSTEM_NAME ios) even for tvOS and still have TARGET_DARWIN_TVOS in core source files?

phunkyfish commented 5 years ago

Even though the build systems are similar I think that core wise it will diverge over time.

What approach is used in the yab branch?

sy6sy2 commented 5 years ago

The yab branch uses #if defined(TARGET_DARWIN_IOS) || defined(TARGET_DARWIN_TVOS) approach in source files. But there is no cmake folder in root folder because Memphiz directly uses a "custom" Xcode project

sy6sy2 commented 5 years ago

Ok, I think that Rechi wants that we use the same approach as Raspberry (see existing Toolchain file) by using the CORE_PLATFORM_NAME variable.

I think something like that:

[...]
elseif(OS STREQUAL ios)
  set(CMAKE_SYSTEM_NAME Darwin)
  set(CORE_SYSTEM_NAME ios)
  if(PLATFORM STREQUAL appletvos)
    set(CORE_PLATFORM_NAME appletvos)
  endif()
[...]

instead of:

[...]
elseif(OS STREQUAL ios)
  set(CMAKE_SYSTEM_NAME Darwin)
  if(PLATFORM STREQUAL appletvos)
    set(CORE_SYSTEM_NAME tvos)
  else()
    set(CORE_SYSTEM_NAME ios)
  endif()
[...]

With this approach we have nothing to modify everywhere there is a if(CORE_SYSTEM_NAME STREQUAL ios) and we only need to apply modification where there is a difference between tvOS and iOS.

What do you think about that?

phunkyfish commented 5 years ago

Sounds good 👍

phunkyfish commented 5 years ago

What is CORE_PLATFORM_NAME set to for iOS?

sy6sy2 commented 5 years ago

I think this variable is currently not set for iOS

phunkyfish commented 5 years ago

Should we ask Rechi this on the PR?

EDIT: you already did 😉

kambala-decapitator commented 5 years ago

The yab branch uses #if defined(TARGET_DARWIN_IOS) || defined(TARGET_DARWIN_TVOS) approach in source files

no, it's not entirely true. In yab both TARGET_DARWIN_IOS and TARGET_DARWIN_TVOS are defined for tvOS, that's why much less code changes were required.

phunkyfish commented 5 years ago

@kambala-decapitator

Do you like this approach:


[...]
elseif(OS STREQUAL ios)
  set(CMAKE_SYSTEM_NAME Darwin)
  set(CORE_SYSTEM_NAME ios)
  if(PLATFORM STREQUAL appletvos)
    set(CORE_PLATFORM_NAME appletvos)
  endif()
[...]```

kambala-decapitator commented 5 years ago

@phunkyfish I have no problem with it, as I'm not a fan of cmake at all :) whatever others think is best, I agree. The only thing: shouldn't CORE_PLATFORM_NAME be tvos, not appletvos?

I'm more interested in source code defines/macros.

sy6sy2 commented 5 years ago

no, it's not entirely true. In yab both TARGET_DARWIN_IOS and TARGET_DARWIN_TVOS are defined for tvOS, that's why much less code changes were required.

Oh yes! Maybe can we follow this approach and also define TARGET_DARWIN_IOS and TARGET_DARWIN_TVOS in our case?

@phunkyfish I have no problem with it, as I'm not a fan of cmake at all :) whatever others think is best, I agree. The only thing: shouldn't CORE_PLATFORM_NAME be tvos, not appletvos?

Maybe, absolutely no idea which one is best ;-)

kambala-decapitator commented 5 years ago

Maybe can we follow this approach and also define TARGET_DARWIN_IOS and TARGET_DARWIN_TVOS in our case?

I don't like this approach tbh, that's why I once mentioned adding a "parent" define for both. But then in some review rechi reminded of the SDK-defined TARGET_OS_IPHONE which is set for both iOS and tvOS, so I think this is the way to go in the sources instead of the currently used "iOS or tvOS" approach. Unless kodi code style requires using only custom defines...

Although, considering that tvOS would be a "variant" of iOS, the yab approach sounds valid.

Maybe, absolutely no idea which one is best ;-)

I'd personally go with tvos

sy6sy2 commented 5 years ago

The TARGET_OS_IPHONE seems to be the cleanest way. But this modification would have an impact on both iOS and tvOS existing code, isn't is? Don't know if Rechi is agree to replace all TARGET_DARWIN_IOS by TARGET_OS_IPHONE in the code. Or maybe I misunderstood a thing?

kambala-decapitator commented 5 years ago

you're absolutely correct :) you could already ask Rechi about that.

for now we can follow yab approach, and after our cmake and core PRs are merged, I can open a separate one that uses TARGET_OS_IPHONE.

phunkyfish commented 5 years ago

In the last PR he referenced a set of defines. Let me look it up.

phunkyfish commented 5 years ago

https://github.com/xbmc/xbmc/pull/15919#discussion_r276098856

sy6sy2 commented 5 years ago

Yes, that's what kambala was referring to I think.

I will ask Rechi about his plan directly in the PR discussion.

phunkyfish commented 5 years ago

That didn’t really work.

From Rechi:

The reason I'm asking is the following comment from TargetConditionals.h in the platforms SDKs. Also most of the build system code looks the same. I was thinking to handle it like the different windowing for linux.

TARGET_OS_* 
These conditionals specify in which Operating System the generated code will
run.  Indention is used to show which conditionals are evolutionary subclasses.  

The MAC/WIN32/UNIX conditionals are mutually exclusive.
The IOS/TV/WATCH conditionals are mutually exclusive.

    TARGET_OS_WIN32           - Generated code will run under 32-bit Windows
    TARGET_OS_UNIX            - Generated code will run under some Unix (not OSX) 
    TARGET_OS_MAC             - Generated code will run under Mac OS X variant
       TARGET_OS_OSX          - Generated code will run under OS X devices
       TARGET_OS_IPHONE          - Generated code for firmware, devices, or simulator
          TARGET_OS_IOS             - Generated code will run under iOS 
          TARGET_OS_TV              - Generated code will run under Apple TV OS
          TARGET_OS_WATCH           - Generated code will run under Apple Watch OS
          TARGET_OS_BRIDGE          - Generated code will run under Bridge devices
       TARGET_OS_SIMULATOR      - Generated code will run under a simulator
       TARGET_OS_EMBEDDED       - Generated code for firmware

    TARGET_IPHONE_SIMULATOR   - DEPRECATED: Same as TARGET_OS_SIMULATOR
    TARGET_OS_NANO            - DEPRECATED: Same as TARGET_OS_WATCH

  +------------------------------------------------+
  |                TARGET_OS_MAC                   |
  | +---+  +-------------------------------------+ |
  | |   |  |          TARGET_OS_IPHONE           | |
  | |OSX|  | +-----+ +----+ +-------+ +--------+ | |
  | |   |  | | IOS | | TV | | WATCH | | BRIDGE | | |
  | |   |  | +-----+ +----+ +-------+ +--------+ | |
  | +---+  +-------------------------------------+ |
  +------------------------------------------------+

sy6sy2 commented 5 years ago

That didn’t really work.

What do you mean?

phunkyfish commented 5 years ago

I tried to link to the comment but it just brought you to the entire PR instead. Copy paste and worked much better.

sy6sy2 commented 5 years ago

Ah ok!

So maybe we can wait for Rechi's answer in the PR.

Edit: I updated my comment in the PR discussion

sy6sy2 commented 5 years ago

So, what I understand from Rechi answer:

Only one PR with ALL modification for Kodi tvOS support
Usage of set(CORE_SYSTEM_NAME ios) + set(CORE_PLATFORM_NAME tvos) in the Toolchain for Apple TV build
Concerning #define preprocessor I am not very sure to understand...

phunkyfish commented 5 years ago

I think he means that TARGET_DARWIN_IPHONE is set for either iOS or tvOS to show they are related as well as their specific include.

He asked if there was a better name for TARGET_DARWIN_IPHONE as it’s not exactly suited as a family name for tvOS.

kambala-decapitator commented 5 years ago

@phunkyfish yes, correct. That's why I thought of the name like TARGET_DARWIN_EMBEDDED. Or maybe TARGET_DARWIN_NONDESKTOP would also do.

phunkyfish commented 5 years ago

Let’s ask Rechi what he thinks.

EDIT: asked on PR.

phunkyfish commented 5 years ago

TARGET_DARWIN_EMBEDDED it is.

So for tvOS that should be defined to denote it’s either iOS or tvOS and also define TARGET_DARWIN_TVOS.

Does that make sense @SylvainCecchetto?

phunkyfish commented 5 years ago

So based on your earlier 3 points they should be:

Only one PR with ALL modification for Kodi tvOS support
Usage of set(CORE_SYSTEM_NAME ios) + set(CORE_PLATFORM_NAME tvos) in the Toolchain for Apple TV build
Concerning #define preprocessor TARGET_DARWIN_EMBEDDED and TARGET_DARWIN_TVOS for tvOS (and by inference iOS would define TARGET_DARWIN_EMBEDDED and TARGET_DARWIN_IOS)

sy6sy2 commented 5 years ago

Sorry I just come back. The plan seems clear to me, thank you for the 3 points above!

Maybe I should rename ATV-cmake-generation with other name like ATV-PR and use this branch to the final PR with ALL modifications? May I keep the current PR open?

phunkyfish commented 5 years ago

I don’t think the branch name matters much. Just rename the PR I’d say and add everything else.

phunkyfish commented 5 years ago

Post here when you are happy with the branch and I’ll kick off a test build.

sy6sy2 commented 5 years ago

Ok no problem, thank you ;-)

kambala-decapitator commented 5 years ago

cmake/treedata/ios/subdirs.txt also needs to be copied for tvOS and modified accordingly, I guess

sy6sy2 commented 5 years ago

Hi all! So, I'm sure that there is:

Errors
Missing things
Wrong order commits

Anyway, this is a first try xD but this branch is up to date with upstream/master (rebase).

I will try to compile everything from scratch also.

Edit: As expected, dependencies build still works

Edit2: I am currently fixing things that Rechi highlighted, I will force push soon on this branch.

sy6sy2 commented 5 years ago

Don't know if I broke something but I cannot build Kodi ATV with this branch ; error on cores/AudioEngine/Skins/AESinkDARWINIOS.mm file

kambala-decapitator commented 5 years ago

would you post the error? :)

sy6sy2 commented 5 years ago

/Users/sylvain/Kodi_dev/kodi/xbmc/cores/AudioEngine/Sinks/AESinkDARWINIOS.mm:105:19: 
Unknown type name 'AudioSessionPropertyID'; did you mean 'AudioServicesPropertyID'?

/Users/sylvain/Kodi_dev/kodi/xbmc/cores/AudioEngine/Sinks/AESinkDARWINIOS.mm:284:45:
Use of undeclared identifier 'kAudioSessionProperty_PreferredHardwareIOBufferDuration'

15 errors

sy6sy2 / xbmc

ATV-cmake-generation branch: next PR #42

ATV-cmake-generation branch

Description

Commits

21 would also fit here