Make team id, app id and a group prefix specifiable in an xcconfig

[roop]

Task/Issue URL: Issue #479
Tech Design URL: -
CC: @brindy @bwaresiak

Description:

To make it easier for external developers to contribute, this PR takes out team id, app id and group prefix out of the project file / entitlements files. Instead, they are specified in an xcconfig file at Configuration/Developer.xcconfig which is kept out of version control.

For developers who are part of the DuckDuckGo team, the Configuration/Developer.xcconfig should have the following content:

DEVELOPMENT_TEAM = HKE973VLUW
APP_ID = com.duckduckgo.mobile.ios
GROUP_ID_PREFIX = group.com.duckduckgo

To enable the group ids need to be accessed from code, the group id prefix is added to the Info.plists as necessary, so that the prefix can be obtained by examining the bundle in code.

For this, Core/global.swift now defines a struct Global. Having a Global defined inside a global.swift doesn't look very good. I think we should rename it to Global.swift and bring isDebugBuild into struct Global, but that should probably be done outside of this PR. What do you think?

Steps to test this PR:

1. If you're part of the DuckDuckGo team, create a Configuration/Developer.xcconfig as specified above

2. Build and run the project as before

3. Make sure the app and extensions work normally

   There are 3 app groups here:

   1. Bookmarks (Used by app, bookmarks widget)
   2. Content Blocker (Used by app only)
   3. Statistics (Used by app, bookmarks widget, quick actions widget)

   So we should test these combinations:

   1. Test that bookmarks work from both the app and the bookmarks widget.
   2. Test that atb statistics are correctly handled from the app, the bookmarks widget and the quick actions widget (I don't know how to test this)

---

Internal references:

Software Engineering Expectations
Technical Design Template

[roop]

It appears that the ci is using a different bitrise.yml file (maybe something configured on their website?), so my changes to the bitrise.yml in the repo aren't being used.

Give this, I'm not sure if my changes to bitrise.yml in commit 1eb51eb are correct, or necessary. I need your help in getting this auto-tested correctly.

[brindy]

I think the yml is just a backup as they're not in sync at the moment anyway.

How about committing Configuration/Developer.xcconfig?

DDG developers at the least should be able to check out, build and run without any additional config. I sometimes have multiple checkouts depending on what I'm doing so it would be painful to have to create this file each time.

[roop]

Since the bitrise.yml in the repo isn't actually used, I'll retract commit 1eb51eb.

We could commit the xcconfig too, but the disadvantage would be that external developers would still have to deal with making sure they don't push their changes to the xcconfig. That is still an improvement over the current scenario because they'll just have one file to change.

To compare:

• Option 1: Keep Developer.xcconfig in the repo

  DDG developers continue to work exactly as they do now.

  External developers need to modify the xcconfig file before they build, but they need to take care to not push that change.

• Option 2: Keep Developer.xcconfig out of the repo

  Both DDG developers and external developers need to ensure the xcconfig is setup for every clone or work tree of the repository. If you use multiple clones or use git-worktree, you could set up a script to make this easier (the script could be made part of the repo too).

Option 2 puts both DDG developers and external developers on equal footing, which I think is a nice way of welcoming external contributions. Nevertheless, I think you should make the call on which is more important here.

[brindy]

Option 1 is preferred for simplicitly, please - it'll still be a lot easier than it has been thus far and with a note in the README hopefully people will see what they need to do.

New developers can use a git command to mark that file as unchanged for themselves locally:

git update-index --assume-unchanged Configuration/Developer.xcconfig

We will catch changes to that file during a code review.

Thanks!

[roop]

Okay.--assume-unchanged is a great idea.

[brindy]

Hi @roop - we've been at our team offsite this week so apologies for the radio silence. @bwaresiak and I were discussing it and he had a good idea, though I can't quite remember the details. It's not far off from this though.

@bwaresiak do you want to elaborate here so we can discuss, please?

[bwaresiak]

@brindy sure!

@roop I like your idea of creating Configuration/Developer.xcconfig and making it customizable. What I'm concerned about is introducing some friction to PR process, specifically:

• Contributors having to ignore changes manually.
• Reviewers having to watch out for config changes.

What would you think about something like that:

• Keep Configuration/Developer.xcconfig as default one as it is now.
• Add optional include #include? "Custom.xcconfig" at the end of it.
• Add Custom.xcconfig to .gitignore.

Now, whenever contributor needs to change configuration, he will:

• Copy Developer.xcconfig and rename it as Custom.xcconfig.
• Make any changes needed in Custom.xcconfig. Changes from this config will override default ones.

Since that file will be ignored by git, nothing should be committed to the repo.

There are alternatives to that (like auto-generated config files that are ignored by git, but could be changed easily by contributors) but I'd prefer simpler approach.

What do you think about it?

[roop]

@bwaresiak I didn't know we can have optional includes. This looks very good. I'm assuming Custom.xcconfig will not be committed to the repo.

Both problems are solved cleanly:

• A member of the DuckDuckGo team (and Bitrise) can just clone and build without any additional step in between
• An external contributor can set it up once and not have to worry about xcconfig changes getting into a PR

I'm not sure I like the name Custom.xcconfig, though. Maybe ExternalDeveloper.xcconfig or DeveloperOverride.xcconfig?

[bwaresiak]

I didn't know we can have optional includes. This looks very good.

Glad you like it! :)

I'm assuming Custom.xcconfig will not be committed to the repo.

Yes.

I'm not sure I like the name Custom.xcconfig, though. Maybe ExternalDeveloper.xcconfig or DeveloperOverride.xcconfig?

To be honest, either is fine with me (tho I feel that shorter = better, maybe just Override ?). I've assumed purpose of that file will be documented within Developer.xcconfig (above the include) and thus name does not matter that much.

[roop]

@bwaresiak Okay, I'll go with DeveloperOverride.xcconfig then.

When the external developer copies Developer.xcconfig, the #include? will be copied too, so I'm thinking we should ask that that line be removed. (I didn't see Xcode complaining, but I think it's cleaner if that's removed.)

[roop]

On second thoughts, here's a slight modification of your idea:

1. We have a Configuration/Configuration.xcconfig that just has these lines:

   #include "DuckDuckGoDeveloper.xcconfig"
   #include? "ExternalDeveloper.xcconfig"
   

2. The Configuration/DuckDuckGoDeveloper.xcconfig file contains DDG-specific values
3. An external contributor shall copy DuckDuckGoDeveloper.xcconfig to ExternalDeveloper.xcconfig and edit all fields.
4. Configuration/Configuration.xcconfig and Configuration/DuckDuckGoDeveloper.xcconfig are committed to the repo, and Configuration/ExternalDeveloper.xcconfig is kept out of the repo.

This way, we don't have to ask people to remove the #include?, so it's a bit cleaner.

[bwaresiak]

Good idea, that's even better. 👍

[bwaresiak]

Hello @roop

Just FYI so you know what's going on: I'll be committing some time to look at & test this next week, as I still have some things I want to finish by the end of this week. Thanks for your patience. :)

Also I've noticed that DDG group id is still hardcoded for AppUserDefaults initializator (that is used later as UserDefaut's suite name). Everything else looks good at the first glance. 👍

[roop]

I'll be committing some time to look at & test this next week.

Sure, ok.

Also I've noticed that DDG group id is still hardcoded for AppUserDefaults initializator (that is used later as UserDefaut's suite name). Everything else looks good at the first glance.

I didn't see a reason to change that and make it dependant on the GROUP_ID_PREFIX since the app can be built and run without having to do that. Do you think it should be un-hardcoded?

[bwaresiak]

I gave it some thought: initially my reasoning was that it would be nice to have it not hardcoded, in case we decide UserDefault's suite is to be group-shared (as that suite name could indicate that). But I guess, that since that particular group is not specified within entitlements, then it doesn't matter and whoever changes that, will be careful enough to set it up properly.

[bwaresiak]

Nice job! With this it is easy to run development version along regular installation.

Few notes:

