search

sy6sy2 / xbmc

Kodi is an award-winning free and open source home theater/media center software and entertainment hub for digital media. With its beautiful interface and powerful skinning engine, it's available for Android, BSD, Linux, macOS, iOS and Windows.
https://kodi.tv/
Other
6 stars 1 forks source link

Follow ATV-PR-depends pull request #19

Closed sy6sy2 closed 5 years ago

sy6sy2 commented 5 years ago

PR: https://github.com/xbmc/xbmc/pull/15919

sy6sy2 commented 5 years ago

All checks have passed!

sy6sy2 commented 5 years ago

I will add https://github.com/SylvainCecchetto/xbmc/commit/b1b800ac79e10bf687847ab76b437e62220e6bc6 and https://github.com/SylvainCecchetto/xbmc/commit/61c87ef77ea0383e202e473d718bb78eda57b773 to the PR today.

phunkyfish commented 5 years ago

Posted in the forum but should have posted here:

Looks like the Leia branch has been created. Should mean depends changes should be possible to merge soon. @SylvainCecchetto can you check if any of the latest commits need to added to the ATV-Depends branch?

We should be good after those and the fix from @fuzzard for multi-threaded builds when ready.

sy6sy2 commented 5 years ago

I just replied on the forum ;-)

phunkyfish commented 5 years ago

Review on this PR just posted.

sy6sy2 commented 5 years ago

Yep! I will try to resolve most of the "issues" pointed by Rechi

fuzzard commented 5 years ago

a rebase on master should fix (with some minor touchups) a number of the issues around the issues he brought up for the following

xbmc/platform/darwin/osx/network platform/osx/network xbmc/platform/darwin/osx/peripherals platform/osx/peripherals xbmc/platform/darwin/osx/powermanagement platform/darwin/osx/powermanagement xbmc/platform/darwin/osx/storage platform/osx/storage

a commit by Rechi himself was merged that relocates the above from the osx down to darwin level folders

sy6sy2 commented 5 years ago

I resolved obvious issues and rebase with upstream/master. Still some issues and questions.

sy6sy2 commented 5 years ago

I will update tools/depends/configure.ac according to the comment.

sy6sy2 commented 5 years ago

I will move tvOS windowing into separate folder like Rechi did for iOS here https://github.com/xbmc/xbmc/commit/5f4b47ad8c012b8de47ef3f212ca1f1db028e48c (Hope I will not break anything) Edit: This modification impact also Kodi C++ sources files (move and rename files). But this PR is not intended to treat about Kodi core :-/ What is our plan concerning this modification?

fuzzard commented 5 years ago

I would have followed it up as core changes were PR'd, but if he wants it done before a merge, probably have to go with it.

sy6sy2 commented 5 years ago

