Add quick check for nested frameworks #4226

AliSoftware · 2021-05-19T13:02:45Z

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:

Checkout this branch, open the WooCommerce workspace in Xcode 12.4 and build the project
Check that you don't get any error during the build. In particular, you might want to go to the Build Log tab and expand the 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!
Select the Yosemite project in the workspace, go to the "General" tab, and in the "Frameworks and Libraries" section, change the line for Codegen from "Do Not Embed" to "Embed & Sign"
Build the project again, and check that you get a build error telling you all about the nested framework issue.

peril-woocommerce · 2021-05-19T13:29:10Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-woocommerce · 2021-05-19T13:29:10Z

You can trigger an installable build for these changes by visiting CircleCI here.

koke · 2021-05-19T13:56:27Z

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 TARGET_BUILD_DIR is the right one:

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

AliSoftware · 2021-05-19T14:05:09Z

Oh that's a good point and good idea 👍 Yeah will try as a build phase indeed!

AliSoftware · 2021-05-19T18:01:07Z

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:

▸ Running script 'Check for nested frameworks'

❌  error: Found nested framework in /Users/distiller/project/DerivedData/Build/Products/Debug-iphonesimulator/WooCommerce.app/Frameworks/Networking.framework/Frameworks/Codegen.framework -- Such a configura
tion is invalid and will be rejected by TestFlight. Please fix by choosing 'Do Not Embed' for the nested framework `Codegen.framework` within the `Networking` Xcode project containing it. You might need to use Xcode 12.5 to fix this, due to an Xcode 12.4 bug – see paNNhX-ee-p2

❌  error: Found nested framework in /Users/distiller/project/DerivedData/Build/Products/Debug-iphonesimulator/WooCommerce.app/Frameworks/Storage.framework/Frameworks/Codegen.framework -- Such a configuration is invalid and will be rejected by TestFlight. Please fix by choosing 'Do Not Embed' for the nested framework `Codegen.framework` within the `Storage` Xcode project containing it. You might need to use Xcode 12.5 to fix this, due to an Xcode 12.4 bug – see paNNhX-ee-p2

❌  error: Found nested framework in /Users/distiller/project/DerivedData/Build/Products/Debug-iphonesimulator/WooCommerce.app/Frameworks/Yosemite.framework/Frameworks/Codegen.framework -- Such a configuration is invalid and will be rejected by TestFlight. Please fix by choosing 'Do Not Embed' for the nested framework `Codegen.framework` within the `Yosemite` Xcode project containing it. You might need to use Xcode 12.5 to fix this, due to an Xcode 12.4 bug – see paNNhX-ee-p2

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.

AliSoftware · 2021-05-19T18:02:55Z

WooCommerce/WooCommerce.xcodeproj/project.pbxproj

+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = /bin/sh;
+			shellScript = "# Nested frameworks (i.e. having a Frameworks/ folder inside *.app/Frameworks/.framework) is invalid and will make the build be rejected during TestFlight validation. But this can happen especially due to an Xcode 12.4 UI bug when linking binary frameworks to the project which always embed the binary (bug that is fixed in Xcode 12.5)\n\nNESTED_FMKS=$(find \"${TARGET_BUILD_DIR}\"/WooCommerce.app/Frameworks/*.framework/Frameworks -name '*.framework') \nif [ -z \"$NESTED_FMKS\" ]; then\n    echo \"✅ No nested framework found, you're good to go!\"\nelse\n    for f in $NESTED_FMKS; do\n        nested=$(basename $f)\n        parent=$(basename $(dirname $(dirname $f)) .framework)\n        echo \"error: Found nested framework in $f -- Such a configuration is invalid and will be rejected by TestFlight. Please fix by choosing 'Do Not Embed' for the nested framework \\`$nested\\` within the \\`$parent\\` Xcode project containing it. You might need to use Xcode 12.5 to fix this, due to an Xcode 12.4 bug – see paNNhX-ee-p2\"\n    done\n    exit 1\nfi \n";


