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

Add Swift Package Manager support #10

Merged
merged 1 commit into from Jul 12, 2020

Conversation

sebj
Copy link
Contributor

@sebj sebj commented Jun 15, 2020

(WIP) Added Swift Package Manager support, to resolve #9

As SPM targets can't contain 'mixed' Objective-C and Swift code, I've separated code into two source folders (ObjCSources and SwiftSources), with the main Swift library target depending on the ObjC target. This only requires the ObjC folder be excluded from the Swift target, and vice-versa for the ObjC target.

The other approach would have been to keep the existing file/folder structure, but define each and every file individually as excluded inside the Package.swift file.

Remaining issues:

  • Localised assets aren't included in the Swift Package (blocked until Swift 5.3 availability)
  • When using the Swift Package, 'KeyboardKitObjC' is being exposed as importable alongside 'KeyboardKit'

@douglashill
Copy link
Owner

douglashill commented Jun 21, 2020

Thank you. I'll try to look over this before Swift 5.3 is out.

@sebj
Copy link
Contributor Author

sebj commented Jun 29, 2020

Just upped the minimum Swift tools version to 5.3 and added support for the localised string assets – tested out in a SPM project, seemed to work well!

Created a symbolic link to the KeyboardKit/Localised folder at KeyboardKit/SwiftSources/Localised, as the KeyboardKit target in Package.swift seems to prefer resource paths that are relative to the path that's specified for sources (KeyboardKit/SwiftSources).

Copy link
Owner

@douglashill douglashill left a comment

Thank you for getting this together. I tested it and everything seems to work.

If possible, I would prefer if we could avoid moving so much stuff around. I think it should work to leave the Swift files alone and just move the Objective-C files into a subdirectory. (I experimented in https://github.com/douglashill/KeyboardKit/tree/swiftpm and I think that approach should work although I didn’t get it all building yet.)

Did you find a way to test SwiftPM locally, or does it seem to be necessary to push to GitHub after each change?

@@ -594,18 +615,18 @@
inputFileListPaths = (
);
inputPaths = (
"$(SRCROOT)/KeyboardKit/Localised/en.lproj/Localizable.strings",
"$(SRCROOT)/KeyboardKit/UpdateLocalisedStringKeys.swift",
"$(SRCROOT)/KeyboardKit/Localised/en.lproj/KeyboardKit.strings",
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

Good spot! This is broken on the main branch.

96A241CA2495417600C77B57 /* ScrollViewKeyHandler.swift in Sources */,
96A241C22495417600C77B57 /* SelectableCollectionKeyHandler.swift in Sources */,
96A241C12495417600C77B57 /* KeyboardTabBarController.swift in Sources */,
96A241BD2495417600C77B57 /* ResponderChainDebugging.m in Sources */,
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

It looks like a lot of files have been removed and re-added to the project. Is it possible to move these and retain the existing references in the project. I have some private WIP branches that will likely have conflicts otherwise.

The order of the files has changed. The files are roughly ordered by importance and scope (more general classes like KeyboardApplication near the bottom) so it would be nice to preserve that.

I’m not particularly keen on moving so much stuff around (both on disk and in the Xcode project) because I feel it makes the hierarchy unnecessarily deep for a framework of this size. Could we instead leave all the Swift files where they are and only move the Objective-C files into a subdirectory (called ObjC or something). I would create have this subdirectory on disk, not as a group in the Xcode project. The Swift SwiftPM target could then exclude that directory while the Objective-C SwiftPM target could have its path set to this subdirectory. Do you think this would work?

@@ -0,0 +1,2 @@
#import "BarButtonItem.h"
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

These contortions to work with SwiftPM are unfortunate, but I haven’t been able to work out a better way. 😞

For this file, I think it would be a bit nicer to use

#if SWIFT_PACKAGE
#import "BarButtonItem.h"
#else
#import <KeyboardKit/BarButtonItem.h>
#endif

in the existing umbrella header rather than added a second alternative umbrella header. That way it matches the pattern used in BarButtonItem.m to deal with this problem.

Copy link
Contributor Author

@sebj sebj Jul 6, 2020

Choose a reason for hiding this comment

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

You’re completely right – I forked off your swiftpm branch locally and seem to have gotten it working with the single existing umbrella header (left a full comment further below).

@@ -0,0 +1 @@
../Localised
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

If the Swift source files are changed to stay in their previous location (KeyboardKit rather than KeyboardKit/SwiftSources) then I think this can be removed?

Copy link
Contributor Author

@sebj sebj Jul 6, 2020

Choose a reason for hiding this comment

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

Yep!

@@ -2,8 +2,12 @@

import Foundation

#if SWIFT_PACKAGE
private let keyboardKitBundle = Bundle.module
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

Tangent:

Out of interest I had a look what Bundle.module actually does, and it’s like this:

import class Foundation.Bundle

private class BundleFinder {}

extension Foundation.Bundle {
    /// Returns the resource bundle associated with the current Swift module.
    static var module: Bundle = {
        let bundleName = "KeyboardKit_KeyboardKit"

        let candidates = [
            // Bundle should be present here when the package is linked into an App.
            Bundle.main.resourceURL,

            // Bundle should be present here when the package is linked into a framework.
            Bundle(for: BundleFinder.self).resourceURL,

            // For command-line tools.
            Bundle.main.bundleURL,
        ]

        for candidate in candidates {
            let bundlePath = candidate?.appendingPathComponent(bundleName + ".bundle")
            if let bundle = bundlePath.flatMap(Bundle.init(url:)) {
                return bundle
            }
        }
        fatalError("unable to find bundle named KeyboardKit_KeyboardKit")
    }()
}

So it looks like SwiftPM doesn’t make a framework for KeyboardKit. Instead the KeyboardKit classes are directly in the app bundle and there is a separate (code-free) bundle for the resources.

I might rename rename Marker to BundleFinder to be consistent with how SwiftPM implemented similar functionality.

Package.swift Outdated
name: "KeyboardKit",
defaultLocalization: "en",
platforms: [
.iOS(.v13)
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

Is there any reason not to set this back to iOS 11 to be consistent with the the Xcode project and CocoaPods?

(Although I’ll probably bump this from 11 to 12 very soon.)

Copy link
Contributor Author

@sebj sebj Jul 6, 2020

Choose a reason for hiding this comment

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

No reason, this should indeed be iOS 11

Package.swift Outdated
name: "KeyboardKitObjC",
path: "KeyboardKit/ObjCSources",
exclude: ["ResponderChainDebugging.m"],
publicHeadersPath: ""),
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

Is there a reason for this empty string?

Suggested change
publicHeadersPath: ""),
),

Copy link
Contributor Author

@sebj sebj Jul 6, 2020

Choose a reason for hiding this comment

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

Without explicitly setting this path (i.e. it being the default value of nil), SwiftPM uses the default umbrella directory component of include and appends this value to the target path (KeyboardKit/ObjSources), giving KeyboardKit/ObjCSources/include as the public headers path.

Setting it to an empty string ensures KeyboardKit/ObjCSources is used as the public headers directory.

Package.swift Outdated
path: "KeyboardKit/SwiftSources",
exclude: ["UpdateLocalisedStringKeys.swift"],
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

What I asked about above is basically like this I think:

Suggested change
path: "KeyboardKit/SwiftSources",
exclude: ["UpdateLocalisedStringKeys.swift"],
path: "KeyboardKit",
exclude: ["ObjC", "UpdateLocalisedStringKeys.swift"],

Package.swift Outdated
dependencies: ["KeyboardKitObjC"],
path: "KeyboardKit/SwiftSources",
exclude: ["UpdateLocalisedStringKeys.swift"],
resources: [.process("Localised")]),
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

I think this can be omitted because SwiftPM infers that .lproj directories are localised resources.

Copy link
Contributor Author

@sebj sebj Jul 6, 2020

Choose a reason for hiding this comment

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

You’re totally right, thanks!

README.md Outdated
@@ -92,6 +92,13 @@ Both Swift and Objective-C apps are supported. Since KeyboardKit uses Swift, it

CocoaPods may not be as well tested as the recommended steps above. Please [open a pull request](https://github.com/douglashill/KeyboardKit/pulls) if you notice any problems.

### Swift Package Manager
Copy link
Owner

@douglashill douglashill Jul 5, 2020

Choose a reason for hiding this comment

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

This section can go just above CocoaPods if you like.

@sebj
Copy link
Contributor Author

sebj commented Jul 6, 2020

@douglashill Thanks for the feedback! Makes a lot of sense. Completely agree with your assessment that it’d be better not to move so many files around.

I was able to test the Swift Package in a local project by dragging the local repo folder into Xcode’s navigator with a project open, at which point it’s recognised as a package and the library just has to be added to the app target.

Seems there’s only a minimal set of changes on top of your swiftpm branch to get this working for both SwiftPM & CocoaPods, so I’ve changed this PR to point to that branch. Feel free to close if you’ve got this sorted already, just happy to get SwiftPM support 😄

@sebj sebj changed the base branch from main to swiftpm Jul 6, 2020
Copy link
Owner

@douglashill douglashill left a comment

Looks good. Thank you @sebj.

I didn’t want to undercut what you did before but I haven’t worked with SwiftPM before so needed to dig in a bit to learn it. Your original branch was really helpful for me to understand how it all works.

Sorry I’ve been a bit slow getting to this. Lots of stuff going on after WWDC!

@douglashill douglashill merged commit 4f90b42 into douglashill:swiftpm Jul 12, 2020
@sebj
Copy link
Contributor Author

sebj commented Jul 12, 2020

@douglashill All good, totally understand! Thanks for merging! :)

@sebj sebj deleted the swift-package branch Jul 12, 2020
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.

Swift Package Manager support
2 participants