Skip to content
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 Mac App Store target #927

Merged
merged 111 commits into from
Jan 26, 2023
Merged

Add Mac App Store target #927

merged 111 commits into from
Jan 26, 2023

Conversation

ayoy
Copy link
Collaborator

@ayoy ayoy commented Jan 12, 2023

Task/Issue URL: https://app.asana.com/0/72649045549333/1203407505908963/f

Description:

  • This change adds 3 new targets to the app: DuckDuckGo Privacy Browser App Store for the app
    and Unit Tests App Store and Integration Tests App Store for tests.
  • Schemes for building debug/release and review app were also duplicated.
  • InputFilesChecker SPM Build Tool plugin has been added to input files of App Store
    and non-App Store targets in sync.
  • App Store app uses com.duckduckgo.mobile.ios bundle identifier (together with .review and .debug suffixes)
  • App Store app uses App Sandbox. Because of that we’re disabling Bitwarden integration
    in the App Store build. This is the only user-facing change compared to the DMG distribution.
  • Because FileManager API returns paths from within app container, user’s Library and Application Support
    directories are computed relative to home directory for the sandboxed target. Hence existence of
    URL.nonSandboxLibraryDirectoryURL and URL.nonSandboxApplicationSupportDirectoryURL.
  • Sandboxed app deployment target is macOS 12.3 to remove as many private API calls as possible.
    Remaining usage was converted to utilize performSelector so that it’s resolved in runtime.
  • App Store builds define APPSTORE compile time flag and this one is used to conditionally exclude some
    code, especially references to private API calls. It turns out that if #available(macOS 12.0, *) is not enough
    to get rid of private API symbols landing inside the resulting binary.
  • archive.sh was updated with a possiblity to make “App Store” review and release builds (effectively
    just notarized sandbox builds).
  • App Store app uses manual signing for distribution and fastlane match was set up to simplify it.
  • App Store GitHub actions workflow was added.
  • There are some minimal changes to seemingly unrelated code – this is to limit the number of compile
    warnings after bumping deployment target to 12.3 for the App Store target.

Steps to test this PR:
Setting up development environment:

  1. Run bundle install (if needed).
  2. Get Apple certificates encryption password from Last Pass item called Fastlane match Encryption password.
  3. Run bundle exec fastlane sync_signing username:<your_ddg_apple_id> to set up App Store certificates and provisioning profiles. You will be asked for the fastlane match password which will be stored in the keychain.
  4. Open Xcode, verify that signing for the App Store target looks ok (no errors). Note that fastlane match is currently only enabled for the App Store target.
  5. Verify that you can build the app.
  6. Verify that you can archive the app.
  7. Verify that you can build and archive the non-App Store app.

Testing the app

  1. Run the app, verify that it starts from scratch (no user data because it’s a different data directory)
  2. Go to Autofill Settings and verify that Bitwarden is not there
  3. Try importing data, verify that importing works the same as in the non-App Store build.
  4. Verify that downloads work (including setting custom downloads directory).
  5. Smoke test the app (e.g. try camera/location permissions) and verify that it doesn’t behave different from the non-App Store build.

Other

  1. Try archive.sh and its new options to build different app flavors.
  2. Here is a sample run of the new App Store GitHub Actions workflow

Testing checklist:

  • Test with Release configuration
  • Test proper deallocation of tabs
  • Make sure committed submodule changes are desired
  • Make sure configuration is changed only in xcconfig files, not in the Xcode project file directly

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Kudos for finding a solution to all problems that appeared. 👏

As a follow up, it would be fine to have option to clean data for App Store build with clean-app.sh. But that is a small detail you don't need to worry about now.

Configuration/Version.xcconfig Outdated Show resolved Hide resolved
@ayoy ayoy assigned samsymons and unassigned tomasstrba Jan 13, 2023
Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as far as I can tell! Followed through all of the testing steps, and generally just smoke tested everything. Great work!

@samsymons samsymons assigned ayoy and unassigned samsymons Jan 16, 2023
@@ -38,6 +38,9 @@ extension WKWebView: WKWebView_macOS_11_3 {
extension WKWebView {

func startDownload(_ request: URLRequest, completionHandler: @escaping (WebKitDownload) -> Void) {
#if APPSTORE
startDownload(using: request, completionHandler: completionHandler)
#else
if #available(macOS 11.3, *) {
(self as WKWebView_macOS_11_3).startDownload(using: request, completionHandler: completionHandler)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the WKWebView_macOS_11_3 protocol can be safely removed as the API availability bug has been fixed in Xcode

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out @mallexxx! I’ve removed this protocol in this commit. I’ve tested it on both targets, but I’d appreciate if you could have a look just to double check. Thanks a lot :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, looks good 👍

@github-actions
Copy link
Contributor

github-actions bot commented Jan 16, 2023

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 8c9a68f

@ayoy ayoy merged commit d523579 into develop Jan 26, 2023
@ayoy ayoy deleted the dominik/appstore branch January 26, 2023 09:58
samsymons added a commit that referenced this pull request Jan 26, 2023
# By Dominik Kapusta (4) and others
# Via GitHub
* develop:
  Blockable JS Alerts inside tabs (#904)
  Fix add bookmark / folder modal in fullscreen (#930)
  Add Mac App Store target (#927)
  Pull request review checklist added to the pull request template (#935)
  Version Bump to 0.31.7
  Update embedded files
  Update BSK with autofill 6.1.2 (#942)
  Migrate to targeted Swift Concurrency checking (#937)
  Add bundler configuration to limit Gemfile.lock noise (#939)
  update openssl depedency (#918)
  Remove tab content when showing TabContent.none (such as for new tabs opened from navigation links) (#938)
  Update GRDB to 2.0.0 (upstream 6.6.0, SQLCipher 4.5.3) (#933)
  Sparkle 2 (#934)
  Bump Submodules/privacy-reference-tests from `de75d51` to `4fdbbb6` (#932)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants