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

chore(ios,mac): support build on Apple Silicon using Xcode 15.3 #11302

Merged
merged 17 commits into from
May 2, 2024

Conversation

sgschantz
Copy link
Contributor

@sgschantz sgschantz commented Apr 25, 2024

Several changes to get Keyman for iOS and macOS building on Apple Silicon on macOS Sonoma with the latest version of Xcode 15.3.

Most significantly, the minimum supported version of macOS had to be increased from 10.10 (Yosemite) to 10.13 (High Sierra).

Fixes #10671

@keymanapp-test-bot skip

@sgschantz sgschantz added the ios/ label Apr 25, 2024
@sgschantz sgschantz added this to the B17S6 milestone Apr 25, 2024
@sgschantz sgschantz self-assigned this Apr 25, 2024
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Apr 25, 2024
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Apr 25, 2024
@@ -0,0 +1,11 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#!/bin/bash
#!/usr/bin/env bash

We should be using env throughout to signify that we want the bash version from the path, even though we are attempting to run the scripts with the correct version of bash.

@sgschantz sgschantz marked this pull request as ready for review April 26, 2024 03:45
Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

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

LGTM.

That said, the minimum version increase, this late in the game... makes me wonder if we should do a first release before the update lands, then follow up with this one.

Then again, if anything feels broken with the release... we probably don't want to leave users with 10.10 - 10.12 out to dry. So we probably shouldn't use it as a reason, even if it's unfortunate to shift minimum-requirements despite the prior announcement.

https://community.software.sil.org/t/keyman-17-0-beta-release/8388

  • Minimum supported version of macOS is 10.10 Yosemite.

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

Nits, plus the change from #!/bin/bash to #!/usr/bin/env bash for all the build scripts. But looking good.

  • We should use resources/build/mac/ folder for the "embedded" scripts?
  • We need to update the oem/fv xcodeproj as well
  • Several of the embedded shellscripts have not yet been extracted. We need to do the wrapping for all the scripts in the xcodeproj files.


Keyman should also work on future releases of macOS, even if they are not yet listed here.

## Resource Requirements

Keyman for macOS has minimal resource requirements. Any computer running
Mac OS X 10.10 or later should be able to run Keyman for macOS without trouble.
Mac OS X 10.13 or later should be able to run Keyman for macOS without trouble.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mac OS X 10.13 or later should be able to run Keyman for macOS without trouble.
macOS 10.13 or later should be able to run Keyman for macOS without trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

keymanapp/keyman.com@486df1b updates the minimum version listed on keyman.com

@@ -3,4 +3,4 @@ title: What are Keyman's hardware requirements?
---

Keyman for macOS has minimal resource requirements. Any computer running
Mac OS X 10.10 or later should be able to run Keyman for macOS without trouble.
Mac OS X 10.13 or later should be able to run Keyman for macOS without trouble.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mac OS X 10.13 or later should be able to run Keyman for macOS without trouble.
macOS 10.13 or later should be able to run Keyman for macOS without trouble.

@mcdurdin
Copy link
Member

Note re sentry update:

chore(mac): upgrade sentry pod to 8.24.0

Existing version of Sentry pod, 5.x, had a mac deployment
target of 10.10, but with the upgrade to XCode 1111.66, we
must have a minimum target of 10.13. The min target came in
with sentry 8.0.0, but we opted to go straight to the latest
released minor version in 8.0 series.

This forced an update to cocoapods to support visionos, and
that forced an upgrade of Ruby away from the macOS supplied
2.6.8 to 3.3.1.

Used https://www.moncefbelyamani.com/how-to-install-xcode-homebrew-git-rvm-ruby-on-mac/
to upgrade, seemed to work.

When upgrading to Xcode 15.3 and a newer version of Sentry, the dmg
template file was too small for the newly generated executable.
@mcdurdin mcdurdin changed the title chore(ios): support build on Apple Silicon using Xcode 15.3 chore(ios,mac): support build on Apple Silicon using Xcode 15.3 Apr 26, 2024
@darcywong00 darcywong00 added this to the A18S1 milestone Apr 28, 2024
@mcdurdin
Copy link
Member

That said, the minimum version increase, this late in the game... makes me wonder if we should do a first release before the update lands, then follow up with this one.

Then again, if anything feels broken with the release... we probably don't want to leave users with 10.10 - 10.12 out to dry. So we probably shouldn't use it as a reason, even if it's unfortunate to shift minimum-requirements despite the prior announcement.

Yes, better to change minimum version in beta than in 17.0-stable cycle. In any case, we don't have a choice here because we were not ready to release before Apple's deadline.

@mcdurdin
Copy link
Member

mcdurdin commented Apr 28, 2024

iOS Test build failure:

https://build.palaso.org/buildConfiguration/Keyman_iOS_TestPullRequests/458637?buildTab=log&linesState=901193&logView=flowAware&focusLine=901224

WARN [2024-04-28 15:27:23.17]:  [33m[altool] 2024-04-28 15:27:23.172 *** Error: The provided entity includes an attribute with a value that has already been used The bundle version must be higher than the previously uploaded version: ‘200’. (ID: 2cbb1a59-d121-4cd4-acb4-6190963629fc) (-19232)

iOS Sample and Test Projects build failure:

https://build.palaso.org/buildConfiguration/Keyman_iOS_TestSamplesAndTestProjects/458641?buildTab=log&linesState=84&logView=flowAware&focusLine=909

/Users/marc_durdin/buildAgent/work/99b311828f4ee7c/keyman/ios/samples/KMSample1/build/KMSample1.build/Debug-iphoneos/KMSample1.build/DerivedSources/GeneratedAssetSymbols.swift:84:10: error: Type Name Violation: Type name '_ACResourceInitProtocol' should only contain alphanumeric and other allowed characters (type_name)

@mcdurdin
Copy link
Member

Note: iOS build is running afoul of:

altool is returning the same error 149,727 times ... hmm.

It may be that this is causing the next step to fail.

Temporary mitigation: adding a hosts entry for northamerica-1.object-storage.apple.com

@darcywong00
Copy link
Contributor

@mcdurdin
Copy link
Member

iOS build failure (upload to TestFlight for FVKeyboards) seems to be versioning-related and exposed by changes in XCode 15.0.

fastlane/fastlane#20743

https://build.palaso.org/buildConfiguration/Keyman_iOS_TestPullRequests/458644?buildTab=log&linesState=2747.2799&logView=flowAware&focusLine=2830

WARN [2024-04-28 16:42:28.14]:  [33m[altool] 2024-04-28 16:42:28.139 *** Error: The provided entity includes an attribute with a value that has already been used The bundle version must be higher than the previously uploaded version: ‘200’. (ID: d9ee2dd3-47d4-4c25-9e01-9c538b265c9e) (-19232)

@jahorton
Copy link
Contributor

jahorton commented Apr 29, 2024

06:42:28   WARN [2024-04-28 16:42:28.14]: [altool]     NSLocalizedDescription = "The provided entity includes an attribute with a value that has already been used";
06:42:28   
06:42:28   WARN [2024-04-28 16:42:28.14]: [altool]     NSLocalizedFailureReason = "The bundle version must be higher than the previously uploaded version: \U2018200\U2019. (ID: d9ee2dd3-47d4-4c25-9e01-9c538b265c9e)";
06:42:28   
06:42:28   WARN [2024-04-28 16:42:28.14]: [altool]     NSUnderlyingError = "Error Domain=IrisAPI Code=-19241 \"The provided entity includes an attribute with a value that has already been used\" UserInfo={status=409, detail=The bundle version must be higher than the previously uploaded version., source={\n    pointer = \"/data/attributes/cfBundleVersion\";\n}, id=d9ee2dd3-47d4-4c25-9e01-9c538b265c9e, code=ENTITY_ERROR.ATTRIBUTE.INVALID.DUPLICATE, title=The provided entity includes an attribute with a value that has already been used, meta={\n    previousBundleVersion = 200;\n}, NSLocalizedDescription=The provided entity includes an attribute with a value that has already been used, NSLocalizedFailureReason=The bundle version must be higher than the previously uploaded version.}";

\U2018200\U2019 is mentioned as version in one part of the message, but that doesn't look like a version string to me. Then again... \u2018 is... the left-side single-quote. So, basically, it is just reading '200' from somewhere, but with curly quotes.

Wait a sec... it failed for the FV app, but not ours.

image

<key>CFBundleVersion</key>
<string>200</string>

I think the block linked above is not being properly updated, for whatever reason.

... and I don't see any code changes / "Files Changed" within the oem/fv section of our repo... yep, that'd do it.

Wrap scripts for FirstVoices so that environment variables are retained
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM. A bunch of tidyup work that we should do in 18.0 cycle. I've marked the comments as A18S1 to indicate that we should do them while this is still fresh and early in the dev cycle, but A18S2 also fits well.

@@ -950,7 +950,7 @@
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = "/usr/bin/env bash";
shellScript = "verstr1=$(/usr/libexec/PlistBuddy -c \"Print CFBundleShortVersionString\" \"$SRCROOT/Keyman-Info.plist\")\nverstr2=$(/usr/libexec/PlistBuddy -c \"Print CFBundleVersion\" \"$SRCROOT/Keyman-Info.plist\")\n/usr/libexec/PlistBuddy \"$SRCROOT/Settings.bundle/Root.plist\" -c \"set PreferenceSpecifiers:0:DefaultValue $verstr1 (build $verstr2)\"\n\nif which swiftlint >/dev/null; then\n swiftlint --config ../../.swiftlint.yml\nelse\n echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\"\nfi\n";
shellScript = "#verstr1=$(/usr/libexec/PlistBuddy -c \"Print CFBundleShortVersionString\" \"$SRCROOT/Keyman-Info.plist\")\n#verstr2=$(/usr/libexec/PlistBuddy -c \"Print CFBundleVersion\" \"$SRCROOT/Keyman-Info.plist\")\n#/usr/libexec/PlistBuddy \"$SRCROOT/Settings.bundle/Root.plist\" -c \"set PreferenceSpecifiers:0:DefaultValue $verstr1 (build $verstr2)\"\n\nif which swiftlint >/dev/null; then\n swiftlint --config ../../.swiftlint.yml\nelse\n echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\"\nfi\n";
Copy link
Member

Choose a reason for hiding this comment

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

This call to swiftlint should be cleaned up into a wrapped script as well in 18.0 (A18S1), and commented lines should be removed.

@@ -970,7 +970,7 @@
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = "/usr/bin/env bash";
shellScript = ". \"$KEYMAN_ROOT/resources/build/xcode-utils.sh\"\n\nif [ ${UPLOAD_SENTRY:-false} = true ]; then\n # Calls resource script to perform the dSYM upload\n phaseSentryDsymUpload \"keyman-ios\"\nfi\n";
shellScript = ". \"$KEYMAN_ROOT/resources/build/xcode-utils.sh\"\n\nif [ ${UPLOAD_SENTRY:-false} = true ]; then\n # Calls resource script to perform the dSYM upload\n \"$KEYMAN_ROOT/resources/build/xcode-wrap.sh\" \"$KEYMAN_ROOT/resources/build/sentry-dsym-upload.sh\"\nfi\n";
Copy link
Member

Choose a reason for hiding this comment

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

The if test should be inside the sentry-dsym-upload.sh script so that the shellScript is a pure wrapped call to the script (A18S1).

mac/Keyman4MacIM/Keyman-template.dmg Show resolved Hide resolved
Comment on lines +475 to +476
shellPath = "/usr/bin/env bash";
shellScript = "#verstr1=$(/usr/libexec/PlistBuddy -c \"Print CFBundleShortVersionString\" \"$SRCROOT/FirstVoices/Info.plist\")\n#verstr2=$(/usr/libexec/PlistBuddy -c \"Print CFBundleVersion\" \"$SRCROOT/FirstVoices/Info.plist\")\n#/usr/libexec/PlistBuddy \"$SRCROOT/Settings.bundle/Root.plist\" -c \"set PreferenceSpecifiers:0:DefaultValue $verstr1 (build $verstr2)\"\n";
Copy link
Member

Choose a reason for hiding this comment

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

This script should be removed in 18.0 cycle (A18S1).

Copy link
Member

Choose a reason for hiding this comment

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

All these scripts should be in resources/build/mac in 18.0 (A18S1)

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to combine all the set-bundle-versions-... scripts and parameterise, given they are so similar. A18S1

Copy link
Member

Choose a reason for hiding this comment

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

Let's see if we can move this to resources/build/mac as well A18S1

Copy link
Member

Choose a reason for hiding this comment

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

Let's see if we can move this to resources/build/mac as well A18S1

@mcdurdin mcdurdin merged commit f41d1ad into beta May 2, 2024
25 checks passed
@mcdurdin mcdurdin deleted the chore/ios/10671-xcode-15-build branch May 2, 2024 04:31
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.318-beta

srl295 added a commit that referenced this pull request Aug 19, 2024
- add as option to compiler
- Match version in #11302

Fixes: #11423
mcdurdin pushed a commit that referenced this pull request Aug 21, 2024
- add as option to compiler
- Match version in #11302

Fixes: #11423
mcdurdin pushed a commit that referenced this pull request Aug 22, 2024
- add as option to compiler
- Match version in #11302

Fixes: #11423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore(ios): upgrade iOS SDK to 17 - requires Xcode 15 - by April 29, 2024
5 participants