Maybe I misunderstand your answer. Do I have to apply modification on cmake/treedata/tvos/subdirs.txt file (like iOS) but without pushing xbmc/* modifications?

fuzzard commented 5 years ago

the windowing change requested would need to be done just the way the IOS one rechi did was.

The other 4 (network, peripherals, powermanagement, storage) have already been done and merged by rechi, so the changes would just be to update the subdirs.txt file for those to as an example

xbmc/platform/darwin/network platform/network

I would inquire if they would let the windowing/osx change be done in a followup commit when further core changes are PR'd.

sy6sy2 commented 5 years ago

Ok, I updated subdirs.txt on the PR branch.

It remains "Why make tvOS a complete separate platform rather than a special variant of the iOS platform, like it was done before?" question.

phunkyfish commented 5 years ago

I think it was @Memphiz who said to separate it out. I'm not sure how else to differentiate the platforms if we don't do this.

If you have examples of where this is needed maybe that will be sufficient explanation for Rechi. Or he may be suggest a cleaner alternative to distinguish between iOS and tvOS.

sy6sy2 commented 5 years ago

This is here https://forum.kodi.tv/showthread.php?tid=312805&pid=2842516#pid2842516

fuzzard commented 5 years ago

i tried a quick revert back to platform_os="ios" for tvos, and it doesnt play nice with the following errors. i assume it will be a cflag somewhere that we set via the tvos platform_os with the current PR

In file included from crypto/async/arch/../async_locl.h:30: crypto/async/arch/../arch/async_posix.h:45:13: error: 'setcontext' is unavailable: not available on tvOS setcontext(&n->fibre);

I know someone came across that early on in the piece, but i dont really know why not to split them as memphiz said the way its been done. Whats the benefit to keep it as a special variant? I dont quite grasp all of the cmake nuances, so maybe rechi can shed some light as to why?

sy6sy2 commented 5 years ago

I am agree with you. Most of dependencies are build in the same way on iOS or tvOS but there is exception like openSSL.

fuzzard commented 5 years ago

ifeq ($(OS), tvos) # Need to add "no-async" to avoid "'setcontext' is unavailable: not available on tvOS" error CONFIGURE=./Configure iphoneos-cross no-shared zlib no-asm no-async --prefix=$(PREFIX) endif

so, it was just the no-async flag in the configure command that we set. $(OS) is the platform_os we set, so that ifeq would need to differentiate between ios/tvos some other way if we go rechi's suggested way. As far as i can see, we dont keep any other variable we can call on that differentiates cleanly that its tvos. I guess you could go the route of if cpu and something else, but seems a worse path to me.

edit: i do see a way.

tools/target/depends/openssl/Makefile

ifeq ($(TARGET_PLATFORM), appletvos) # Need to add "no-async" to avoid "'setcontext' is unavailable: not available on tvOS" error CONFIGURE=./Configure iphoneos-cross no-shared zlib no-asm no-async --prefix=$(PREFIX) endif

# Stolen pathes here: https://gist.github.com/felix-schwarz/c61c0f7d9ab60f53ebb0 if test "$(TARGET_PLATFORM)" = "appletvos"; then \ sed -E -ie "s|^CFLAGS=-|CFLAGS=$(CFLAGS) -|" "$(PLATFORM)/Makefile"; \

sy6sy2 commented 5 years ago

In the yab branch, Memphiz sets platform_os to iOS for both Apple TV and iPhone. But he sets target_platform to appletvos or iphoneos and he uses this variable in case the build step need to be different for iOS or tvOS. https://github.com/Memphiz/xbmc/blob/yab/tools/depends/configure.ac

fuzzard commented 5 years ago

yeah, exactly what i just found. Happy to go with that if thats what they want.

phunkyfish commented 5 years ago

Doing it the same way as Memphiz means most of the binary addons would not need to be changed as they would compile as is for iOS.

phunkyfish commented 5 years ago

Also no need to modify p8platform etc. It's much cleaner.

sy6sy2 commented 5 years ago

Why not!

fuzzard commented 5 years ago

Will reduce the diff down quite drastically as well, which is a good thing. A lot of stuff was duplicated with no apparent changes required (except openssl). We can just work off the target_platform for any future requirements

sy6sy2 commented 5 years ago

Yep! Have you already started to apply this modification? (In order to avoid working on the same thing)

fuzzard commented 5 years ago

Ill leave to you, happy to review though.

phunkyfish commented 5 years ago

I asked Memphiz under the PR for his thoughts. It would be a shame to do this work and then undo again if he has a good reason to split instead.

sy6sy2 commented 5 years ago

Good idea to ask him about that. According to its answer I will try to apply modification tonight on the ATV branch.

phunkyfish commented 5 years ago

Are there many core changes which are dependent on os_platform?

We use CORE_PLATFORM_NAME_LC and CORE_SYSTEM_NAME in the cmake files for kodi core. I assume they are set using os_platform currently and would need to change to target_platform if the direction is to change it.

sy6sy2 commented 5 years ago

Is one of you have an idea on how things works for Android? I suppose that Kodi works on both Android TV and Android smartphone no? Is it comparable as iOS and tvOS choices?

Memphiz commented 5 years ago

I’s Say do as it has less impact. If my former approach is better - then go that route - just disregard my comment on the other PR then. Those are just facts about the differences. Build system side is pretty similar between iOS and tvOS - if introduction of platform_os means all the binary add on stuff needs adaption then this is a bad idea indeed.

Memphiz commented 5 years ago

Android is a single platform - doesn’t matter if android tv or not

phunkyfish commented 5 years ago

Thanks for the quick response 😉

sy6sy2 commented 5 years ago

Thank for your answer. My english is not perfect but if I understand correctly and according to what we know; it's better to use the same platform_os for iPhone and Apple TV (at lest for dependencies and binary addons)?

phunkyfish commented 5 years ago

Thank for your answer. My english is not perfect but if I understand correctly and according to what we know; it's better to use the same platform_os for iPhone and Apple TV (at lest for dependencies and binary addons)?

Yes, that is also my understanding.

fuzzard commented 5 years ago

@SylvainCecchetto just for reference, (and i couldnt sleep), ive whipped up a quick commit with all the changes to go back to the single ios platform. I dont have it neat and tidy across the several commits of your original, so probably just easier to use as a reference and confirmation more than anything as you redo.

https://github.com/fuzzard/xbmc/commit/7c036b5027732ddc152f514f7e93364a69abce28

There are some changes that i believe are better than the original, but they are minor things (iphoneossim in configure.ac) Im running a single threaded build whilst i go to sleep, fingers crossed it goes all nice.

phunkyfish commented 5 years ago

So CORE_SYSTEM_NAME is used in kodi core to differentiate iOS and tvOS. It’s also used the binary adddon cmake files. So binary addons changes may be needed regardless.

fuzzard commented 5 years ago

That isnt a problem Phunky. If you rework Toolchain.cmake.in as below, it sets core_system_name based on our platform. Should be all good

tools/depends/target/Toolchain.cmake.in

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() endif()

Memphiz commented 5 years ago

I didn’t change anything in pvr addons in the yab branch for example ...

phunkyfish commented 5 years ago

Cool

phunkyfish commented 5 years ago

But in addons like visualisation.waveform there is cmake like this:

if(NOT CORE_SYSTEM_NAME STREQUAL ios) which would need to change to:

if(NOT CORE_SYSTEM_NAME STREQUAL ios AND NOT CORE_SYSTEM_NAME STREQUAL tvos)

phunkyfish commented 5 years ago

This is ok, and PVR addons would not change I don't think.

phunkyfish commented 5 years ago

@SylvainCecchetto should probably move the dist clean for binary addons to a different PR as it’s not related to depends.

sy6sy2 commented 5 years ago

Thank you @fuzzard for your reference commit. I will begin work on that thing in a few hours.

Ok for the dict clean of binary addon. I will remove it from this PR.

kambala-decapitator commented 5 years ago

what about the defines TARGET_DARWIN_IOS and TARGET_DARWIN_TVOS? Will they be completely separate or ATV will have both? If separate, then maybe it makes sense to add a "parent" define like TARGET_DARWIN_EMBEDDED that will be defined for both iOS and tvOS? This would help to get rid of rather ugly #if defined(TARGET_DARWIN_IOS) || defined(TARGET_DARWIN_TVOS).

phunkyfish commented 5 years ago

I’m not sure the embedded define is a good idea. It should be obvious if something is iOS or tvOS or both. Embedded infers a meaning beyond iOS and tvOS (not that there is anything else).

That’s merely my opinion though.

sy6sy2 commented 5 years ago

I modified the ATV-PR-depends according to https://github.com/fuzzard/xbmc/commit/7c036b5027732ddc152f514f7e93364a69abce28. I also removed the clean/distclean addition of binary-addons.

Hope I did not break anything.

If everything is ok for you we will need to apply these changes to ATV branch?

fuzzard commented 5 years ago

Looks ok, except the windowing change. But I guess the point is the depends and not the Kodi build, so hopefully should be ok for now.

Think there will be a couple nit picks about new lines at end of some files, the removal of that dependency for firmware in the Deb control file, and maybe an indentation change on configure.ac.

phunkyfish commented 5 years ago

@SylvainCecchetto your last force push of this was incorrect. It looks like something weird happened in the rebase.