Add automation to strip Wormholy from non-Debug builds #15813
Conversation
|
|
mokagio
added
the
category: tooling
| remove_wormholy_dependency_from_package_swift | ||
|
|
There was a problem hiding this comment.
Do we want to remove it from Prototype Builds though?
My understanding is that Prototype Builds is exactly a case when Wormholy could be helpful for developers to debug network requests on a build without having to have to hook up Xcode when running the app to add breakpoints and intercept network responses, for example. So I'd assume that this is precisely one of the cases where they'd be interested in keeping it? (And only remove it for App Store submission)
Note: OTOH, I'm a bit wary of having a change (like the removal of Wormholy) being done only on the App Store builds if that means that it would never give us an opportunity to validate before the final release that this removal doesn't break the Release build… otherwise we'd risk not notice something breaking until the final release… or worse only after it has been submitted for review).
But I'm expecting that beta builds (i.e. TestFlight builds) would still allow us to catch those before the final release, even if Debug and Prototype Builds didn't have a change to validate earlier if a build that removed Wormholy would still compile.
There was a problem hiding this comment.
Do we want to remove it from Prototype Builds though?
I had this configured only to strip on Release build, but the import has always been #if DEBUG so it wouldn't run in Prototype Builds anyway
26688b4#diff-9d849b4aa93181781d29c444d1280080fc0a011e503496222848ce2f5fcd5977R22-R24
There was a problem hiding this comment.
Ah, I see, got it.
Could still be worth asking the dev team what they prefer though, as they're the one who will be using Wormholy for various debugging situations 🙃
There was a problem hiding this comment.
cc @woocommerce/ios-developers for the question of whether to have Wormholy in the prototype build.
My two cents: It seems like a useful tool to have in an internal build.
| # This relies on Package.swift being straightforward in how it defines dependencies. | ||
| # Given the code is only for usage in this repo at this time, it seems safe to do. | ||
| # | ||
| # This also assumes no other dependency, target, etc. includes "Wormholy" in its name. | ||
| # That also seem safe. | ||
| cleaned_content = content.lines.reject do |line| | ||
| line.include? 'Wormholy' | ||
| end.join |
There was a problem hiding this comment.
This might be ok for a first iteration of this, but still feels a bit fragile (especially in case one day we reformat the file to use multiline entries).
It's a shame that swift package command seems to have a dedicated add-dependency subcommand but no remove-dependency counterpart subcommand…
I also tried swift package dump-package to print the JSON representation and see if we could do something with it, but seems quite verbose and I'm not sure there's a way to regenerate a Package.swift from an altered JSON (i.e. after having removed Wormholy from the JSON)…
Maybe a safer alternative would be to encode the inclusion of the Wormholy package by some logic in the Swift code of the Package.swift file, like:
let includeWormholy = true
let package = Package(
…
dependencies: [
.package(url: …, from: …),
includeWormholy ? .package(url: "https://github.com/pmusolino/Wormholy", from: "2.0.0") : nil,
.package(url: …, from: …),
].compact
…
// and same for the `.product` in the `.xcodeTarget` entry later in the fileThen this helper method in the Fastfile would just have to content.gsub('let includeWormholy = true', 'let includeWormholy = false').
That way this would reduce the risk in case we later refactor the Package.swift to use multiline statements, or DRY some of the .package(…) references into variables so we can reference them in multiple places in the Package(…) definition… by having that Fastlane method only change a single, clearly defined line, instead of relying on more brittle .include? logic…
WDYT?
There was a problem hiding this comment.
The let to control the insertion is neat.
I considered reading CONFIGURATION from the env, but it would not have worked because the package resolution happens outside of the process that populates it from the build settings.
To be honest though, the more I think of the approach to remove Wormholy this way the less I like doing so.
I doubt a debugging library would take up a lot of space though, I downloaded the zip and the whole folder including the demo folder took up 673 KB (852 KB on disk) with the Sources folder being 133 KB (233 KB on disk). I'd say let's go for it, and if app size proves to be an issue with the library contributing to it by a significant amount, we can remove it.
Maybe we'd be better off putting this on the shelf for the sake of keeping the process straightforward.
There was a problem hiding this comment.
Yeah. Might be worth comparing the binary size with and without Wormholy (all other things the same) to have a definite answer of the size impact of the compiled lib in the final binary. But indeed the size difference might be insignificant enough to maybe prefer just keep it at all times after all, rather than trying to be clever and potentially introducing unforeseen side effect due to workaround…
There was a problem hiding this comment.
I tested building the Prototype Build with and without Wormholy while asking xcode to apply code thinning and generating IPA Size reports:
| For iPhone17,5, iOS 18 | Download Size | Install Size |
|---|---|---|
| With Wormholy | 97.22 MB | 191.50 MB |
| Without Wormholy | 97.09 MB | 191.20 MB |
cc @jaclync
How I got those numbers
1️⃣ Edit the Fastfile to ask for generating the app thinning reports when building Prototype Build:
--- a/fastlane/Fastfile
+++ b/fastlane/Fastfile
@@ -798,7 +798,8 @@ lane :build_for_prototype_build do |fetch_code_signing: true|
export_options: {
**COMMON_EXPORT_OPTIONS,
method: 'enterprise',
- iCloudContainerEnvironment: 'Production'
+ iCloudContainerEnvironment: 'Production',
+ thinning: '<thin-for-all-variants>'
}
)
end2️⃣ Run bundle exec fastlane build_for_prototype_build locally (with Wormholy still included in Modules/Package.swift), then copy the build/app-thinning.plist file to app-thinning-with-wormholy.plist
3️⃣ Comment the mentions of Wormholy in the Modules/Package.swift
--- a/Modules/Package.swift
+++ b/Modules/Package.swift
@@ -89,7 +89,7 @@ let package = Package(
.package(url: "https://github.com/markiv/SwiftUI-Shimmer", from: "1.0.0"),
.package(url: "https://github.com/nalexn/ViewInspector", from: "0.10.0"),
.package(url: "https://github.com/onevcat/Kingfisher", from: "7.6.2"),
- .package(url: "https://github.com/pmusolino/Wormholy", from: "2.0.0"),
+ // .package(url: "https://github.com/pmusolino/Wormholy", from: "2.0.0"),
.package(url: "https://github.com/pavolkmet/ScrollViewSectionKit", from: "1.2.0"),
.package(url: "https://github.com/Quick/Nimble.git", from: "13.0.0"),
.package(url: "https://github.com/simibac/ConfettiSwiftUI.git", from: "1.0.0"),
@@ -388,7 +388,7 @@ enum XcodeSupport {
.product(name: "Shimmer", package: "SwiftUI-Shimmer"),
.product(name: "StripeTerminal", package: "stripe-terminal-ios"),
.product(name: "WordPressEditor", package: "AztecEditor-iOS"),
- .product(name: "Wormholy", package: "Wormholy"),
+ // .product(name: "Wormholy", package: "Wormholy"),
.product(name: "ZendeskSupportSDK", package: "support_sdk_ios"),
]
),4️⃣ Run bundle exec fastlane build_for_prototype_build again, then copy the build/app-thinning.plist file to app-thinning-without-wormholy.plist
5️⃣ Compare the reported sizes. I chose to pick the entry for device: iPhone17,5, os-version: 18.0 as the baselime for that comparison, since that's the most recent iPhone model and version.
$ plutil -convert json -o - app-thinning-with-wormholy.plist | jq '.variants.[] | { downloadSize: .sizeCompressedApp, installSize: .sizeUncompressedApp, model: .variantDescriptors | select(.) | map(.device + " - " + .["os-version"] | select(. == "iPhone17,5 - 18.0")) | first } | select(.model)'
{
"downloadSize": 101946873,
"installSize": 200801487,
"model": "iPhone17,5 - 18.0"
}
$ plutil -convert json -o - app-thinning-without-wormholy.plist | jq '.variants.[] | { downloadSize: .sizeCompressedApp, installSize: .sizeUncompressedApp, model: .variantDescriptors | select(.) | map(.device + " - " + .["os-version"] | select(. == "iPhone17,5 - 18.0")) | first } | select(.model)'
{
"downloadSize": 101810448,
"installSize": 200491589,
"model": "iPhone17,5 - 18.0"
}
There was a problem hiding this comment.
For future reference, here's the complete App Thinning Reports (with and without Wormholy, plist and txt) I generated from my tests above:
There was a problem hiding this comment.
Thanks a lot for figuring out a way to test the app size before/after @AliSoftware! I have to say the 0.13-0.3 MB increase is higher than I thought, especially since we already got an App Store warning about the app size and we're making effort to minimize the size.
WDYT about the approach suggested by Alex in p91TBi-diV-p2#comment-14321 to have a separate debug target where the library is included in? Or a separate release target that excludes the library, whichever is easier.
There was a problem hiding this comment.
Also notice the discussion here: pmusolino/Wormholy#144
I tried this approach the other day and I could still see Wormholy in the build logs. But maybe I didn't configure the build settings properly.
Just leaving this here since I won't have the time to try it soon in case someone else wants to have a go.
|
Version |
|
Version |
|
Version |
Description
Supersedes #15804 implementing @AliSoftware's suggestion to script the Wormholy removal.
Steps to reproduce & Testing information
bundle exec fastlane build_for_app_storeand verify:Module/Package*files changed with Wormholy removedScreenshots
RELEASE-NOTES.txtif necessary. — N.A.