1. 'Home screen widget' plist field should also take bundle ID from configuration (otherwise for app with customized bundle id that widget won't appear after long-pressing app icon).
2. Tested on Xcode 11.0: One has to remember to clean whole project after adding ExternalDeveloper.xcconfig otherwise Xcode won't apply changes properly (it seems to work well when removing that file). Maybe we should add info about this to readme?
3. Tested on the simulator, so this may be the factor: There is something tricky going on with linking to app from extensions (e.g. bookmarks): when I have two apps installed at the same time (with different bundle ids, say "original" and "external") the "original" DDG app is opened no matter which extension I use. When I remove "original" one, the "external" one is opened as expected. I've checked the plist files/config but did not see anything suspicious.

[roop]

Nice job! With this it is easy to run development version along regular installation.

Ah, yes. That's true.

1. 'Home screen widget' plist field should also take bundle ID from configuration (otherwise for app with customized bundle id that widget won't appear after long-pressing app icon).

The PR already derives the bundle IDs of extensions (BookmarksTodayExtension, QuickActionsTodayExtension and ShareExtension) from the APP_ID in the xcconfig (commit 3477410).

2. Tested on Xcode 11.0: One has to remember to clean whole project after adding ExternalDeveloper.xcconfig otherwise Xcode won't apply changes properly (it seems to work well when removing that file). Maybe we should add info about this to readme?

I didn't really test this scenario. An external developer will be able to build only after he creates a ExternalDeveloper.xcconfig, so this will be only for DuckDuckGo employees who want to switch between the two xcconfigs. Do you think this should be on the readme?

3. Tested on the simulator, so this may be the factor: There is something tricky going on with linking to app from extensions (e.g. bookmarks): when I have two apps installed at the same time (with different bundle ids, say "original" and "external") the "original" DDG app is opened no matter which extension I use. When I remove "original" one, the "external" one is opened as expected. I've checked the plist files/config but did not see anything suspicious.

The extensions open the DDG app using URL schemes (like ddgQuickLink:// from the bookmarks extension). Both apps ("original" and "external") register themselves as the handling app for the same URL scheme, and in that case iOS is free to pick one of them arbitrarily.

[bwaresiak]

The PR already derives the bundle IDs of extensions (BookmarksTodayExtension, QuickActionsTodayExtension and ShareExtension) from the APP_ID in the xcconfig (commit 3477410).

What I meant is Main project's Info.plist file and Home screen widget field, see:

The exact line. Try long-pressing app icon on iOS 13 to see the issue. :)

I didn't really test this scenario. An external developer will be able to build only after he creates a ExternalDeveloper.xcconfig, so this will be only for DuckDuckGo employees who want to switch between the two xcconfigs. Do you think this should be on the readme?

Yes, I'd add 'Clean and rebuild project' as the last step.

The extensions open the DDG app using URL schemes (like ddgQuickLink:// from the bookmarks extension). Both apps ("original" and "external") register themselves as the handling app for the same URL scheme, and in that case iOS is free to pick one of them arbitrarily.

Yes, but in plist file where custom URL schemes are specified, there is also Identifier field ( PRODUCT_BUNDLE_IDENTIFIER ) that according to documentation:

The identifier you supply with your scheme distinguishes your app from others that declare support for the same scheme. To ensure uniqueness, specify a reverse DNS string that incorporates your company’s domain and app name. Although using a reverse DNS string is a best practice, it does not prevent other apps from registering the same scheme and handling the associated links.

First sentence suggest that identifier is used to distinguish apps (and I interpret the last one as: other apps that can "hijack" scheme if using same identifier). Nonetheless I don't see anything wrong in your PR - as PRODUCT_BUNDLE_IDENTIFIER is populated using APP_ID - so I wonder if that's an issue on iOS side...

One more thing: what about adding Configuration/ExternalDeveloper.xcconfig to .gitignore? :)

[roop]

The exact line. Try long-pressing app icon on iOS 13 to see the issue. :)

I understand now. Sorry about missing that. :(

Yes, I'd add 'Clean and rebuild project' as the last step.

Ok.

First sentence suggest that identifier is used to distinguish apps (and I interpret the last one as: other apps that can "hijack" scheme if using same identifier). Nonetheless I don't see anything wrong in your PR - as PRODUCT_BUNDLE_IDENTIFIER is populated using APP_ID - so I wonder if that's an issue on iOS side...

I see it as: The identifier is indeed used to distinguish apps (though it's a little weird that it needs a separate field for that instead of just using the bundle id). Nevertheless, when two apps have registered for the same URL scheme, what can iOS do anyway? iOS knows from the identifiers that two different apps have registered, but it's unclear which should be launched, hence the line saying "it does not prevent other apps from registering the same scheme and handling the associated links".

One more thing: what about adding Configuration/ExternalDeveloper.xcconfig to .gitignore? :)

Missed that. Will add that.

[bwaresiak]

I understand now. Sorry about missing that. :(

No problem :)

I see it as: The identifier is indeed used to distinguish apps (though it's a little weird that it needs a separate field for that instead of just using the bundle id). Nevertheless, when two apps have registered for the same URL scheme, what can iOS do anyway? iOS knows from the identifiers that two different apps have registered, but it's unclear which should be launched, hence the line saying "it does not prevent other apps from registering the same scheme and handling the associated links".

That makes sense. Seems like I was expecting too much: optional matching the identifier of the app invoking the URL call with the identifier of the one that is supposed to handle it. Nothing to do here. :)

[bwaresiak]

One more thing that came to my mind: can you include license header (same as the one we prepend the source code with) in xcconfig files?

[roop]

One more thing that came to my mind: can you include license header (same as the one we prepend the source code with) in xcconfig files?

Just to be clear, are you talking about what Xcode populates a file with when we create a new file in the project? AFAIK we can't specify that in an xcconfig, but we can set up an Xcode template header like this.

[bwaresiak]

What I mean is that I think it would be good to add Copyright notice (see AppDelegate lines 5-17) to the files you've created Configuration/Configuration.xcconfig & Configuration/DuckDuckGoDeveloper.xcconfig.

Sorry if my previous message was unclear :)

[bwaresiak]

Everything looks good to me, thank you for contribution 👍 :)