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.
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
fix(client/cordova/apple/ios): fix failing release build for the cordova ios target #1840
fix(client/cordova/apple/ios): fix failing release build for the cordova ios target #1840
Changes from 2 commits
667850941f6a222807d7dc8f74ecFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbruens will this break mac Catalyst?
I don't understand why having this would break the build, since an app can be in multiple application groups. Perhaps we need to configure the application group to include the ios app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when it fails it says that the provision (
org.outline.ios.client) doesn't match what's in this group (e.g. the macos group). perhaps we can use the interpolation to change this at compile time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will most likely break macOS, because they share this file.
Please wait for @sbruens to chime in.
Other than that, Makefile and pbxproj look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will break catalyst because this app group is used in several places to share data between the network extension, launcher and host app, like the logs directory for Sentry and the tunnel store (see https://github.com/search?q=repo%3AJigsaw-Code%2Foutline-client%20%22QT8Z3Q9V3A.org.outline.macos.client%22&type=code).
Those values need to match the app groups the app has access to. I think we may be able to move catalyst over to use the ios app group but it would mean that the catalyst app would no longer use the existing mac paths for logs and tunnel store preferences, but that might be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I added those entitlements very recently: #1811
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the iOS app provisioning profile needs to be updated online to add those groups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is that macOS groups are special and not managed in the Developer portal. So you can't even create an app group that doesn't start with
group..That app groups shouldn't be set on the iOS app anyway. But the XCode UI is weird where it automatically adds macOS app groups to the iOS section (but not the other way around?). Is there a way to set them in the release script?
I guess the best course of action would be to update those hard-coded values and move catalyst over to the iOS app group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the user lose their server list? That would be a very bad experience.
Perhaps we need separate projects? Can we have a per-platform plist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. They are not stored under the app group.
@daniellacosse you should be able to check that by running the catalyst app after you change these values and checking that your servers show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, still seeing my servers: