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

[NTV-606] KingFisher Carthage -> SPM #1722

Merged
merged 2 commits into from Sep 7, 2022
Merged

[NTV-606] KingFisher Carthage -> SPM #1722

merged 2 commits into from Sep 7, 2022

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Sep 6, 2022

📲 What

As we migrate over to M1 machines for local development, we'll want to move over our Carthage dependencies into Xcode dependency manager where we can.

🤔 Why

There are many benefits of using SPM over Carthage, but the main reason is to avoid handling manual compilation scripts to support multiple architecture types like this one found inside carthage.sh

# For Xcode 13 make sure EXCLUDED_ARCHS is set to arm architectures otherwise
# the build will fail on lipo due to duplicate architectures.

CURRENT_XCODE_VERSION=$(xcodebuild -version | grep "Build version" | cut -d' ' -f3)
echo "EXCLUDED_ARCHS__EFFECTIVE_PLATFORM_SUFFIX_simulator__NATIVE_ARCH_64_BIT_x86_64__XCODE_1300__BUILD_$CURRENT_XCODE_VERSION = arm64 arm64e armv7 armv7s armv6 armv8" >> $xcconfig

echo 'EXCLUDED_ARCHS__EFFECTIVE_PLATFORM_SUFFIX_simulator__NATIVE_ARCH_64_BIT_x86_64__XCODE_1300 = $(EXCLUDED_ARCHS__EFFECTIVE_PLATFORM_SUFFIX_simulator__NATIVE_ARCH_64_BIT_x86_64__XCODE_1300__BUILD_$(XCODE_PRODUCT_BUILD_VERSION))' >> $xcconfig
echo '

As you can see it's verbose, very cryptic, and we're not entirely sure if we can add M1 support to strings like EXCLUDED_ARCHS

At least for now, we're going to move all dependencies that we can over to SPM that are listed inside the Cartfile.
For now we'll do the 3rd party dependencies. There's also binaries which aren't built as schemas (the step failing on make bootstrap with M1 because of the code snippet above). Finally there's internal libraries that do get converted to schemas, which we might need to create custom SPM packages out of as we did with Kickstarter-Prelude.

🛠 How

  • Removed KingFisher from Cartfile.
  • Removed KingFisher from Cartfile.resolved
  • Remove references inside Kickstarter/Frameworks folder and within each target.
  • Remove references inside Carthage-xcfilelist
  • Add https://github.com/onevcat/Kingfisher to Package Dependencies version 7.3.2
  • Add package once downloaded to Library target
    • SPM downloads and caches the library, but it's still needed wherever KingFisher was included before, in this case the only place was "Library" target.
  • Ensure app builds, tests pass.
  • Set breakpoints for where import statements are import KingFisher, set breakpoints for where framework references are, run app and hit breakpoints. Continue running app and smoke tests where framework call sites occur.

✅ Acceptance criteria

  • Breakpoint most instances of code used in files that import KingFisher and run the app on device to check the breakpoints are hit and the app continues to run as expected.
  • Run app and smoke test where framework is supposed to be used.

@scottkicks scottkicks self-assigned this Sep 6, 2022
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1722 (a200cbb) into main (7c9bd4c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1722   +/-   ##
=======================================
  Coverage   85.33%   85.33%           
=======================================
  Files        1275     1275           
  Lines      114806   114806           
  Branches    30343    30343           
=======================================
  Hits        97970    97970           
  Misses      15784    15784           
  Partials     1052     1052           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Most changes look good, one thing I noticed, (on the AlamofireImage PR too), is to not include the framework in the main Kickstarter-iOS target if it can be avoided.

In this case the previous Carthage version of Kingfisher was included only in Library-iOS, and because Library-iOS is imported in Kickstarter-iOS, it should be enough to just include Kingfisher in only Library-iOS.

A good way to check is to look at exactly which targets the library is included in before removing from Carthage and add it back in those same targets (always under Build Phases Link Binary with Libraries) after SPM caches it.

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

good to go!

@scottkicks scottkicks merged commit 38e286c into main Sep 7, 2022
@scottkicks scottkicks deleted the NTV-606 branch September 7, 2022 19:24
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

2 participants