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

[ios] set minimum ios version to 11.0, fix deprecations and cleanup dependencies #17323

Merged
merged 3 commits into from
Mar 2, 2020

Conversation

phunkyfish
Copy link
Contributor

@phunkyfish phunkyfish commented Feb 5, 2020

Description

Set minimum supported iOS version of 11.0. Removes support for 32bit iOS devices (which covers iPad 1-4 and iPhone 5 and earlier).

Motivation and Context

Simplifies patching a logic around the iOS platform, especially around addons.

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)

Not quite sure how to categorise this.

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

@phunkyfish phunkyfish added WIP PR that is still being worked on Component: Depends CMake v19 Matrix labels Feb 5, 2020
@phunkyfish
Copy link
Contributor Author

Early days for this one. Plenty more to do.

@kambala-decapitator
Copy link
Contributor

  • tools/depends/target/fontconfig/fix-aarch64_atomics.patch:17 (if this is Kodi's patch)
  • xbmc/platform/darwin/ios/XBMCController.mm:710

@phunkyfish
Copy link
Contributor Author

phunkyfish commented Feb 5, 2020

@kambala-decapitator the first is still required for 64bit if I'm not mistaken. For the second it is already set to 11.0 which is what it should be, correct?

@kambala-decapitator
Copy link
Contributor

oh right, first is patch :)

in the second you should remove the condition, as it's always true now.

@phunkyfish
Copy link
Contributor Author

I assume I can condense it to this:
return UIEdgeInsetsInsetRect(self.view.bounds, m_window.safeAreaInsets);

@phunkyfish phunkyfish changed the title WIP - [ios] set minimum ios version to 11.0, fix deprecations and cleanup dependencies [ios] set minimum ios version to 11.0, fix deprecations and cleanup dependencies Feb 5, 2020
@phunkyfish phunkyfish requested a review from Rechi February 5, 2020 21:45
@phunkyfish
Copy link
Contributor Author

phunkyfish commented Feb 5, 2020

@Rechi should removing ios (armv7) from Jenkins be part of the PR or would that be something that must be done manually on/before merge?

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

if test "$use_platform" = "tvos"; then
use_cpu=arm64
target_platform=appletvos
platform_min_version="$target_platform-version-min=11.0"
else
# setup which cpu to use
if test "x$use_cpu" = "xauto"; then
use_cpu=arm64
fi
target_platform=iphoneos
platform_min_version="$target_platform-version-min=11.0"
fi

Can be rewritten to something like

        use_cpu=arm64
        platform_min_version="$target_platform-version-min=11.0"
        if test "$use_platform" = "tvos"; then
          target_platform=appletvos
        else
          target_platform=iphoneos
        fi

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

if test "$use_platform" = "tvos"; then
case $use_sdk in
11.*);;
12.*);;
13.*);;
*)
AC_MSG_ERROR(error in configure of --with-sdk=$use_sdk)
;;
esac
else
case $use_sdk in
11.*);;
12.*);;
13.*);;
*)
AC_MSG_ERROR(error in configure of --with-sdk=$use_sdk)
;;
esac
fi

Can be rewritten like

        case $use_sdk in
          11.*);;
          12.*);;
          13.*);;
            *)
              AC_MSG_ERROR(error in configure of --with-sdk=$use_sdk)
            ;;
        esac

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

if [ ! test "x$use_cpu" = "xarm64" ]; then
platform_cflags+=" -mcpu=cortex-a8 -mfpu=neon"
platform_ldflags+=" -Wl,-segalign,4000"
fi

can be removed

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

There is a line in README.iOS.md under section 2. Prequisites

Device with iOS 9.0 or newer to install Kodi after build.

Will need updating.

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

End of section 4 in the same readme, theres a NOTE that references an ios 9 build.

NOTE: Advanced developers may want to specify an iOS SDK version (if multiple versions are installed) in the configure line(s) shown above. The example below would use the iOS SDK 9.0:

./configure --host=arm-apple-darwin --with-cpu=arm64 --with-sdk=9.0

Probably need to just alter that to 11

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

// since iOS 8
@"/var/mobile/Containers/Bundle/",

should be removable. The line below it should most likely stay as i believe its relevant for ios10/11/12

@fuzzard
Copy link
Contributor

fuzzard commented Feb 5, 2020

if [ "$DARWIN_ARM_CPU" == "arm64" ]
then
UPLOAD_FILENAME="kodi-$(getBuildRevDateStr)-ios64.deb"
else
UPLOAD_FILENAME="kodi-$(getBuildRevDateStr)-ios.deb"
fi

no need for the check now with a single ios platform

UPLOAD_FILENAME="kodi-$(getBuildRevDateStr)-ios64.deb"

@fuzzard
Copy link
Contributor

fuzzard commented Feb 6, 2020

# check if build is 64-bit
if [[ "$(lipo -info "$APP/@APP_NAME@" | awk '{print $NF}')" == "arm64" ]]; then
ARM64=true
fi

No need for the check, just set ARM64

@fuzzard
Copy link
Contributor

fuzzard commented Feb 6, 2020

Can probably remove the previous ARM64 check completely, and then

