[Card Payments] Add additional configuration keys to Info.plist for Debug builds

ctarda commented 3 years ago

Closes #3593

Please see this comment for context.

We need to provide several keys in the Info.plist file in order for the Stripe Terminal SDK to work. For more info, please see: https://stripe.com/docs/terminal/sdk/ios#configure

This PR adds a new build phase that would merge the contents of an Info.plist file with entries specific for a given build configuration into the final Info.plist

Changes

Add AdditionalKeys-Debug-Info.plist with keys specific for the integration with the Stripe Terminal SDK
Add a Run Script Build Phase that will merge these additional keys with the content of Info.plist into a new file (that is added to .gitignore)
Change the base build configuration to point to the Info.plist file that contains the merged data

How to test

Checkout the branch, build and run in debug mode. Check the there is no local diff
See this comment for detailed testing instructions: https://github.com/woocommerce/woocommerce-ios/pull/3656#issuecomment-778485222

Update release notes:

[x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-woocommerce[bot] commented 3 years ago

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

peril-woocommerce[bot] commented 3 years ago

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

AliSoftware commented 3 years ago

@ctarda Pushed a commit with some changes.

Ended up making the script even more flexible, allowing additional keys both for Debug and Release, even if for now we only have ones for Debug.

The script now:

Copies the Info.plist file (the one in which we keep the keys that are common for all configs, our base) to Consolidated-$CONFIGURATION-Info.plist – aka the new value for the INFOPLIST_PATH build setting.
Then instead of testing if we are in Debug or not and do it, since I liked your idea of testing if the file existed if [ -f $FILE ]) in your script, I decided to directly only rely on that file's existence, and deriving that file's name from the current configuration.
This means that if a file named AdditionalKeys-${CONFIGURATION}-Info.plist exists, then the script will merge it with the Consolidated-${CONFIGURATION}-Info.plist file. (And if no such file exists, it won't do the merge and keep the consolidated file unchanged.)
Currently we only have a AdditionalKeys-Debug-Info.plist file. But if one day we need additional keys only for Release, we would just need to add a AdditionalKeys-Release-Info.plist file and that would work automatically too!

So TL;DR it's not based on if the $CONFIGURATION is "Debug" anymore but instead on if a AdditionalKeys-${CONFIGURATION}-Info.plist file exists which makes it more flexible.

Note that to make it work with the New Build System, which checks that all input files will exist before starting the build, I had to define the Input and Output files of the Script Build Phase, to tell Xcode which input files this script relied on (Info.plist + AdditionalKeys-${CONFIGURATION}-Info.plist), and which output files this script will produce or modify ($INFOPLIST_PATH in our case).

This has 2 benefits:

Xcode will see if the input files have not been modified, and if not, will skip the generation of the output file, as it should not be needed if the inputs are unchanged. This will avoid losing time during the build.
It allows Xcode to know that the Info.plist file the project is configured to use, $INFOPLIST_PATH, will be generated by the script. So even if the file does not yet exist on your disk yet (because you never did a build with this configuration, or you git clean-ed your working copy of those files for example), Xcode now knows that the script will generate this file by the time it's needed so it's all ok. While if we didn't define that as an "output file" on the Script Build Phase, Xcode would have fail to even start compiling your target, and bail with "Input file XXX doesn't exist").

ctarda commented 3 years ago

Thats fantastic @AliSoftware thanks! I'll submit it for formal review

ctarda commented 3 years ago

@AliSoftware I was thinking... you are the one that really knows this area. I took a look at your last two commits and it's a :shipit: from me. Do you want to merge this?

AliSoftware commented 3 years ago

Sure but I did only very quick checks with those changes, mainly playing with the script at the end of my day… without really doing in-depth stress tests in the code.

So I think it could be valuable to have someone do some monkey testing before merging just to be sure.

How to test (example)

Do a clean build in Debug
Check, using plutil -p <PathToAppInBuildProductsInDerivedData/Info.plist>, that the final Info.plist that is in the final app has the expected keys
Do a clean build in Release
Likewise, check the keys are not in the Info.plist inside the final generated .app
Do an incremental build in Debug
Check result again
Do another incremental Debug build, check Xcode skips the build phase (because no changes)
Check the result
Do an incremental Release build
Check result
Another incremental Release build, check phase is skipped
Check result in plist from within generated .app

Or some variation of those various "build and check and build again" cycles with randomly changing the configuration in-between cycles and randomly sometimes cleaning DerivedData (clean build) sometimes not (incremental build) between tries, and check that it won't break or include the wrong keys or fail to compile.

AliSoftware commented 3 years ago

Also since I am the one who wrote it I don't think I should be the one to review it: better to use a different pair of eyes on reviews than trust myself, especially on code I wrote late and past then end of a long Friday 😄

(Maybe another MIE can give it a quick look if you want)

allendav commented 3 years ago

Sorry if I am off-base, but would the simpler approach mentioned here suffice and avoid the scripting? https://stackoverflow.com/questions/8971488/configuration-dependent-value-in-info-plist-file

AliSoftware commented 3 years ago

Sorry if I am off-base, but would the simpler approach mentioned here suffice and avoid the scripting? https://stackoverflow.com/questions/8971488/configuration-dependent-value-in-info-plist-file

This approach only works for providing different values to a key that would always be present in the Info.plist file. But here we want to add new keys in Debug, keys that should not be present at all in the Info.plist when building for Release – as opposed to being present but with a blank value).

PS: FYI there is yet another other alternative method I know about to achieve this… but I really don't recommend it though (see spoiler for details).

There's also another approach that was possible, which is to use `#if DEBUG` in the `Info.plist` file (yes, when you edit the `plist` as source and see its XML source, you can add preprocessor conditions like here, and if you enable the appropriate Build Settings you can ask Xcode to preprocess the Info.plist via the C Preprocessor, interpreting those). But I highly discourage this alternative because it's completely hidden when you then look at the `Info.plist` in Xcode in the Plist (not source) view, and if you edit your `Info.plist` in Xcode using the Plist graphical view you will discard those hidden `#if` anyway, so TL;DR using this techniques not only makes it quite hidden but also makes it quite easily to accidentally override those hacks with a quick edit by someone on something unrelated in the Plist)

allendav commented 3 years ago

Thanks @AliSoftware ! I hadn't considered that we couldn't use that approach to have a key only present in one of the plists.

And now, attempting to summarize our discussion about this in slack... the Info.plist file in question only changes a few times a year. In light of this, and since this approach is only needed presently for less than a year while we build out the card reader integration, I proposed we consider a simpler approach for starters - forking the plist file into a Release-Info.plist used for release builds and a Debug-Info.plist used for development. The Debug-Info.plist would include these keys that we are not yet ready to have in Release:

NSBluetoothAlwaysUsageDescription NSBluetoothPeripheralUsageDescription NSLocationWhenInUseUsageDescription UIBackgroundModes

Edit: From https://stripe.com/docs/terminal/sdk/ios#configure

To deter developers from making changes to Debug-Info.plist without considering whether those changes should also be included in Release-Info.plist (and vice versa) a Peril rule could monitor pull requests. Again, this is a temporary simple "hack" and we forsee no other deltas other than those four above - with the goal of a single Info.plist before year's end.

@woocommerce/platform-9 - @jkmassel - thoughts?

ctarda commented 3 years ago

I'm way beyond my work hours for today so just in case someone else picks this up, I pushed a draft PR that duplicates the info.plist file. I don't know how to add a rule to Peril, so I didn't do that: #3676

ctarda commented 3 years ago

Putting this on hold for now

woocommerce / woocommerce-ios