Skip to content

remove requirment on macOS 10.13 #156

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

Merged
merged 3 commits into from
Aug 10, 2020
Merged

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Aug 5, 2020

motivation: simplify downstream libraries

changes: use custom ISO8601 decoders on macOS < 10.13

@tomerd
Copy link
Contributor Author

tomerd commented Aug 5, 2020

closes #152

@tomerd tomerd linked an issue Aug 5, 2020 that may be closed by this pull request
@tomerd
Copy link
Contributor Author

tomerd commented Aug 5, 2020

@helje5

@tomerd tomerd requested a review from fabianfett August 5, 2020 04:56
@tomerd tomerd added this to the 0.3.0 milestone Aug 5, 2020
@tomerd
Copy link
Contributor Author

tomerd commented Aug 5, 2020

note strptime is not available via Glibc so this is a bit uglier with #if os blocks

@tomerd tomerd added the 🔨 semver/patch No public API change. label Aug 5, 2020
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Hey @tomerd, I've thought about this a little more... Since strptime is not available through Glibc, we use this really only for macOS before 10.13. In the Swift.package we define the minimum Swift version though with 5.2. I doubt that there is any macOS version out there with 10.12 that runs Swift 5.2, not to speak of a current Xcode version that runs on macOS 10.12 or below, which is the only use case for this.

So maybe we should just preconditionFailure if we are below 10.13? wdyt? Sorry for coming up with this now, after you've already implemented it.

@tomerd
Copy link
Contributor Author

tomerd commented Aug 6, 2020

that sounds okay to me too. @helje5 wdyt?

@helje5
Copy link

helje5 commented Aug 6, 2020

Either is fine for me. There are some Mac users which tend to stick to older versions, but they are probably not Swift target audience in the first place ;-)

@helje5
Copy link

helje5 commented Aug 6, 2020

The really interesting question here is why spm decided to stick to 10.10 still, that feels a little weird, but the same reasoning should be applied i guess

@tomerd
Copy link
Contributor Author

tomerd commented Aug 8, 2020

@fabianfett what is your concern about taking this as-is? code bloat/complexity?

@fabianfett
Copy link
Member

@fabianfett what is your concern about taking this as-is? code bloat/complexity?

@tomerd I came to the conclusion, when I thought about, how we could test this... Date conversions should be tested by a unit test imho. When thinking about it, I wasn’t sure if there is actually a machine that could fulfill the requirements to run this branch. If there is no machine that the code could be ever run on, we shouldn’t add it because it increases complexity.

tomerd added 2 commits August 10, 2020 09:36
motivation: simplify downstream libraries

changes: use custom ISO8601 decoders on macOS < 10.13
@tomerd
Copy link
Contributor Author

tomerd commented Aug 10, 2020

@fabianfett addressed

Comment on lines 15 to 20
#if os(Linux)
import Glibc
#else
import Darwin.C
#endif

Copy link
Member

Choose a reason for hiding this comment

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

@tomerd leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, removed

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Thanks @tomerd LGTM

@tomerd tomerd merged commit f731664 into swift-server:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove requirement for macOS 10.13
3 participants