Add quick check for nested frameworks#4226
Conversation
ad7ce9b to
6f32c0c
Compare
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
I think we can add this as a Run Script phase, so we catch the error locally and not just on CI. This seems to work, although I'm not too familiar with build environment vars to know if NESTED_FMKS=$(find "${TARGET_BUILD_DIR}"/WooCommerce.app/Frameworks/*.framework -name Frameworks)
if [ -n "$NESTED_FMKS" ]; then
echo "error: Found some nested frameworks inside Frameworks/*/Frameworks subdirectories. Such a configuration is invalid and will be rejected by TestFlight. Please fix by choosing 'Do Not Embed' for the nested framework in the Xcode project of the parent framework containing it"
echo $NESTED_FMKS
exit 1
fi |
|
Oh that's a good point and good idea 👍 Yeah will try as a build phase indeed! |
|
Updated the PR to use a Script Build Phase, and this is what it looks like in Xcode when it finds nested frameworks: And as a bonus, here's the expected build failure on CI too when the fix hasn't been applied yet: I'm gonna rebase that branch on top of the fix now to double-check that the script goes green and doesn't error anymore once fixed. |
There was a problem hiding this comment.
@koke Seems like using $TARGET_BUILD_DIR was the right decision, according to my favorite Build Settings reference 👍 :
Run Script build phases that operate on product files of the target that defines them should use the value of this build setting.
There was a problem hiding this comment.
Following up on the comment above, having the run script in an actual script also makes it simpler to read and edit it, as well as having a sensible diff to look at as opposed to a single very long line (:xcode: 
)
There was a problem hiding this comment.
Yeah I agree that I could have extracted that script into a file in Scripts/….sh, to make it more readable and diff-able 👍
That's actually something I usually do so not sure why I didn't that time around 😺
This reverts commit 6f32c0c91425f4014073493b6c73a198ad3ca93d.
42f7214 to
581c114
Compare
|
And this is what it looks like after rebasing on top of |
|
Retrospective P2 post here for reference: paNNhX-ee-p2 |
mokagio
left a comment
There was a problem hiding this comment.
Default behavior:
Hacked behavior to force failure:
👍 
One nitpick
I found the error output from find before the successful setup message confusing. "Why am I seeing something that looks like an error when the log tells me everything is fine? 🤔"
Have you considered forwarding find's output to /dev/null?
There was a problem hiding this comment.
Following up on the comment above, having the run script in an actual script also makes it simpler to read and edit it, as well as having a sensible diff to look at as opposed to a single very long line (:xcode: )
@mokagio I can't forward the output of find to |
|
I think the error on success is because the I think this should work instead: find "${TARGET_BUILD_DIR}"/WooCommerce.app/Frameworks -name Frameworks -depth 2 |
+ fix misleading message when no nested framework
d6e8860 to
e72ad46
Compare
This is now fixed – and also extracted in a dedicated script file as @mokagio suggested 🎉 I had to tweak the script a bit more so that the error messages were still nice with this new result format of the new
|
| echo "❌ Found nested \`Frameworks\` folder inside frameworks of final bundle." | ||
| for fmk_dir in $NESTED_FMKS_DIRS; do | ||
| parent_fmk=$(basename $(dirname $fmk_dir) .framework) | ||
| nested_fmks=$(cd "${fmk_dir}" && find . -name '*.framework' -depth 1 | sed "s:^./\(.*\)$:\`\1\`:" | tr '\n' ',') |
There was a problem hiding this comment.
-depth 1here is so that ifNetworking.frameworkis embed inYosemitebutCodegenis also embed inNetworking, we don't want to print about the 2nd level ie theWooCommerce.app/Frameworks/Yosemite.framework/Frameworks/Networking.framework/Frameworks/Codegen.framework– since the nesting of Codegen within Networking will already be printed as a separate error- The
sedcall is to get rid of the./at the beginning of each output line offind . -name …, and also to quote each line between a pair of backticks - The
trcall is to join all the lines with a,. This will add an extra,at the end of the list, but we'll get rid of this by using bash substitution${nested_fmks%,}when printing the list line 17, to get rid of the suffixed comma.
mokagio
left a comment
There was a problem hiding this comment.
Default:
Hacked to emulate issue:
Thanks @mokagio for the suggestion.
93cac05 to
eff300e
Compare
Following the issue we had with TestFlight validation and nested frameworks introduced by the Xcode 12.4 bug (see paNNhX-ee-p2 for details), this check will hopefully catch if we encounter such a configuration issue again later.
To test
I already tested this myself (see comments below) but it could be nice to validate the check works on other's machines too.
To test this:
Run custom shell script 'Check for nested frameworks'step at the end and check that it says✅ No nested framework found, you're good to go!Yosemiteproject in the workspace, go to the "General" tab, and in the "Frameworks and Libraries" section, change the line forCodegenfrom "Do Not Embed" to "Embed & Sign"