search

launchdarkly / swift-eventsource

Server-sent events (SSE) client implementation in Swift for iOS, macOS, tvOS, and watchOS
https://launchdarkly.github.io/swift-eventsource/
Other
187 stars 34 forks source link

Remove explicitly typed package products #48

Closed simba909 closed 2 years ago

simba909 commented 2 years ago

Requirements

Related issues

Didn't create an issue up-front as it seemed like more work than just contributing a PR :)

Describe the solution you've provided

As of Xcode 14 beta 5 (have not tested earlier betas, it works on 13.4.1), adding the LaunchDarkly SDK to a project breaks SwiftUI previews. The error you get is fairly non-descriptive but strongly hints to an issue finding an executable (?) for the LDSwiftEventSourceStatic product:

SettingsError: noExecutablePath(<IDESwiftPackageStaticLibraryProductBuildable:ObjectIdentifier(0x0000600046ac78d0):'LDSwiftEventSourceStatic'>)

Looking at the Package.swift file for LaunchDarkly I noticed that it's referencing the explicitly static typed target for the event source package. Changing this to a target without explicit package type solves the issue. This is trivially reproducible using Xcode 14b5:

I'm not sure why this package declares both types explicitly, as this isn't the recommended approach when a package supports both static and dynamic linking as noted by SPM documentation (note the type parameter docs). I'm sure there's a reason though, so in order to limit breaking changes this PR renames the current LDSwiftEventSource product to LDSwiftEventSourceDynamic and introduces a new LDSwiftEventSource without an explicit product type. Doing this and changing the LaunchDarkly package to reference this new target instead of the static one, the project created above renders previews successfully. The downside is that any library consumers relying on the fact that LDSwiftEventSource is of dynamic type now might see build/linkage failures.

Describe alternatives you've considered

None, as I'm not sure what other solutions might exist.

Additional context

None.

keelerm84 commented 2 years ago

Thank you for this contribution, and my apologies for the delay in responding.

I'm not sure why this package declares both types explicitly

It seems this change is actually tied back to an issue reported upstream on the SDK. You can see reports here and here.

However, I am not sure this is an issue any longer. I did some preliminary testing and removing the explicit target types doesn't seem to generate a binary with the same @rpath output from otool that was reported in the original issue.

in order to limit breaking changes

I actually just merged a long pending PR that is going to require a version bump anyway. So I think we might as well remove the additional dynamic product you added and just leave it as the docs suggest it should be.

What do you think?

simba909 commented 2 years ago

Hey @keelerm84, thank you for the feedback! Also, thank you for looking into whether both targets are still needed, much appreciated. Agree that the explicitly dynamic target can be removed. Should I keep the static one for now or remove that as well?

keelerm84 commented 2 years ago

Should I keep the static one for now or remove that as well?

I would think you could remove that one as well. Theoretically the package manager should be able to figure out what type of linking it should do if we leave it blank. :shrug:

I know this will require a change to the SDK as well, but I believe it should be fine as that upstream change shouldn't in itself be version breaking.

simba909 commented 2 years ago

@keelerm84 Removed both explicitly typed products now and changed the commit message / PR title to match 🙂

keelerm84 commented 2 years ago

Thank you for making those updates!

I have some internal blockers I have to work through before I can release this, and then subsequently the SDK. If I can't get to this by the EOD, it might be delayed another week as I have some PTO. But when I get back, if I will make sure to get this released and the SDK updated as well.

keelerm84 commented 2 years ago

@simba909 sorry for the delay. I have released the update iOS SDK (6.2.0). Thanks again for your contribution!