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

[Request] Swift 4.2 #20

Closed
freeubi opened this issue Jun 16, 2018 · 36 comments
Closed

[Request] Swift 4.2 #20

freeubi opened this issue Jun 16, 2018 · 36 comments
Assignees

Comments

@freeubi
Copy link

freeubi commented Jun 16, 2018

Hello,

can you please make a cocoapod version that can imported in xCode10 and swift 4.2?

Thanks :)

@poulpix
Copy link

poulpix commented Jun 17, 2018

Hi,
+1
LocoKit currently can't be used in iOS 12 Beta projects because of the LocoKitCore framework compiled in Swift 4.1.
Thanks!

@sobri909
Copy link
Owner

Okay, I'll get this sorted within the next couple of hours. Stay tuned...

I keep forgetting about this with each new Swift version, because I'm always building LocoKitCore from source, so I don't notice the binary incompatibility. Swift 5.0's ABI stability can't come soon enough!

@sobri909
Copy link
Owner

Okay, I've uploaded a LocoKitCore built with Swift 4.2 to the develop branch. So you'll need to modify your Podfile to pull from that branch.

pod 'LocoKit', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'develop'
pod 'LocoKit/LocalStore', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'develop'
pod 'LocoKitCore', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'develop'

You'll also need to migrate your code to LocoKit 6.0 API changes. They're fairly minor, and some of them Xcode will provide fixits for anyway.

The main difference is that TimelineManager has been split into two separate classes, and you should now interact with TimelineRecorder instead.

let store = PersistentTimelineStore()
var recorder: TimelineRecorder

func startup() {
    recorder = TimelineRecorder(store: store)
    recorder.startRecording()
}

Or if you're using activity type classifiers:

func startup() {
    LocoKitService.apiKey = "<insert your API key here>"
    recorder = TimelineRecorder(store: store, classifier: TimelineClassifier.highlander)
    recorder.startRecording()
}

If you're not using TimelineManagers at all, then your code probably won't have to change, except for the renames which Xcode will provide fixits for.

@sobri909
Copy link
Owner

Oh, I forgot to mention that the LocoKitCore binary doesn't include sim archs. Xcode 10 didn't want to do it, for some reason, and I haven't had time to debug it yet.

It's generally a bad idea to be doing any dev work in the sim anyway, unless absolutely necessary.

I also haven't spent much/any time testing the non persistent timeline store in v6.0.0 yet. So if you've been using TimelineManager instead of PersistentTimelineManager, then there may or may not be some surprises.

@freeubi
Copy link
Author

freeubi commented Jun 18, 2018

Thanks, I will try out this today.
I'm not using any of the fancy features, just the LocoKitCore.

@poulpix
Copy link

poulpix commented Jun 18, 2018

Thanks!

It looks like there are still a few things that are not ready for Swift 4.2, such as UIBackgroundTaskInvalid which must be replaced by UIBackgroundTask.invalid in several places.

It also looks like there is now a "hard" dependency between LocoKit and LocoKit/LocalStore. Previously I was able to just reference the first one in my Podfile, but now I'm forced to reference all three pods (including LocoKitCore) because of a reference to PersistentSample in TimelineProcessor.swift. Is this intended? It's also not possible anymore to just create a TimelineManager without local, in-memory storage, since TimelineStore's default initializer cannot be called from outside the SDK. This was possible with the previous version though.

Thank you anyway.

@sobri909
Copy link
Owner

sobri909 commented Jun 18, 2018

such as UIBackgroundTaskInvalid which must be replaced by UIBackgroundTask.invalid

Ah, for those ones you can probably get around them by manually changing the Swift version for the LocoKit pod target in Xcode to Swift 4 instead of Swift 4.2.

Edit: Also, I should add this to the podspec, given that that's possible to do now.

now I'm forced to reference all three pods (including LocoKitCore) because of a reference to PersistentSample in TimelineProcessor.swift

Oops! Ok, I'll get that fixed shortly. Looks like insertDataGapBetween() is using the wrong class type for its samples.

It's also not possible anymore to just create a TimelineManager without local, in-memory storage, since TimelineStore's default initializer cannot be called from outside the SDK.

In LocoKit 6.0 TimelineManager doesn't exist. The replacement is TimelineRecorder. Is that what you meant?

@poulpix
Copy link

poulpix commented Jun 18, 2018

Yes, I meant TimelineRecorder, which requires a store at initialization. In previous LocoKit versions you could create a TimelineManager without any storage (in-memory), but now it seems you can't anymore since the store parameter is required and the only constructible class suitable for this parameter is the PersistentTimelineStore (which inherits from TimelineStore, which itself isn't initializable from outside the SDK due to no default initializer, so considered internal by Swift and thus not accessible). But that may be intended, I don't know.

As for the UIBackgroundTask.invalid, this is a detail :) I didn't change the Swift language version, instead I've replaced locally those occurrences directly in your code. I know I'll have to do that for every pod update but that's not too painful.

@sobri909
Copy link
Owner

In previous LocoKit versions you could create a TimelineManager without any storage (in-memory)

If you're not using the persistent store / timelines, then the combo you want is TimelineRecorder and TimelineStore. So the setup should look something like this:

let store = TimelineStore()
var recorder: TimelineRecorder

func startup() {
    recorder = TimelineRecorder(store: store)
    recorder.startRecording()
}

Is that not working? It's not letting you create a TimelineStore instance? I'd assumed that if there's no explicit init method, then creating an instance would be publicly doable. But maybe I misunderstood Swift's intentions there.

As for the UIBackgroundTask.invalid, this is a detail :) I didn't change the Swift language version

I just checked, and the podspec was already specifying Swift 4.1, but using the old style workaround way of doing it. I guess that's not working properly anymore.

I'm about to push a change to the podspec which uses CocoaPods' new way of specifying the Swift version. Hopefully that will make Xcode happy.

@sobri909
Copy link
Owner

@poulpix Give it another try now. I've committed a bunch of changes that should fix all those problems.

What worries me more is that the non persistent store might not be retaining timeline items that it needs to retain, so they'll get released before they've finished being processed and finalised. That's the main remaining todo item on #11, which I haven't had a chance to get to yet.

Will be interesting to see how it performs. I suspect not well, unless the items are retained after creation by something external.

@poulpix
Copy link

poulpix commented Jun 18, 2018

Brilliant! Everything compiles fine now, I've been able to remove the explicit reference to LocoKit/LocalStore. The public init() does indeed solve the problem (without it by default Swift considers an empty initializer to be internal).

NB : I think the UIBackgroundTask.invalid change is related to Swift 4.2, not Swift 4.1, so that's the reason why you saw no problems when setting the Podspec to Swift 4.1.

As for how it performs, it needs some field testing now...

Thank you very much

@sobri909
Copy link
Owner

without it by default Swift considers an empty initializer to be internal

Yeah, I didn't expect that!

It doesn't seem intuitive to me. I'd prefer Swift to use the same visibility for the implicit init() method as the class itself, so if the class is public, the implicit init() is public. But I guess the Swift team have different ideas.

I think the UIBackgroundTask.invalid change is related to Swift 4.2, not Swift 4.1

Yeah, they've changed a bunch of enums in Swift 4.2. The old podspec was using a hack to get CocoPods to add the correct Swift version to the project, and that hack no longer works. But they've replaced it with a proper swift_version property in podspecs now, which does appear to work. Yay!

As for how it performs, it needs some field testing now...

I'd pay special attention to the results of the timeline item processing.

LocoKit 5.0 kept strong references to an array of activeItems that it considered incomplete, and still needing processing. LocoKit 6.0 doesn't keep such an array, and instead builds it dynamically at processing time, by walking the nextItem / previousItem links. So my guess is that without the persistent store, timeline items won't be processed properly, and will instead be released too early.

It's on my todo list to create a replacement for activeItems for non persistent stores. So I'll try to get to that in the next day or two. It should be simple enough to do.

@freeubi
Copy link
Author

freeubi commented Jun 19, 2018

I tried it too. I'm using the LocoCore and LocoKit pods, it's working well.

@andreastack
Copy link

I'm trying to compile the demo app under Xcode 10:
I cloned the project: git clone https://github.com/sobri909/LocoKit.git
I modified the Podfile according to post 4 to point to branches
-> I see a LocoKitCore file under path Pods/LocoKitCore/LocoKitCore.framework/ dated June 22
I still get an error message in file LocoKitCoreBridge.swift: /Users/atack/Desktop/LocoKit/Pods/LocoKit/LocoKit/Base/LocoKitCoreBridge.swift:6:8: Module compiled with Swift 4.1 cannot be imported in Swift 4.1.50: /Users/atack/Desktop/LocoKit/Pods/LocoKitCore/LocoKitCore.framework/Modules/LocoKitCore.swiftmodule/i386.swiftmodule

Can you point me to my mistake?

@freeubi
Copy link
Author

freeubi commented Jun 22, 2018

It only works on device, not in simulator.

@sobri909
Copy link
Owner

Yeah, the LocoKitCore binary in the develop branch doesn't include the sim archs, so it will only run on device.

In general I also strongly advice not doing any location based dev work in the sim.

The sim produces CLLocation data that doesn't match any real world conditions (eg the horizontalAccuracy is returned as an impossible 0). It also doesn't produce any CoreMotion data.

@freeubi
Copy link
Author

freeubi commented Jun 23, 2018

Thats true, but on the other hand its pretty hard to make routes on the device in front of a computer :D

@sobri909
Copy link
Owner

This leads me to a tangent that's been playing on my mind for a while.

I meant to file a new task last week for the maintenance of some "test databases", with the goal of being able to do test processing runs over a series of pre recorded samples.

So far I've found automated testing to be effectively impossible for the core "ActivityBrain" engine, due to the requirement of a steady flow of CoreLocation and CoreMotion data inputs, where the timing of the inputs is significant. If it's not receiving inputs with effectively real world timings and values, there's very little point in testing it at all. The tests simply wouldn't be useful.

But what can be tested is the results of the timeline processing engine(s) (TimelineProcessor et al). So what I plan to do is record one or more sessions, with several hours of travel + stationary data, then keep those as "test databases". That would allow processing to be repeatedly run over the same data, and tested for desirable results.

I'll file that now, so that I don't forget again...

@poulpix
Copy link

poulpix commented Jun 24, 2018

Hi,
This is a great idea, but you’d need plenty of various raw data for this to be useful.
What would be really cool would be to be able to record such data right from the sample app, and give an option to users to push it to a repo, along with a description of what kind of motion and itinerary the sample represents.

@sobri909
Copy link
Owner

sobri909 commented Jun 24, 2018

What would be really cool would be to be able to record such data right from the sample app

I love it! Great idea 😄 I'll add that to the todos for the task.

you’d need plenty of various raw data for this to be useful.

I'm lucky that the city I live in (Bangkok) has a full mix of all transport types, so in a single day I can travel by bicycle, bus, underground train, above ground train, boat, motorbike, and even airplane (if I've got somewhere to fly to!). So I often record days in Arc App that include four or more different activity types.

Although one thing that tends to make a lot of difference is variation in city density. How iOS deals with lack of GPS line of sight, lack of (or high availability of) wifi hotspots, etc, can have a big impact on how energy efficient location recording is and the accuracy of results (and sometimes in very counter intuitive ways). So while I can get a broad mix of building densities in my city, I can't exactly replicate the densities in European cities, or cities in the US, for example.

Having recordings of "problem trips" in various cities would probably help a lot for debugging various mystery problems with location accuracy.

@EugeneEl
Copy link

Hi, thank you for your great work with SDK.
Any chance master will be update for the latest Xcode 10?
Or I still need to install some previous Xcode version?

I've tried to use pods from develop branch, however I got an error while updating pods.

