-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Make iOS export fast and easy #26263
Conversation
Example of the final
|
@@ -197,7 +199,7 @@ | |||
ProvisioningStyle = Automatic; | |||
SystemCapabilities = { | |||
com.apple.AccessWiFi = { | |||
enabled = $access_wifi; | |||
enabled = 0; |
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.
All games can still access wifi. This is poorly named (on Apple's part).
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.
yeah Apple has some strange ideas about these capabilities. Unless someone runs into a situation where this needs to be turned on, take it out.
|
||
// if enabled, add development (sandbox) APNS entitlements | ||
if ((bool)p_preset->get("capabilities/push_notifications")) { | ||
strnew += "\n\"CODE_SIGN_ENTITLEMENTS[sdk=iphoneos*]\" = " + binary_path + ".entitlements;\n"; |
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.
We can probably drop this in the future and let Xcode generate entitlements as well what it's already doing in this PR.
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'm a fan of letting XCode do as much of the work for us as possible, that way if apple decides to change/enhance it we should end up piggy backing on that change.
PRODUCT_NAME = "$(TARGET_NAME)"; | ||
PROVISIONING_PROFILE = "$provisioning_profile_uuid_debug"; | ||
TARGETED_DEVICE_FAMILY = "1,2"; | ||
VALID_ARCHS = "armv7 armv7s arm64 i386 x86_64"; |
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.
No more i386
OS X machines will run a recent version of Xcode :(
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 its worth discussing whether we want to get rid of the 32bit stuff as well. Apple is about to pull the plug on that anyway. Or maybe wait for 3.2 for that one?
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'd wait for 3.2, but then yeah, we can likely drop 32-bit stuff. See discussion in godotengine/godot-docs#2212.
@akien-mga I just realized there's a section in the docs about cross compiling iOS apps on Linux. I'm not sure if this is still supported, or even a goal of this project. Certainly the code signing aspects wouldn't work. This PR will further add a dependence on Xcode, but that's already required for submitting to the App Store. |
As long as we can still build the templates themselves on Linux (which we currently do for official templates), that should be fine :) |
@@ -243,26 +244,24 @@ void EditorExportPlatformIOS::get_export_options(List<ExportOption> *r_options) | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "custom_package/debug", PROPERTY_HINT_GLOBAL_FILE, "*.zip"), "")); | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "custom_package/release", PROPERTY_HINT_GLOBAL_FILE, "*.zip"), "")); | |||
|
|||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/app_store_team_id"), "")); | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/app_store_team_id", PROPERTY_HINT_PLACEHOLDER_TEXT, "ZXSYZ493U4"), "")); |
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.
Is ZXSYZ493U4 just a dummy value?
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's probably Sams team id so we can now all release games in his name 😉
@@ -100,7 +100,7 @@ | |||
D0BCFE2B18AEBDA2004A7AAE = { | |||
isa = PBXGroup; | |||
children = ( | |||
87D30C9722235A2200E688DC /* Config.xcconfig */ | |||
87D30C9722235A2200E688DC /* Config.xcconfig */, |
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.
Would be nice if you could squash this commit into its parent (40a2122b).
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.
and/or group some of the other related commits that iteratively modify the same file.
I'll tentatively put this on the 3.1 milestone even though it's quite late in the process, as the iOS export pipeline is far from optimal currently, so those changes might be very welcome in our new minor release. I'll need help from experienced iOS devs to validate these changes ASAP though, cc @bruvzg @BastiaanOlij @endragor. |
// This warnings detects when a function will recursively call itself on every | ||
// code path though that function. More information can be found here: | ||
// http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20131216/096004.html | ||
CLANG_WARN_INFINITE_RECURSION = YES |
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.
Missing newline at EOF.
@@ -19,3 +18,5 @@ WRAPPER_EXTENSION = app; | |||
|
|||
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; | |||
ASSETCATALOG_COMPILER_LAUNCHIMAGE_NAME = LaunchImage; | |||
|
|||
PRODUCT_NAME = "$(TARGET_NAME)"; |
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.
Missing newline at EOF.
@@ -6,7 +6,7 @@ | |||
CONFIGURATION_BUILD_DIR = "$(BUILD_DIR)/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)"; | |||
SDKROOT = iphoneos; | |||
VALID_ARCHS = "armv7 armv7s arm64 x86_64"; | |||
ENABLE_BITCODE = NO | |||
ENABLE_BITCODE = NO; |
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.
Would be good if this hotfix was amended into the commit where the line was added in the first place.
@@ -4,7 +4,7 @@ | |||
// Build Settings | |||
// | |||
SDKROOT = iphoneos; | |||
VALID_ARCHS = "armv7 armv7s arm64 x86_64"; | |||
VALID_ARCHS = armv7 armv7s arm64 x86_64; |
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.
Same for this one.
platform/iphone/export/export.cpp
Outdated
r_options->push_back(ExportOption(PropertyInfo(Variant::INT, "application/export_method_release", PROPERTY_HINT_ENUM, "App Store,Development,Ad-Hoc,Enterprise"), 0)); | ||
|
||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/name", PROPERTY_HINT_PLACEHOLDER_TEXT, "Game Name"), "")); | ||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/info"), "Made with Godot Engine")); | ||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/identifier", PROPERTY_HINT_PLACEHOLDER_TEXT, "com.example.game"), "")); | ||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/public_version"), "1.0.0")); | ||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/version"), "1.0.0")); | ||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/copyright"), "")); | ||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/copyright"), "2019")); |
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.
Is application/copyright
meant to be a year?
@@ -257,7 +257,7 @@ void EditorExportPlatformIOS::get_export_options(List<ExportOption> *r_options) | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/identifier", PROPERTY_HINT_PLACEHOLDER_TEXT, "com.example.game"), "")); | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/public_version"), "1.0.0")); | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/version"), "1.0.0")); | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/copyright"), "2019")); | |||
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/copyright"), "")); |
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 answers my previous question... should be squashed in the commit where it was changed.
The changes look good to me overall, but I'd like the git history to be amended a bit to make it easier to follow the changes, removing intermediate changes that are no longer relevant once merged. So typo fixes and cleanups should be made directly in the original commit that added the need for such fixes. The final commits that we do merge should not add bugs, even temporarily; each commit should bring the engine from one valid state to another valid state, so that changes can be bisected if need be. |
I'd very much like to push this to 3.2, maybe it can go in 3.1.1 if we have to. This will take a while to sort out on my end. |
I agree that we shouldn’t rush this out. This is a vast change that deserves more testing. |
Is this going to be in 3.2? Very much looking forward to it! |
Moving to 4.0 milestone as this is not ready to merge. |
See also #33086 for a related PR with probably a smaller scope. |
Remove CFBundleSignature since that key is for legacy OS X apps Rename short_version to public version for clarity. Use 2 periods for version string as Apple recommends
Add missing comma in xcode project. Add xcconfig file Skip missing icons on export Remove IPHONEOS_DEPLOYMENT_TARGET and use value in xcconfig Remove asset catalog references from xcodeproj Include a Common.xcconfig so we can modify the godot xcconfig Export to xcodeproj now works with automatic profile and certificate selection Default to iphone developer and distribution Migrated almost all build variables to xcconfig Move more permanent settings to Common.xcconfig Add CLANG_WARN_SUSPICIOUS_MOVE to silence Xcode project upgrade Enable testability in debug to prevent project upgrade warning Prevent empty signing identity
84e65b6
to
014afbf
Compare
@akien-mga @godotengine/ios This has been rebased and updated. |
@samgreen What is the status of this PR? The two PRs linked by Akien are both closed. If this is still desired, this should be rebased and re-tested so that we know it still works. |
I don’t have a good answer here. I’m happy to make this and any further changes. I’d love to see iOS export get some love. This is a huge barrier to wider adoption. |
@samgreen Hi, I am kinda new to Godot, but not to Xcode. One thing I don't understand is why the Godot "runtime" isn't exported to Xcode as source instead of built binary, which would allow Xcode to build the source for device or simulator? This would presumably also cut down on the maintenance effort of the exporter itself? |
There is no Xcode on Linux. |
No what I meant was the iOS exporter is comprised of the source files of the Godot runtime and the necessary Xcode project files so when the user exports to iOS on their Mac they then built it to whatever target they like (device, simulator, arm64, x64, whatever). Has this not been considered? |
I presume doing this would slow down exporting speeds, since the native code would have to be compiled when you export the project (at least for the first time). Nonetheless, this is how custom builds for Android work (they're still opt-in right now). By shipping precompiled libraries within the iOS export template ZIP, we avoid this slowdown. |
It would be slower for sure, but with the mix of arm64 and x64 Macs and arm64 and x64 iOS Simulators it might prove useful to some users. Avoiding having to provide all the icons in the export dialog would be a win if you already a pre-prepared |
I can test this change but I'm having trouble rebasing onto master or 3.x as there's a bunch of conflicts in godot_ios.xcodeproj/project.pbxproj that I don't understand. If someone resolves them, I can test it on my end. |
Seems that if this was retested we could see if we merge it or not |
This PR is sadly abandoned, but #70662 should supersede it. |
I'd like to get to 1-click deploy from the editor for iOS games. The first step is ensuring the export pipeline is solid, up-to-date and has very reasonable defaults.
Changes
iPhone Developer
andiPhone Distribution
to pick from available code signing identities (Beward, this can cause iOS export configuration needs better validation #24866 and loss of sanity). The old behavior is still here if you choose to modify the export settings. Most of this process should now be automatic for most users.xcodeproj
much less. We want to stay away from modifying this file and instead let Xcode manage it. To that end, most settings have been moved toxcconfig
files.Common.xcconfig
these settings are applied to all (Debug and Release) buildsgodot.xcconfig
and add toxcodeproj
Resolves
.xcconfig
file.Clean Up
CFBundleSignature
fromplist
(This is an OS X key).CFBundleIcons
from `plist (We previously migrated to Asset Catalogs)Proposal for future work
capabilities
and what they mean.