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

Swift 5 support #154

Merged
merged 5 commits into from Nov 29, 2018
Merged

Swift 5 support #154

merged 5 commits into from Nov 29, 2018

Conversation

djones6
Copy link
Contributor

@djones6 djones6 commented Nov 27, 2018

Description

Adds a build against a Swift 5 development snapshot to Travis, resolves Swift 5 compilation warnings, and drops support for Swift 3.

Compilation warnings

The compiler now emits a warning if a redundant access control modifier is applied to a member of an extension. This comes about if you declare an explicit access control modifier on the extension itself (ie. public extension Foo) and also on members (public var Bar).

There are two choices: keep the default access modifier on the extension (and remove public from all members of the extension), or remove the default access modifier and ensure all members that should be public are marked as such.

I've chosen the latter, as I think it makes it more clear as to what forms part of our public API: things that are public are still marked as such.

(note: my understanding is that removing the public from the extension only affects the default visibility of its members. Public members of the extension remain public. See: https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html#ID25)

Minimum swift support

Swift 5 drops support for the older Swift 3 package format, so packages that have a Package.swift and Package@swift-4.swift will fail to compile. This PR renames Package@swift-4.swift to Package.swift, bringing the minimum supported version to 4.0.

We could instead introduce a Package@swift-5.swift, but this increasingly adds complexity for the sake of supporting an obsolete version.

Dropping Swift 3 support should be released under a new SemVer minor, to enable anyone who still requires Swift 3 support to remain on the previous minor version.

How Has This Been Tested?

Checklist:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

Package.swift Outdated
targets: [Target(name: "Socket")],
exclude: ["BlueSocket.xcodeproj", "BlueSocket.xcworkspace", "README.md", "Sources/Info.plist", "Sources/Socket.h"]
products: [
// Products define the executables and libraries produced by a package, and make them visible to other packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete lines like this boilerplate please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed an update that removes the stock comments, updates the copyright header to be consistent with the style of other IBM-Swift repos, and also removed the redundant (I believe?) excludes from the Socket target.

Consistent copyright header

Remove redundant excludes
Copy link
Collaborator

@billabt billabt left a comment

Choose a reason for hiding this comment

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

Why did you removed the exclusions? When building with SPM, they're totally unneeded. Please restore.

@billabt
Copy link
Collaborator

billabt commented Nov 28, 2018

Also, I'd prefer not to merge this PR until a similar PR is presented for BlueSSLService. I'd like to keep them in lockstep. Thanks.

@djones6
Copy link
Contributor Author

djones6 commented Nov 28, 2018

@billabt I've restored the excludes - can you explain what effect this has (is it in a particular build environment)?

The documentation for SwiftPM led me to believe this is a feature that allows it to ignore specific sources. But since the files listed here are not sources that SwiftPM would consider, I thought they were unnecessary.

Re. BlueSSLService, I was holding off on a PR because it depends on this one, but I've now raised Kitura/BlueSSLService#64 - it's much more straightforward because the Package files are already in a Swift 5 compatible format, but it'll fail until this change is tagged.

@billabt
Copy link
Collaborator

billabt commented Nov 29, 2018

@djones6 I'm under the impression that the excludes will speed up the SPM processing since SPM will know upfront not to process those files.

@billabt billabt merged commit e15b0bc into master Nov 29, 2018
@billabt billabt deleted the issue.swift5 branch November 29, 2018 13:38
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.

None yet

3 participants