-
Notifications
You must be signed in to change notification settings - Fork 118
Add build phase script to remove Wormholy files #15804
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
Add build phase script to remove Wormholy files #15804
Conversation
Generated by 🚫 Danger |
|
# exit 0 | ||
# fi | ||
|
||
# FIXME: Delete from all builds to test the implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously we should remove the comments before merging.
@jaclync this works for me when running on device, with the commit that strips the files for every build configuration. What do you think? @AliSoftware @iangmaia @twstokes what's your opinion on the implementation? I feel iffy about brute-force deleting all the Wormholy files, which by the way are:
But given the single import for Wormholy is fenced in a Update Mmm... there's something not working, because even if the It's possible I'm looking at the wrong folder where to act... I'll need to look more into it. |
#if DEBUG | ||
import WormholySwift | ||
#endif | ||
//#if DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was gonna say something about trying condition: .when(configuration: .debug)
but just seen this discussion 😞
SwiftPM allows conditional platform dependencies, but not conditional configurations.
BUILT_PRODUCTS_DIR=${BUILT_PRODUCTS_DIR:-"$TARGET_BUILD_DIR"} | ||
|
||
# Crude way to remove all the Wormholy files from the build. | ||
rm -rf "$BUILT_PRODUCTS_DIR/Wormholy*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using find
instead (or find
+ rm
), so we can have more control of what we're searching, can add error handling, etc?
if find "$BUILT_PRODUCTS_DIR" -name "*Wormholy*" -print -delete | grep -q .; then
echo "info: Successfully removed Wormholy files"
else
echo "warning: No Wormholy files found to remove"
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that find … -delete
deletes folders (as opposed to only files)? Or if it does, that it is able to delete the folder recursively, or only in the right order only after *Wormholy*
files it contains have been deleted and it happens that there would be no remaining files. We might need a -prune
to avoid find
to try to scan directories after they've been deleted as well…
Besides, using find
like this means it might delete *Wormholy*
files other than just the ones at the root of the $BUILT_PRODUCTS_DIR
and potentally files deeper in this folder structure. Maybe that's actually why you suggested this @iangmaia … but I'm also not sure this would be the right idea as this feels a tad more "dangerous" than only removing $BUILT_PRODUCTS_DIR/Wormholy*
(what if frameworks and targets unrelated with Wormholy itself would include the name "Wormholy" in their file name deep inside a folder that is not $BUILT_PRODUCTS_DIR/Wormholy*/
but $BUILT_PRODUCTS_DIR/Somethingelse/…/BlahWormholyBah
…)
Overall I'm not sure there's any perfect solution, unless we find a way to determine the actual list of files generated by the Wormholy package in $BUILT_PRODUCTS_DIR/
by analyzing its Package or SwiftPM target in some clever way (checking the compilation database or whatever?)… which I'm not sure is worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, are we sure that removing the Wormholy*
files from BUILT_PRODUCTS_DIR
is enough to remove Wormholy from the final binary? I'm sure it would help reducing the binary file size if that works by ensuring the .o
is not linked to the final binary if that .o
doesn't exist anymore… but I'm worried about other side effects to the compilation process of deleting files from $BUILT_PRODUCTS_DIR/
in the middle of a build there. Like wouldn't the linker risk failing while it'd try to link to the Wormholy.o
file and not find it? Or even if it doesn't, maybe there's a risk of crash at runtime on the Release build in case Wormholy is integrated as a dynamic framework (as opposed to statically linked) and dyld
would still find instructions to load that Wormholy framework (as per otool -L
) in its loading instructions… fail to find the framework in its @rpath
…
Which would actually be the worst case scenario, as that would make Debug builds compile and work, and Release builds compile too (making us think they also work) but failed to launch… and it'd be easy to thus submit a Release build to App Store that would in practice fail to launch and not realize it until it's too late…
I don't have any magical solution for that though tbh. The only option I could think of that might feel safe enough to me would be remove Wormholy from the project before building a Release Build. That is, to write a script that would remove Wormholy
as a Package Dependency on the project, and run that script on CI before we resolve package dependencies and compile the Release app for submission to TestFlight and AppStore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, using the command as-is would delete only files (and empty directories), but indeed also files deeper in the directory structure. My main idea was to go deeper in the structure and have more control over the search (we could do alternatively something like find "$BUILT_PRODUCTS_DIR" -name "*Wormholy*" -exec rm -rf {}
)... but 💯 agree it can be dangerous and might not be enough to remove it from the final binary.
remove Wormholy from the project before building a Release Build
This actually doesn't sound too bad given the current constraints 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, to write a script that would remove
Wormholy
as a Package Dependency on the project, and run that script on CI before we resolve package dependencies and compile the Release app for submission to TestFlight and AppStore.
🤔 🤔 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in #15813.
Closing in favor of #15813 |
Builds on top of #15771 to try and remove Wormholy from the Release build.
Note
Currently strips from all builds, so we can test.
Run on device from Xcode or via Prototype Build and verify it... does not crash?