Fix VPN uninstall not working for App Store users. #2951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
bot: not in app board
diegoreymendez
removed
the
bot: not in app board
diegoreymendez
commented
Jul 8, 2024
| @@ -9531,7 +9531,7 @@ | |||
| ); | |||
| runOnlyForDeploymentPostprocessing = 0; | |||
| shellPath = /bin/sh; | |||
| shellScript = "# Embeds login items for the App Store build.\n\n# Skip login item embedding for release builds until they're ready to go live.\nif [ \"${CONFIGURATION}\" = \"Release\" ]; then\n VPN_AGENT_NAME=\"${AGENT_RELEASE_PRODUCT_NAME}\"\n PIR_AGENT_NAME=\"${DBP_BACKGROUND_AGENT_RELEASE_PRODUCT_NAME}\"\nelse\n VPN_AGENT_NAME=\"${AGENT_PRODUCT_NAME}\"\n PIR_AGENT_NAME=\"${DBP_BACKGROUND_AGENT_PRODUCT_NAME}\"\nfi\n\nVPN_AGENT_ORIGIN=$(readlink -f \"${CONFIGURATION_BUILD_DIR}/${VPN_AGENT_NAME}.app\")\nPIR_AGENT_ORIGIN=$(readlink -f \"${CONFIGURATION_BUILD_DIR}/${PIR_AGENT_NAME}.app\")\nAGENT_DESTINATION=\"${CONFIGURATION_BUILD_DIR}/${CONTENTS_FOLDER_PATH}/Library/LoginItems\"\n \n# Make sure that Library/LoginItems exists before copying\nmkdir -p \"$AGENT_DESTINATION\"\n \necho \"Copying VPN agent from $VPN_AGENT_ORIGIN to $AGENT_DESTINATION\"\nrsync -r --links \"$VPN_AGENT_ORIGIN\" \"$AGENT_DESTINATION\"\n \necho \"Copying Personal Information Removal agent from $PIR_AGENT_ORIGIN to $AGENT_DESTINATION\"\nrsync -r --links \"$PIR_AGENT_ORIGIN\" \"$AGENT_DESTINATION\"\n"; | |||
| shellScript = "# Embeds login items for the App Store build.\n\n# Skip login item embedding for release builds until they're ready to go live.\nif [ \"${CONFIGURATION}\" = \"Release\" ]; then\n PIR_AGENT_NAME=\"${DBP_BACKGROUND_AGENT_RELEASE_PRODUCT_NAME}\"\nelse\n PIR_AGENT_NAME=\"${DBP_BACKGROUND_AGENT_PRODUCT_NAME}\"\nfi\n\nVPN_AGENT_NAME=\"${AGENT_PRODUCT_NAME}\"\n\nif [ -z \"$VPN_AGENT_NAME\" ]; then\n echo \"VPN_AGENT_NAME is empty.\"\n exit 1\nelif [ -z \"$PIR_AGENT_NAME\" ]; then\n echo \"PIR_AGENT_NAME is empty.\"\n exit 1\nelse\n echo \"Both VPN_AGENT_NAME and PIR_AGENT_NAME are set.\"\n echo \"VPN_AGENT_NAME is set to '$VPN_AGENT_NAME'.\"\n echo \"PIR_AGENT_NAME is set to '$PIR_AGENT_NAME'.\"\nfi\n\nVPN_AGENT_ORIGIN=$(readlink -f \"${CONFIGURATION_BUILD_DIR}/${VPN_AGENT_NAME}.app\")\nPIR_AGENT_ORIGIN=$(readlink -f \"${CONFIGURATION_BUILD_DIR}/${PIR_AGENT_NAME}.app\")\nAGENT_DESTINATION=\"${CONFIGURATION_BUILD_DIR}/${CONTENTS_FOLDER_PATH}/Library/LoginItems\"\n \n# Make sure that Library/LoginItems exists before copying\nmkdir -p \"$AGENT_DESTINATION\"\n \necho \"Copying VPN agent from $VPN_AGENT_ORIGIN to $AGENT_DESTINATION\"\nrsync -r --links \"$VPN_AGENT_ORIGIN\" \"$AGENT_DESTINATION\"\n \necho \"Copying Personal Information Removal agent from $PIR_AGENT_ORIGIN to $AGENT_DESTINATION\"\nrsync -r --links \"$PIR_AGENT_ORIGIN\" \"$AGENT_DESTINATION\"\n"; | |||
There was a problem hiding this comment.
You might want to look at this in Xcode. Basically removed the override and just use AGENT_PRODUCT_NAME.
I've also added a check to ensure that VPN_AGENT_NAME and PIR_AGENT_NAME are not empty, since if they're empty some really odd stuff happens later in the build.
diegoreymendez
commented
Jul 8, 2024
| @@ -54,7 +54,7 @@ AGENT_BUNDLE_ID[config=CI][sdk=*] = $(AGENT_BUNDLE_ID_BASE).debug | |||
| AGENT_BUNDLE_ID[config=Review][sdk=*] = $(AGENT_BUNDLE_ID_BASE).review | |||
|
|
|||
| AGENT_PRODUCT_NAME = DuckDuckGo VPN App Store | |||
| AGENT_RELEASE_PRODUCT_NAME = DuckDuckGo VPN | |||
| AGENT_PRODUCT_NAME[config=Release][sdk=*] = DuckDuckGo VPN | |||
There was a problem hiding this comment.
Quite honestly there should be no difference in just setting this directly, because in the end we were loading this value directly into the product name.
Additionally AGENT_PRODUCT_NAME is saved in Info.plist so that the code knows what the agent product name is - this is what was causing the issue since the app was trying to launch the VPN menu app with the wrong product name (loaded from info.plist).
diegoreymendez
commented
Jul 8, 2024
| @@ -44,7 +44,7 @@ CODE_SIGN_IDENTITY[config=CI][sdk=macosx*] = | |||
| PRODUCT_NAME_PREFIX = $(AGENT_PRODUCT_NAME) | |||
| PRODUCT_NAME = $(AGENT_PRODUCT_NAME) | |||
| PRODUCT_NAME[config=Review][arch=*][sdk=*] = $(AGENT_PRODUCT_NAME) | |||
| PRODUCT_NAME[config=Release][arch=*][sdk=*] = $(AGENT_RELEASE_PRODUCT_NAME) | |||
| PRODUCT_NAME[config=Release][arch=*][sdk=*] = $(AGENT_PRODUCT_NAME) | |||
There was a problem hiding this comment.
Read my comment below. Even though we always use AGENT_PRODUCT_NAME this is specifically expanded because the value of AGENT_PRODUCT_NAME is different for release builds, and Xcode otherwise seems to miss it.
github-actions
bot
added
the
bot: not in app board
diegoreymendez
requested review from
samsymons and
ayoy
and removed request for
ayoy and
samsymons
diegoreymendez
removed
the
bot: not in app board
samsymons
approved these changes
Jul 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/1203108348835387/1207749767717701/f
Description
App Store users can't uninstall the VPN due to an issue with the VPN menu app target name.
Testing
Since making a test App Store release build isn't trivial, this can be checked by looking at some files instead. That said once this is merged we should quickly make a new internal build and check that the VPN works correctly.
Test 1: Verify the product name is right
ManualAppStoreRelease.xcconfig... you need to uncomment a line in that file to make a manual buildAGENT_PRODUCT_NAMEisDuckDuckGo VPNDuckDuckGo VPN.appTest 2: Test the VPN in debug
Reset all changes and ensure the VPN works fine in debug builds.
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation