Skip to content

Remove requirement for macOS 10.13 #152

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

Closed
helje5 opened this issue Jul 31, 2020 · 11 comments · Fixed by #156
Closed

Remove requirement for macOS 10.13 #152

helje5 opened this issue Jul 31, 2020 · 11 comments · Fixed by #156
Assignees
Labels
kind/enhancement Improvements to existing feature.
Milestone

Comments

@helje5
Copy link

helje5 commented Jul 31, 2020

The runtime requires macOS 10.13, which is quite inconvenient because all consuming packages now also need to explicitly require 10.13 in the package manifest. It is especially annoying if one tries to conditionally implement Lambda support in a 10.10 package via #canImport, e.g.: https://github.com/Macro-swift/Macro/blob/feature/lambda-1/Sources/http/Lambda/lambda.swift#L9

Slack says this requirement only exists for the ISO date formatter, not sure whether you use it for rendering only or also for parsing. In any case, it should be quite easy to replace w/ a custom parser/renderer. Should be reasonable because the format is fixed and never changes.

It could be built on top of timegm and strptime, though it needs an extra processing step to capture the milliseconds:

import func   Foundation.strptime
import func   Foundation.timegm
import struct Foundation.tm

let s          = "20180905T140903.591680Z"
var parsedTime = tm()

s.withCString { cstr in
  strptime(cstr, "%Y%m%dT%H%M%S.%fZ", &parsedTime) // this needs to extract the ms
}
let time = timegm(&parsedTime)
let ti   = TimeInterval(time) // + milliseconds

You could also provide this, and still use the ISO formatter if available (via if #available(macOS 10.13, ...)), which is what I do in Macro.

@fabianfett
Copy link
Member

fabianfett commented Jul 31, 2020

I'm a +1 on this and think that we should address it. @tomerd wdyt?

@tomerd
Copy link
Contributor

tomerd commented Jul 31, 2020

+1 conceptually - I dont like the 10.13 constraint either.

do we know if this can be done in all formats coming form AWS? I think foundation used ICU for some timezone localization aspects which may be more complicated than the one demonstrated here, but it may not be used here

@tomerd tomerd added discussion kind/enhancement Improvements to existing feature. labels Jul 31, 2020
@helje5
Copy link
Author

helje5 commented Jul 31, 2020

? It’s ISO, there are only really two variants, with ms and without

@helje5
Copy link
Author

helje5 commented Jul 31, 2020

Oh, timezones, this format doesn’t support timezones (anything locale bound requiring tzdata) , max gmt offsets, though it probably is Z always

@tomerd
Copy link
Contributor

tomerd commented Jul 31, 2020

right, but IIRC there are other date formats we deal with in the library (unfortunately different AWS events have different date formats) so trying to understand if we can get away with this in all cases

@helje5
Copy link
Author

helje5 commented Jul 31, 2020

neither dateformatter nor locale require 10.13, it is just the specific isodateformatter, and iso is fixed

@tomerd
Copy link
Contributor

tomerd commented Jul 31, 2020

works for me!

@fabianfett
Copy link
Member

@tomerd We can go full strptime here or we just create a DateFormatter that has the iso as the dateFormat string like here:

    let formatter = DateFormatter()
    formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ss.SSSZ"
    formatter.timeZone   = TimeZone(secondsFromGMT: 0)
    formatter.locale     = Locale(identifier: "en_US_POSIX")
    return formatter

https://github.com/fabianfett/swift-lambda-runtime/blob/c306ac8b32a79a42ef8ada855a79930f3d368a2a/Sources/LambdaEvents/SNS.swift#L94

@fabianfett fabianfett assigned fabianfett and tomerd and unassigned fabianfett Aug 3, 2020
@helje5
Copy link
Author

helje5 commented Aug 3, 2020

date formatters are really really slow, i kicked them out of shrugs because they have been taking like 40% of the startup time. they are locale aware which is zarro necessary here

@helje5
Copy link
Author

helje5 commented Aug 3, 2020

i think strptime is fine, but the ms need to be handled separately

@tomerd
Copy link
Contributor

tomerd commented Aug 5, 2020

ugh, looks like strptime not avail on through Glibc. I guess I could make this Darwin only since we will use the formatters in linux anyways

@tomerd tomerd linked a pull request Aug 5, 2020 that will close this issue
@tomerd tomerd added this to the 0.3.0 milestone Aug 6, 2020
helje5 added a commit to Macro-swift/MacroLambda that referenced this issue Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants