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

Stop using opaque return types to resolve build error #38

Merged
merged 3 commits into from
Dec 2, 2023
Merged

Stop using opaque return types to resolve build error #38

merged 3 commits into from
Dec 2, 2023

Conversation

gwynne
Copy link
Contributor

@gwynne gwynne commented Dec 2, 2023

When the 1.0.1 release introduced the use of opaque return types (some Collection<> etc.), it broke certain Xcode builds. Specifically:

  • When the root project is an Xcode project rather than a SwiftPM package,
  • Where at least one other package or project anywhere in the dependency graph also lacks the platform declarations, or declares a minimum supported platform of less than macOS Catalina/iOS 13 etc.

Such builds fail immediately with this error:

swift-http-types/Sources/HTTPTypes/HTTPFields.swift:219:54 'some' return types are only available in iOS 13.0.0 or newer

Adding the missing platform specifiers, as this PR does, fixes the issue by correctly declaring the now-required minimums.

Edit: See discussion below; for the time being, rather than add platform requirements, the use of opaque types has been removed instead.

(N.B.: Technically, this means that 1.0.1 introduced an accidental semver-major break due to source incompatibility; under SwiftPM rules, raising a minimum platform/OS version requirement is always a breaking change, even if - as in my case - no deployment to older platforms was actually happening.)

For the record, I am running Sonoma 14.1.2, Xcode and Swift versions are:

$ xcodebuild -version
Xcode 15.1
Build version 15C5059c
$ swift --version
swift-driver version: 1.87.3 Apple Swift version 5.9.2 (swiftlang-5.9.2.2.56 clang-1500.1.0.2.5)
Target: arm64-apple-macosx14.0

(Xcode 15.1 beta 3)

The same failure also occurred with Xcode 15.0.1.

@guoye-zhang
Copy link
Contributor

@swift-ci test

@guoye-zhang
Copy link
Contributor

I think we should remove the usage of opaque result type for now

@gwynne
Copy link
Contributor Author

gwynne commented Dec 2, 2023

TBH I don't agree - I don't think pre-Catalina Darwin deployment targets are still a major use case, especially for a package like this one which is intended primarily for the server ecosystem.

@guoye-zhang
Copy link
Contributor

While I agree, other packages like swift-nio-extras depend on this package and we don't want to ask all of them to bump the supported OS versions.

@gwynne
Copy link
Contributor Author

gwynne commented Dec 2, 2023

Hm, yeah, that's a good point. It's a shame, though - the opaque result types are so much cleaner 😕

@guoye-zhang
Copy link
Contributor

Yeah. Do you want to update the PR? Should be fine to just move HTTPFieldSequence out and make it private. I'll tag a new release.

…`some` rather than forcing other packages to change their manifests.
@gwynne
Copy link
Contributor Author

gwynne commented Dec 2, 2023

@guoye-zhang PR updated, let me know if I missed anything!

@FranzBusch
Copy link
Member

I haven't fully looked into this but we are already using opaque return types in other packages so I don't think backing the usage out here is needed.
I would like to understand a bit better what the setup is here that leads to the failure because just package based projects work correctly on all of our supported swift versions.

@guoye-zhang
Copy link
Contributor

It fails when building the package in Xcode and select "Any Mac" as the target since it tries to back deploy to macOS 10.13

@guoye-zhang
Copy link
Contributor

Is changing the opaque argument necessary? I thought that's fine to back deploy

@gwynne
Copy link
Contributor Author

gwynne commented Dec 2, 2023

I removed all usage out of an abundance of caution. But yeah, the failure only shows up when building an Xcode project with a generic destination, it's not a configuration/scenario I'd really expect there to be any coverage of.

@guoye-zhang
Copy link
Contributor

@FranzBusch Is it safe to delete the tag to unship the version? We can investigate this a bit further next week

@Lukasa
Copy link

Lukasa commented Dec 2, 2023

Don’t delete the tag: instead, ship a new tag that reverts to the prior version with a higher version number.

@guoye-zhang
Copy link
Contributor

@swift-ci test

@guoye-zhang guoye-zhang changed the title Add missing platform specifications to resolve build error Stop using opaque return types to resolve build error Dec 2, 2023
@guoye-zhang guoye-zhang merged commit 1827dc9 into apple:main Dec 2, 2023
@gwynne gwynne deleted the patch-1 branch December 2, 2023 21:19
@gwynne
Copy link
Contributor Author

gwynne commented Dec 2, 2023

@guoye-zhang Thanks for taking care of this so quickly!

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.

5 participants