@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.

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: )

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 6f32c0c.

AliSoftware · 2021-05-19T18:06:41Z

And this is what it looks like after rebasing on top of develop after the commit that got the fix 🎉

AliSoftware · 2021-05-19T18:09:04Z

Retrospective P2 post here for reference: paNNhX-ee-p2

mokagio

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?

WooCommerce/WooCommerce.xcodeproj/project.pbxproj

mokagio · 2021-05-20T01:22:26Z

WooCommerce/WooCommerce.xcodeproj/project.pbxproj

+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = /bin/sh;
+			shellScript = "# Nested frameworks (i.e. having a Frameworks/ folder inside *.app/Frameworks/.framework) is invalid and will make the build be rejected during TestFlight validation. But this can happen especially due to an Xcode 12.4 UI bug when linking binary frameworks to the project which always embed the binary (bug that is fixed in Xcode 12.5)\n\nNESTED_FMKS=$(find \"${TARGET_BUILD_DIR}\"/WooCommerce.app/Frameworks/*.framework/Frameworks -name '*.framework') \nif [ -z \"$NESTED_FMKS\" ]; then\n    echo \"✅ No nested framework found, you're good to go!\"\nelse\n    for f in $NESTED_FMKS; do\n        nested=$(basename $f)\n        parent=$(basename $(dirname $(dirname $f)) .framework)\n        echo \"error: Found nested framework in $f -- Such a configuration is invalid and will be rejected by TestFlight. Please fix by choosing 'Do Not Embed' for the nested framework \\`$nested\\` within the \\`$parent\\` Xcode project containing it. You might need to use Xcode 12.5 to fix this, due to an Xcode 12.4 bug – see paNNhX-ee-p2\"\n    done\n    exit 1\nfi \n";


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: )

AliSoftware · 2021-05-20T05:50:35Z

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?

@mokagio I can't forward the output of find to /dev/null because that would defeat the purpose of having the list of nested frameworks as an output of this command when some are found. But I should be able to || true after the command to avoid this case though, will try that.

koke · 2021-05-20T06:31:13Z

I think the error on success is because the *.framework/Frameworks comes from shell expansion and not find, so when they don't exist, we are calling find with an invalid path.

I think this should work instead:

find "${TARGET_BUILD_DIR}"/WooCommerce.app/Frameworks -name Frameworks -depth 2

+ fix misleading message when no nested framework

AliSoftware · 2021-05-20T11:38:19Z

I think the error on success is because the *.framework/Frameworks comes from shell expansion and not find, so when they don't exist, we are calling find with an invalid path.

I think this should work instead:
find "${TARGET_BUILD_DIR}"/WooCommerce.app/Frameworks -name Frameworks -depth 2

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 find command in all situations… but this comes out pretty nice!

AliSoftware · 2021-05-20T11:44:16Z

Scripts/check-nested-frameworks.sh

+    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' ',')


-depth 1 here is so that if Networking.framework is embed in Yosemite but Codegen is also embed in Networking, we don't want to print about the 2nd level ie the WooCommerce.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 sed call is to get rid of the ./ at the beginning of each output line of find . -name …, and also to quote each line between a pair of backticks

The tr call 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

Default:

Hacked to emulate issue:

Scripts/check-nested-frameworks.sh

@mokagio

Thanks @mokagio for the suggestion.

AliSoftware self-assigned this May 19, 2021

AliSoftware added the category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. label May 19, 2021

AliSoftware force-pushed the check/nested-frameworks branch from ad7ce9b to 6f32c0c Compare May 19, 2021 13:29

AliSoftware commented May 19, 2021

View reviewed changes

AliSoftware added 3 commits May 19, 2021 20:03

Add quick check for nested frameworks

9387e4b

Revert "Add quick check for nested frameworks"

dc68f1d

This reverts commit 6f32c0c.

Add check as a build phase

581c114

AliSoftware force-pushed the check/nested-frameworks branch from 42f7214 to 581c114 Compare May 19, 2021 18:05

