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-609] Migrates SwiftSoup to SPM #1723

Merged
merged 1 commit into from Sep 8, 2022
Merged

[NTV-609] Migrates SwiftSoup to SPM #1723

merged 1 commit into from Sep 8, 2022

Conversation

scottkicks
Copy link
Contributor

πŸ“² 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 SwiftSoup from Cartfile.
  • Removed SwiftSoup from Cartfile.resolved
  • Remove references inside Kickstarter/Frameworks folder and within each target.
  • Remove references inside Carthage-xcfilelist
  • Add https://github.com/scinfu/SwiftSoup to Package Dependencies version 2.0.0
  • Add package once downloaded to KsApi target
  • Ensure app builds, tests pass.
  • Set breakpoints for where import statements are import SwiftSoup, 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 8, 2022
@scottkicks scottkicks added this to the release-5.5.0 milestone Sep 8, 2022
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #1723 (f92422e) into main (38e286c) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1723   +/-   ##
=======================================
  Coverage   85.33%   85.33%           
=======================================
  Files        1275     1275           
  Lines      114806   114806           
  Branches    30343    30343           
=======================================
+ Hits        97968    97970    +2     
+ Misses      15786    15784    -2     
  Partials     1052     1052           
Impacted Files Coverage Ξ”
Library/Navigation.swift 85.85% <0.00%> (+0.64%) ⬆️

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@scottkicks scottkicks marked this pull request as ready for review September 8, 2022 13:18
@msadoon msadoon added needs review and removed draft labels Sep 8, 2022
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!
One minor thing, I missed this on the Kingfisher pr, but can you set the minimum version in package dependencies to the minimum version we have in the Cartfile and Cartfile.resolved? Ie. for Kingfisher make it 7.3.2 and for SwiftSoup 2.4.3

Just so going forward we maintain that Carthage minimum version is the same as in SPM.

Can do that in next pr.

@scottkicks scottkicks merged commit edd69a7 into main Sep 8, 2022
@scottkicks scottkicks deleted the ntv-609 branch September 8, 2022 16:40
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