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

Get BUNDLE_IDENTIFIER from Config.xcconfig for use in Fastfile #17

Merged
merged 2 commits into from
Mar 30, 2024

Conversation

bjornoleh
Copy link
Contributor

@bjornoleh bjornoleh commented Mar 18, 2024

  • Allows building of different instances of the app by only editing Config.xcconfig.

- Replace $(DEVELOPMENT_TEAM) parsed from BUNDLE_IDENTIFIER in Config.xcconfig with ENV["TEAMID"]
- Use BUNDLE_ID instead of hard coded bundle identifier in Fastfile, inspired by Artificial-Pancreas/iAPS 6a46a2a by @Jon-b-m 
- Allows building of different instances of the app by only editing Config.xcconfig.
@bjornoleh
Copy link
Contributor Author

bjornoleh commented Mar 19, 2024

Edit: backing out the last commit that included "Add branch+commit to "What to Test" field in TestFlight ", and created an issue report for this instead: #23

@bjornoleh bjornoleh requested review from Sjoerd-Bo3 and dnzxy and removed request for Sjoerd-Bo3 and dnzxy March 19, 2024 21:50
@dnzxy
Copy link
Contributor

dnzxy commented Mar 19, 2024

Not at all a fan of this sorry. I’ve spent hundreds of hours providing support for browser build; and the one simple thing that makes this somewhat possible is the standardization of builds with very very few outliers. Changing bundle IDs is an expert / advanced user thing; this really is not required for everyday use and shouldn’t be part of the main repo.

edit to say: unless this is trying to solve a common use case (which I don’t see)

@Sjoerd-Bo3
Copy link
Contributor

Sjoerd-Bo3 commented Mar 20, 2024

I thought it was a nice feature, but I can get behind Dan. First time this will change since freeaps, so this is a change for a onetime thing. But @dnzxy , do you think this will affect issues? Because changing the config makes problems anyway, dont really see how this really worsens it. If the consensus is that it does, than why not get it out of the config and hardcode it everywhere?!

@bjornoleh
Copy link
Contributor Author

It was certainly not meant to be advertised for end users. People do all sorts of crazy stuff, but at the slightest deviation from the docs, I guess the answer should be “delete your main branch and recreate it from upstream”.

This feature is quite convenient for developers and advanced users/testers. And if we change the default bundle ID now, we will have to make some changes to Fastfile anyways. Also if we rebrand later. If we do this now, that it already covered, and we only need to edit Config

@dnzxy
Copy link
Contributor

dnzxy commented Mar 20, 2024

Good point. I don’t really see an issue with us changing the config… that will need a lot of hand holding for folks migrating. But the important thing is to have a standardized setup so you can point folks to issues and docs, and it’s easier to pinpoint where folks missed configuration steps and that’s why their builds are failing. I like the simplicity this adds for folks that want to change things, but I think this potentially may cause more issues than do good; similar to when an app opens all possible settings to the user and they start tweaking too many knobs at once 🫠

@MikePlante1
Copy link
Contributor

This is already the method used for altering BundleID of Xcode builds, so if it’s shot down for browser build, shouldn’t it be removed as an option for Xcode builders?

@bjornoleh
Copy link
Contributor Author

@dnzxy , Are you still reluctant about merging this? If not, LGTM :-)

@dnzxy
Copy link
Contributor

dnzxy commented Mar 28, 2024

Nope, I was just waiting on Marion‘s testing (as I‘m nowhere near a Mac this week and phone only) 😅 should be good to go!

Edit: Oh, I just realized that this is a different PR. I‘m still not sure if this is a good thing - but from the code it looks good and I‘d be okay to see it merged if the common decision here is to have it in there 👍 as I wrote in my initial comment, this might open us up to a lot of support issues (first level support).

@marionbarker
Copy link
Contributor

I was a little confused by this during code review (mentioning what confused me as I went along - first 2 bullets turn out to be irrelevant)

  • Config uses DEVELOPER_TEAM, not DEVELOPMENT_TEAM as key = value pair.
  • ConfigOverride gets read in after Config
  • Since Fastfile is only used for browser build, this should be OK
    • it has access to the actual TEAMID as an environment variable
    • Browser Build folk will not create an Override file in their fork - they don't need one
  • I have done no testing of this - I will do that and report in a separate comment

details:

These lines are in Config.xcconfig:

APP_DISPLAY_NAME = OiAPS
APP_VERSION = 2.3.3
APP_BUILD_NUMBER = 1
COPYRIGHT_NOTICE =
DEVELOPER_TEAM = ##TEAM_ID##
BUNDLE_IDENTIFIER = ru.artpancreas.$(DEVELOPMENT_TEAM).FreeAPS
APP_GROUP_ID = group.com.$(DEVELOPMENT_TEAM).loopkit.LoopGroup
APP_ICON = OiAPS_Icon
APP_URL_SCHEME = freeaps-x

#include? "ConfigOverride.xcconfig"
#include? "../../ConfigOverride.xcconfig"

Note - the final line currently has a comment, but it really should be in there without a comment to help developers who have multiple clones in an organized folder structure and use Mac build.

The contents of the override file is

// Automatic Signing File
DEVELOPER_TEAM = <TEAMID>

@bjornoleh
Copy link
Contributor Author

Yes, the override file is irrelevant here, since it is in .gitignore and will not be commit to the fork.

I opened #34 regarding placement of config override.

@marionbarker
Copy link
Contributor

I made a test branch and built successfully.

Details:

  • created branch fastlane-test-bundle-id and pulled in changes from Bjorn
  • modified Config.xcconfig (using bogus BUNDLE_IDENTIFIER = mdb.fastlane-test.$(DEVELOPMENT_TEAM).FreeAPS)
  • made branch fastlane-test-bundle-id the default
  • ran build (expecting it to fail and it did)
    • Link to failure to assist in putting errors into documentation
    • Next actions were successful
      • Add Identifiers
      • On Apple page, add Loop app group to the 3 new identifies and then created new bogus app on the appstoreconnect page
      • Create Certificates
      • Build iAPS, link to success
  • create a testing group for bogus app and add myself
  • install from TestFlight
  • Brand new and distinct app is not on my test phone

@marionbarker
Copy link
Contributor

One more item to fix in testflight.md, - suggested change:

diff --git a/fastlane/testflight.md b/fastlane/testflight.md
index 5187168a..ee83941b 100644
--- a/fastlane/testflight.md
+++ b/fastlane/testflight.md
@@ -142,7 +142,8 @@ If you have created a Open-iAPS app in App Store Connect before, you can skip th
     * Select "iOS".
     * Select a name: this will have to be unique, so you may have to try a few different names here, but it will not be the name you see on your phone, so it's not that important.
     * Select your primary language.
-    * Choose the bundle ID that matches `ru.artpancreas.TEAMID.FreeAPS`, with TEAMID matching your team id.
+    * Choose the bundle ID that matches the BUNDLE_IDENTIFIER in your `Config.xcconfig` file
+       * this is `typically org.nightscout.TEAMID.openiaps` with TEAMID matching your team id
     * SKU can be anything; e.g. "123".
     * Select "Full Access".
 1. Click Create

@bjornoleh
Copy link
Contributor Author

I will take the input here so far as support for this feature, and am merging this now, so we can proceed with other PRs :-)

testflight.md will be updated in #31

@bjornoleh bjornoleh merged commit 1d3c8f0 into nightscout:dev Mar 30, 2024
@MikePlante1 MikePlante1 mentioned this pull request Mar 30, 2024
4 tasks
aug0211 added a commit to avouspierre/Trio that referenced this pull request Jul 7, 2024
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.

None yet

5 participants