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

feat(ios): allow keyman engine to be built via Carthage for iOS #11834

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

davidmoore1
Copy link
Contributor

@davidmoore1 davidmoore1 commented Jun 19, 2024

This pull request makes changes to allow a project (such as Scripture App Builder) to include the keyman engine through carthage for ios. I pretty much left the standard builds alone to the extent possible, leaving all the project files and the Carthage that is currently there in place and unchanged. The following changes have been made:

Cartfile and Cartfile.resolved have been added to the top level directory, which is where carthage requires them for this purpose. The original Cartfile and Cartfile.resolved in the iOS folder are still there and used for the standard builds.

A workspace for KeymanEngine has been added to the top level for Carthage to detect and run from. It points to a single project, the KeymanEngineCarthage.xcodeproj file, which is specific to this build.

ios/engine/KMEI/KeymanEngineCarthage.xcodeproj. This project is identical to the KeymanEngine.xcodeproj with the following exceptions:

  • All targets except for KeymanEngine are turned off and marked non shareable. Carthage will build any shared target with the associated build.
  • Frameworks have been modified to look to the Carthage/Build folder at the main directory instead of the one in the ios folder.
  • A script phase has been added to call the build.sh build:engine in ios with a parameter of --carthage-build to update the bundle prior to doing the actual build.

ios/build.sh and ios/engine/build.sh have been modified to accept a new parameter --carthage-build. This parameter is used to prevent the build scripts from calling carthage and to run the update_bundle and then quit.

.gitignore is modified to allow the single xcscheme which Carthage requires for the build to be checked in,

A new script file ios/testCarthageBuild.sh is included to allow for testing of the build. It builds all the carthage components and then runs the keyman engine build from within Carthage. A keymanEngine.xcframework is built in the Carthage/Build folder.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Jun 19, 2024
@keymanapp-test-bot
Copy link

User Test Results

Test specification and instructions

ERROR: user tests have not yet been defined

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S4 milestone Jun 19, 2024
@keyman-server
Copy link
Collaborator

This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

@jahorton jahorton changed the title Allow keyman engine to be built via Carthage for iOS feat(ios): allow keyman engine to be built via Carthage for iOS Jun 20, 2024
@jahorton
Copy link
Contributor

Cartfile and Cartfile.resolved have been added to the top level directory, which is where carthage requires them for this purpose. The original Cartfile and Cartfile.resolved in the iOS folder are still there and used for the standard builds.

Is there any way to have two versions in parallel? Keep in mind that we also have Keyman Engine for Mac, and somewhere down the line someone may wish for that to be distributed as well. The best I can find in this regard would be to make the workspace capable of producing both engines as schemes, then leaving it to the framework consumer to pick which one they want to use in the project's configuration. We can't filter it for them, and they can't filter it when running carthage bootstrap.

Furthermore... it certainly seems that Carthage has a quite opinionated set of requirements for compatibility. I went searching and found numerous issues + feature requests for them to implement some sort of filter on specified dependency repos, and they pretty much rejected all of 'em. No ability to say "use this subpath"; one (older) issue I could find considered monorepo approaches to be "niche" and "one-off".

The best link I could find re: the root-level requirement: Carthage/Carthage#3042


Cartfile and Cartfile.resolved have been added to the top level directory, which is where carthage requires them for this purpose. The original Cartfile and Cartfile.resolved in the iOS folder are still there and used for the standard builds.

A workspace for KeymanEngine has been added to the top level for Carthage to detect and run from. It points to a single project, the KeymanEngineCarthage.xcodeproj file, which is specific to this build.

There's a decent chance that this will be a "hard sell" to our project lead; we've done a lot of work to set up individual silos for each platform and section, and this kind of muddies those waters. I'll leave it to him to make that decision, though.

Something that I bumped into while researching these details: recent versions of Xcode offer a built-in Swift Package Manager that may be worth shifting to. It seemed capable of at least detecting the iOS engine's project when given this repo's URL, though I admittedly doubt it'll work out of the box due to the Carthage dependencies. I personally think it looks like a promising alternative to Carthage, etc, worth looking at, but for now, that's just my perspective. (The Mac project doesn't show up... but that's probably because it doesn't use Swift at this time.)

A script phase has been added to call the build.sh build:engine in ios with a parameter of --carthage-build to update the bundle prior to doing the actual build.

That may provide us a decent avenue to have the repo run the Carthage setup if necessary as an alternative via whatever (other) dependency system(s) we do land with. It'd take investigative work to get right, of course.

Also, note that our primary team members for the platform are currently out of office, so there may be delays in getting a full response to this PR. Not ideal, I know, but I figure it's best to know about the potential for delays regarding potential acceptance of your submission.

@davidmoore1
Copy link
Contributor Author

davidmoore1 commented Jun 20, 2024

For the one issue about the running mac, it is controlled somewhat there by the --platform ios. That makes it look for all schemes that are ios and could have schemes added for mac that would work and then only build in Carthage when the platform is set to build for mac.

I agree that the requirements that Carthage makes are overly specific and that it doesn't allow you a lot of options. Part of the reason for not trying to modify the existing projects to use the other Carthage was to try and leave everything you're already doing alone as much as possible. If you can figure out a way to make this a package dependency through xcode, I would be more than happy to switch. The problem I have been working against is that there is not a good way with the standard repository here to include the engine as part of an SAB build. Originally, I would build it manually and check in the framework, but that required me to rebuild it every time xcode was updated and you really don't like checking in binaries anyway. Adding the carthage stuff made it easy to include the framework into the project and have it built along with the other carthage components. But if you can come up with another solution that would work, I will be happy to try to use it. And if you can't and can't merge the PR, I'll just keep creating it in my own fork when I upgrade as I have been doing.

Thank you for the update.

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.

One extra thing I'd be worried about here - how well does the project build if the user does not have node/npm installed on their machine? There's the potential for confusing side-effects if people aren't aware of our toolchain requirements, since some of those don't come for free with macOS and Carthage.

The embedded Keyman Engine for Web component (see: web/app/webview) requires TypeScript / tsc as run on Node in order to build properly. We don't check those build products into the repository, nor do we check our node_modules folder in. This component would either have to be built locally (thus, needing the node-based dependencies) or pre-prepared in some manner for the Carthage distribution.

For a broad distribution via Carthage, it's probably "bad form" to require other dependency managers, etc that such users might not choose to have installed.

@mcdurdin
Copy link
Member

I'll be back on deck for a few days next week (and then mostly offline again after that for a few more weeks) but can hopefully review properly then.

In the meantime, I wonder what you think about us providing a binary instead as part of our build process -- Carthage does provide some very basic support for binary frameworks -- because that avoids a complex toolchain requirement including Node, Emscripten, brew coreutils, etc, etc.

I imagine that there are problems with binaries as well though (e.g. Swift version mismatches).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat ios/ user-test-missing User tests have not yet been defined for the PR
Development

Successfully merging this pull request may close these issues.

None yet

5 participants