Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Conversation

@ealeksandrov
Copy link
Contributor

Description

With #610 (GoogleSignIn update) merged we don't have any stale dependencies holding us back from arm64 simulator compatibility.

This PR:

  • removes settings to exclude arm64 in project and Podfile
  • removes static framework setting from Podspec
  • converts code to Swift 5
  • update minimum requirement to iOS 13
  • removes unused OHHTTPStubs dev dependency
  • updates OCMock dev dependency
  • fixes all new Xcode warnings

Tested on Intel, M1 in Rosetta mode, M1 native.

Test

  1. Clone this repo, install gems and pods (instructions in README), open project, build and run unit tests.
  2. Try updated dependency with WC-iOS or WP-iOS app. (Switch to test version by using try/apple-silicon-updated branch for WordPressAuthenticator-iOS in Podfile.)

Additional note for WC-iOS + M1 users: if you want to test on arm64 simulator - hack StripeTerminal (like described in p91TBi-5tl-p2). It's not needed for device build or Rosetta mode.

@ealeksandrov ealeksandrov added the enhancement New feature or request label Aug 5, 2021
@ealeksandrov ealeksandrov requested review from a team and dvdchr August 5, 2021 18:41
@ealeksandrov
Copy link
Contributor Author

There is an issue on pod lib lint check in CI (reproduced locally) that I don't know how to fix yet:

> WordPressKit/AccountSettingsRemote.swift:
> import WordPressShared
error: compiling for iOS 11.0, but module 'WordPressShared' has a minimum deployment target of iOS 13.0:

WPAuth has iOS 13 target in this branch. One of dependencies is WordPressShared 1.16.1 that targets iOS 13. But WordPressKit 4.39.0 targets iOS 11 and CocoaPods builds it without bumping to iOS 13.

Easiest way to reproduce:

  1. Pull the branch
  2. Remove post_install script in Podfile:
    post_install do |installer|
    # Let Pods targets inherit deployment target from the app
    # This solution is suggested here: https://github.com/CocoaPods/CocoaPods/issues/4859
    # =====================================
    #
    installer.pods_project.targets.each do |target|
    target.build_configurations.each do |configuration|
    pod_ios_deployment_target = Gem::Version.new(configuration.build_settings['IPHONEOS_DEPLOYMENT_TARGET'])
    if pod_ios_deployment_target <= ios_deployment_target
    configuration.build_settings.delete 'IPHONEOS_DEPLOYMENT_TARGET'
    end
    end
    end
    end
  3. bundle exec pod update
  4. Try building the workspace

While we can hack deployment target with post_install on local setup - it doesn't apply to pod lib lint.

@mokagio
Copy link
Contributor

mokagio commented Aug 6, 2021

@ealeksandrov thank you for taking this on board 🙌

WordPress iOS targets iOS 13. Maybe we could address this by bumping the deployment target for WordPressKit to 13.0, too?

Copy link
Contributor

@dvdchr dvdchr left a comment

Choose a reason for hiding this comment

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

Done with my first pass of review.

  • Code changes look good to me.
  • Build & test passes on my Intel and M1 machines. 👍

Next, I'll take a look at integrating this to WP-iOS and running it in both my Intel & M1 machines.

Maybe we could address this by bumping the deployment target for WordPressKit to 13.0, too?

+1 on this. 🙂

- OHHTTPStubs/OHPathHelpers (8.0.0)
- OHHTTPStubs/Swift (8.0.0):
- OHHTTPStubs/Default
- OCMock (3.8.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 !! Adding context to this, support for ARM-based Macs is introduced in OCMock 3.8.

@ealeksandrov
Copy link
Contributor Author

@mokagio @dvdchr thank you for the suggestion! I agree with alignment of deployment target everywhere.
The PR for WordPressKit is ready: wordpress-mobile/WordPressKit-iOS#430.

@ealeksandrov
Copy link
Contributor Author

CI is green, lint passed 🎉

Comment on lines -46 to -47
pod 'OHHTTPStubs', '8.0.0'
pod 'OHHTTPStubs/Swift', '8.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

😢 goodbye my baby 👋 😄

@AliSoftware AliSoftware requested a review from a team August 10, 2021 11:29
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Hey @ealeksandrov! Sorry for not following up on your progress on this yesterday after you merged the WordPress Kit iOS 13 bump.

I wanted to checkout the Pod on my M1 Mac Mini and see if it all built fine, but I run out of time, both yesterday and today.

Other folks have approved this already, and I agree. We'll deal with M1 issues if they arise 👍

@ealeksandrov
Copy link
Contributor Author

@mokagio no worries! + @jkmassel @dvdchr thanks for everyones reviews and suggestions! 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants