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

requiring Sendable from Baggage is source breaking #22

Closed
weissi opened this issue Jan 28, 2022 · 8 comments
Closed

requiring Sendable from Baggage is source breaking #22

weissi opened this issue Jan 28, 2022 · 8 comments
Milestone

Comments

@weissi
Copy link
Member

weissi commented Jan 28, 2022

This commit required that types confirming to BaggageKey are now Sendable on Swift 5.5+.

That change obviously makes sense but unfortunately, it's source breaking...

That's fine by SemVer (because this package is version 0.x) but probably still an issue.

@slashmo
Copy link
Collaborator

slashmo commented Jan 28, 2022

Thanks for opening the issue, @weissi! 👍 I hit this breaking change in "OpenTelemetry Swift", but am updating now to account for Sendable.

That's fine by SemVer (because this package is version 0.x) but probably still an issue.

Agreed, I think it's okay to break while were in the pre-SemVer stage to move more quickly.

@weissi
Copy link
Member Author

weissi commented Jan 28, 2022

Agreed, I think it's okay to break while were in the pre-SemVer stage to move more quickly.

Sure, it's okay by the rules but breakage between 0.2.1 and 0.2.2 is still not acceptable I think. Usually, people depend on 0.x packages with upToNextMinor because breaking API usually goes into a new minor version. So I think what should happen is:

  • release a 0.2.3 which reverts this change
  • release a 0.3.0 which brings back the breaking change

@slashmo
Copy link
Collaborator

slashmo commented Jan 28, 2022

  • release a 0.2.3 which reverts this change
  • release a 0.3.0 which brings back the breaking change

Ah, I thought it was a minor bump. In that case, I agree with your plan to move the change to a 0.3.0. @ktoso Since you originally made the release, are you okay these steps?

@ktoso
Copy link
Member

ktoso commented Jan 28, 2022

No reason to panic. We don't need to release a revert. Let's calmly think this through first

@ktoso
Copy link
Member

ktoso commented Jan 28, 2022

Heh ok I read the reasoning now, ok sure we can do that. Tho that's strictly stronger guarantees than semver ever does 😉

happy with the proposed plan. I will think more how this would have had to be handled in a stable package so we can document it

@ktoso ktoso added this to the 0.3.0 milestone Jan 29, 2022
@ktoso
Copy link
Member

ktoso commented Jan 29, 2022

Doing the reverts now and I'll talk with concurrency folks how such change could be possible if a module were stable -- I guess this honestly means that users would use it with "@preconcurrency" and by itself is not a breaking change then 🤔

@ktoso
Copy link
Member

ktoso commented Jan 29, 2022

Tho to be honest the actual solution is @preconcurrency import but that didn't land yet (well it did, but not in a release), so we need this dance for now.

Hope that works @weissi

@weissi
Copy link
Member Author

weissi commented Jan 29, 2022

Tho to be honest the actual solution is @preconcurrency import but that didn't land yet (well it did, but not in a release), so we need this dance for now.

If users have to do anything, then it’s a breaking change.
This needs to fixed another way. Users compiling with -swift-version 5 (Usually automatically derived from // swift-tools-version: <5.5) obviously shouldn’t get a new requirement because they clearly predate Concurrency.

Not sure how that’s best expressed in the source language that this should only be Sendable if the client is using a Swift language version that’s new enough.

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

No branches or pull requests

3 participants