AliSoftware marked this pull request as ready for review May 19, 2021 18:06

AliSoftware requested review from koke, a team, oguzkocer, jkmassel and mokagio and removed request for a team May 19, 2021 18:06

AliSoftware added this to the 6.8 milestone May 19, 2021

mokagio approved these changes May 20, 2021

View reviewed changes

AliSoftware requested a review from mokagio May 20, 2021 11:02

Extract script in dedicated shell file

e72ad46

+ fix misleading message when no nested framework

AliSoftware force-pushed the check/nested-frameworks branch from d6e8860 to e72ad46 Compare May 20, 2021 11:37

AliSoftware commented May 20, 2021

View reviewed changes

mokagio approved these changes May 21, 2021

View reviewed changes

Scripts/check-nested-frameworks.sh Show resolved Hide resolved

Add comments to explain the find/sed/tr commands

eff300e

Thanks @mokagio for the suggestion.

AliSoftware force-pushed the check/nested-frameworks branch from 93cac05 to eff300e Compare May 25, 2021 13:40

AliSoftware enabled auto-merge May 25, 2021 13:46

AliSoftware merged commit 3115f4c into develop May 25, 2021

AliSoftware deleted the check/nested-frameworks branch May 25, 2021 13:59

AliSoftware mentioned this pull request Sep 5, 2022

Merge beta 10.2.0.1 into trunk #7648

Merged

AliSoftware mentioned this pull request Jul 11, 2023

[Gutenberg] Tweak Pod dependencies based on React Native version used by Gutenberg wordpress-mobile/WordPress-iOS#21021

Closed

13 tasks

mokagio mentioned this pull request Jul 18, 2023

Add build phase to detect nested frameworks wordpress-mobile/WordPress-iOS#21130

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quick check for nested frameworks #4226

Add quick check for nested frameworks #4226

AliSoftware commented May 19, 2021 •

edited

Loading

peril-woocommerce bot commented May 19, 2021 •

edited

Loading

peril-woocommerce bot commented May 19, 2021 •

edited

Loading

koke commented May 19, 2021

AliSoftware commented May 19, 2021

AliSoftware commented May 19, 2021

AliSoftware May 19, 2021

mokagio May 20, 2021

AliSoftware May 20, 2021

AliSoftware commented May 19, 2021

AliSoftware commented May 19, 2021

mokagio left a comment

mokagio May 20, 2021

AliSoftware commented May 20, 2021 •

edited

Loading

koke commented May 20, 2021

AliSoftware commented May 20, 2021

AliSoftware May 20, 2021

mokagio left a comment

Add quick check for nested frameworks #4226

Add quick check for nested frameworks #4226

Conversation

AliSoftware commented May 19, 2021 • edited Loading

To test

peril-woocommerce bot commented May 19, 2021 • edited Loading

peril-woocommerce bot commented May 19, 2021 • edited Loading

koke commented May 19, 2021

AliSoftware commented May 19, 2021

AliSoftware commented May 19, 2021

AliSoftware May 19, 2021

Choose a reason for hiding this comment

mokagio May 20, 2021

Choose a reason for hiding this comment

AliSoftware May 20, 2021

Choose a reason for hiding this comment

AliSoftware commented May 19, 2021

AliSoftware commented May 19, 2021

mokagio left a comment

Choose a reason for hiding this comment

mokagio May 20, 2021

Choose a reason for hiding this comment

AliSoftware commented May 20, 2021 • edited Loading

koke commented May 20, 2021

AliSoftware commented May 20, 2021

AliSoftware May 20, 2021

Choose a reason for hiding this comment

mokagio left a comment

Choose a reason for hiding this comment

AliSoftware commented May 19, 2021 •

edited

Loading

peril-woocommerce bot commented May 19, 2021 •

edited

Loading

peril-woocommerce bot commented May 19, 2021 •

edited

Loading

AliSoftware commented May 20, 2021 •

edited

Loading