Analyzing dependencies
Pre-downloading: `LocoKit` from `https://github.com/sobri909/LocoKit.git`, branch `develop`
[!] Failed to load 'LocoKit' podspec: 
[!] Invalid `LocoKit.podspec` file: undefined method `swift_version=' for #<Pod::Specification name="LocoKit">.

 #  from /var/folders/6t/f3qp7hk55vd9hng1dzyjtpbm0000gn/T/d20180921-3200-949d2m/LocoKit.podspec:12
 #  -------------------------------------------
 #    s.frameworks   = 'CoreLocation', 'CoreMotion' 
 >    s.swift_version = '4.1'
 #    s.ios.deployment_target = '10.0'
 #  -------------------------------------------

@sobri909
Copy link
Owner

Hi @EugeneGoloboyar. The error you're seeing there looks to me to be a CocoaPods error, due to you using an older version of CocoaPods than the Podfile was built for.

There shouldn't be any problems (that I'm aware of) running the most recent stable release on Xcode 10.

@EugeneEl
Copy link

Thank you for fast reply!
Updating CocoaPods resolved my issue. Also I forgot to specify Swift version for pods.
(I usually specify Swift version in podfile for all pods via config.build_settings['SWIFT_VERSION']).

@freeubi
Copy link
Author

freeubi commented Oct 6, 2018

Unfortunatelly i have issues... i cant build LocokitCore, because its builded with swift 4.1.
Also, i have a strange dublications in the pod, see attached pic.
screenshot 2018-10-06 at 11 19 38

screenshot 2018-10-06 at 11 27 01

In my pod i'm using
pod 'LocoKit', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'develop' pod 'LocoKitCore', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'develop'

Final Xcode10, with Mojave.

@sobri909
Copy link
Owner

sobri909 commented Oct 6, 2018

Hi @freeubi! That's odd, with the duplication. I recommend deleting your Pods folder and doing a fresh pod install.

You will also need to switch to using the timelines branch, as that is where the Swift 4.2 work has been done. I've been meaning to merge it back into develop this week, but ran out of time. I'm also planning on merging it all back up to master next week and doing a new stable release, but again just haven't had the time to get to it yet!

The timelines branch can be considered stable and release quality. (It's the branch that the past few Arc App releases have been built from).

This is the Podfile snippet I'm using for the Demo App in the timelines branch:

pod 'LocoKit', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'timelines'
pod 'LocoKit/Timelines', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'timelines'
pod 'LocoKitCore', :git => 'https://github.com/sobri909/LocoKit.git', :branch => 'timelines'

@freeubi
Copy link
Author

freeubi commented Oct 6, 2018

Thanks, i could build with the timelines branch :)

@freeubi
Copy link
Author

freeubi commented Oct 16, 2018

@sobri909 Any update or ETA about the merge? I will need emulators soon :T

@sobri909
Copy link
Owner

@freeubi What tasks are you using the sim for?

LocoKit can't sensibly be used in the sim, because it don't produce correct CLLocation objects, and can't produce Core Motion data. So even the most basic inputs aren't available.

I'll eventually have another look at figuring out why the sim binaries aren't building. But I don't think lack of them should block any work, because all dev and testing work should be done on device.

@freeubi
Copy link
Author

freeubi commented Oct 16, 2018

@sobri909 Im developing a tamagotchi - fitness tracker combo, so it will be great to use sims for the tamagotchi part. Thats like 70% of my app :/
I dont use the gps feature in the simulator, but if it at least buildable then it will be more than enough.
[https://gettep.com]

@sobri909
Copy link
Owner

@freeubi Looks cool! That makes sense then.

In the meantime, it might be worth conditionally including the LocoKit imports by build configuration. For example I conditionally include AppSpector in my debug builds like this:

pod 'AppSpectorSDK', :configurations => ['Debug', 'MeRelease']

And then in code like this:

#if canImport(AppSpectorSDK)
import AppSpectorSDK
#endif
#if canImport(AppSpectorSDK)
...
AppSpector.run(with: config)
...
#endif

@sobri909
Copy link
Owner

I've added it to my todos for tomorrow anyway, to have another look into it. Presumably it's just some build script change that's needed, to make Xcode 10 happy. Hopefully I can get it back to normal without too much fuss.

@freeubi
Copy link
Author

freeubi commented Oct 16, 2018

@sobri909 Thanks!
Yes, the conditional import was my next plan but its easier if i dont really need to do anything :P
Also, feel to free contact me if you need any help :)

@sobri909
Copy link
Owner

@freeubi I think I've done it. f65b00e

Just doing a LocoKit Demo App fresh pod install and build now, to make sure it's all working proper.

Turns out it wasn't an Xcode 10 problem after all, it was because I used to build LocoKitCore from a workspace, but at some stage I ditched the workspace and used the project file directly, because LocoKitCore doesn't have any dependencies so the workspace was pointless. But that meant the Archive post action script was pointing to the wrong thing (which no longer existed).

In typical Xcode fashion, the error messages were completely unrelated to the actual problem, and sent me on a wild goose chase, until I finally spotted the mismatch in the xcodebuild call myself. Heh.

@sobri909
Copy link
Owner

Ok, confirmed it's working properly on device, and on sim. The new LocoKitCore binary is good to go.

Aside: I want to remove the required dependency on LocoKitCore eventually. Ideally it should only be necessary for use of LocoKitService things (eg modal fetches from the server, push wakeup call requests), and not be necessary if the app isn't using any LocoKitService.

But there's still a few small required chunks of LocoKit hidden away in LocoKitCore (Kalman filters and real time moving/stationary detection being the main two), which I haven't had time to open source yet. Hopefully soon!

@freeubi
Copy link
Author

freeubi commented Oct 17, 2018

https://gph.is/1nngRau

Thanks, i will check it out tonight :)
HUUUUGE thanks!

@freeubi
Copy link
Author

freeubi commented Oct 17, 2018

works like a charm!

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

No branches or pull requests

5 participants