# package identifier for arm64
$ARM64 && ARCHIVE=${PACKAGE_ARM64}_${VERSION}-${REVISION}_@PLATFORM@-arm.deb

Remove the $ARM64 && as every package will be 64bit

@fuzzard
Copy link
Contributor

fuzzard commented Feb 6, 2020

if $ARM64; then
echo "Package: $PACKAGE_ARM64" > $DIRNAME/$PACKAGE/DEBIAN/control
echo "Name: @APP_NAME@-${PP_DEVICE} (64-bit)" >> $DIRNAME/$PACKAGE/DEBIAN/control
echo "Pre-Depends: cy+cpu.arm64" >> $DIRNAME/$PACKAGE/DEBIAN/control
echo "Conflicts: $PACKAGE" >> $DIRNAME/$PACKAGE/DEBIAN/control
echo "Replaces: $PACKAGE" >> $DIRNAME/$PACKAGE/DEBIAN/control
else
echo "Package: $PACKAGE" > $DIRNAME/$PACKAGE/DEBIAN/control
echo "Name: @APP_NAME@-${PP_DEVICE}" >> $DIRNAME/$PACKAGE/DEBIAN/control
fi

Change to just

echo "Package: $PACKAGE_ARM64"                  >  $DIRNAME/$PACKAGE/DEBIAN/control
echo "Name: @APP_NAME@-${PP_DEVICE} (64-bit)"   >> $DIRNAME/$PACKAGE/DEBIAN/control
echo "Pre-Depends: cy+cpu.arm64"                >> $DIRNAME/$PACKAGE/DEBIAN/control
echo "Conflicts: $PACKAGE"                      >> $DIRNAME/$PACKAGE/DEBIAN/control
echo "Replaces: $PACKAGE"                       >> $DIRNAME/$PACKAGE/DEBIAN/control

@fuzzard
Copy link
Contributor

fuzzard commented Feb 6, 2020

Remove

LDID32="$NATIVEPREFIX/bin/ldid32"

This can be reduced to just the LDID64 lines

if [ "${CURRENT_ARCH}" == "arm64" ] || [ "${CURRENT_ARCH}" == "aarch64" ]; then
LDID=${LDID64}
echo "using LDID64"
else
echo "using LDID32"
fi

@Rechi
Copy link
Member

Rechi commented Feb 6, 2020

First get this PR ready.
Once it is approved adjust the Jenkins configuration, run another PR build and if that reports success merge the PR.

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • ldid for 32 bit is still downloaded (extracted and installed)
  • deployment target isn't updated
  • There is still a dependency which explicitly disables a function, which is now available.

docs/README.iOS.md Outdated Show resolved Hide resolved
tools/buildsteps/defaultenv Outdated Show resolved Hide resolved
tools/depends/configure.ac Outdated Show resolved Hide resolved
docs/README.iOS.md Outdated Show resolved Hide resolved
tools/depends/target/ffmpeg/CMakeLists.txt Outdated Show resolved Hide resolved
tools/depends/target/ffmpeg/Makefile Outdated Show resolved Hide resolved
@phunkyfish phunkyfish force-pushed the min-ios-version branch 2 times, most recently from 39a4e98 to 8848354 Compare February 6, 2020 22:24
@phunkyfish
Copy link
Contributor Author

jenkins build this please

@phunkyfish phunkyfish removed the WIP PR that is still being worked on label Feb 15, 2020
@phunkyfish
Copy link
Contributor Author

Ok, ready @Rechi

@fuzzard fuzzard added this to the Matrix 19.0-alpha 1 milestone Feb 24, 2020
@phunkyfish
Copy link
Contributor Author

@Rechi is it ok to make the Jenkins changes now?

@Rechi
Copy link
Member

Rechi commented Feb 25, 2020

There is still a (new) hack because of the wrong host triplet.

@lrusak
Copy link
Contributor

lrusak commented Feb 26, 2020

There is still a (new) hack because of the wrong host triplet.

Could you be more specific? Do you mean about the mixed usage of arm64 and aarch64?

@phunkyfish
Copy link
Contributor Author

Maybe I’ve been looking at this too long but I’m not seeing it.

@Rechi
Copy link
Member

Rechi commented Feb 27, 2020

This branch is 3 commits ahead, 113 commits behind xbmc:master.

https://github.com/phunkyfish/xbmc/tree/min-ios-version

So there are changes from 113 commits which are currently not included in your branch.

@phunkyfish
Copy link
Contributor Author

Ah, that would make sense why I’m not seeing it so. At least I didn’t add the new hack!

@phunkyfish
Copy link
Contributor Author

Ok, I removed the todo's in the two toolchain files. Any other hacks you came across @Rechi ?

I didn't spot anything additional.

@phunkyfish
Copy link
Contributor Author

jenkins build this please

@phunkyfish
Copy link
Contributor Author

@Rechi is it ok to make the jenkins changes now?

@phunkyfish phunkyfish merged commit bbe9b0f into xbmc:master Mar 2, 2020
@phunkyfish phunkyfish deleted the min-ios-version branch March 2, 2020 22:29
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Mar 3, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Mar 4, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request May 5, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
[ios] set minimum ios version to 11.0, fix deprecations and cleanup
dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants