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

[Misc] Fix more concurrency warnings + CI for beta platforms #422

Merged
merged 22 commits into from
Jul 17, 2024

Conversation

Alex293
Copy link
Contributor

@Alex293 Alex293 commented Jun 18, 2024

This is pretty much WIP, not ready for full review, just to get feedback if someone wants to try as I make progress or contribute.

This builds on Xcode 16 / Swift 6 and 5 mode and Xcode 15 swift 5.
Tests are passing except for one that can't find an import. I'll look into it.

Now I went for the easy path annotating quite a lot of thing with @mainactor which might simply be sendable instead. I first wanted to have something compilable and test to run. I'll check deeper now if I can avoid some MainActor requirements as one might want to use some of the conditions for code outside of swift despite this not being the main intent of the lib.

Open to feedbacks, it's my first open contribution outside of some small bug fixes.

Edit: Fixes #418

@GeorgeElsham
Copy link
Contributor

Fixes #418

@GeorgeElsham
Copy link
Contributor

Hmm well I thought that would link the issue and this pull request but apparently not. Seems like you must say it in the pull request description. See here.

@Alex293
Copy link
Contributor Author

Alex293 commented Jun 21, 2024

I finally didn’t have time to investigate further this week. I’m planning on trying things next week. @GeorgeElsham can you try this branch and tell me if you have issues in your project like over constraints to main actor ? That way I can adapt the PR before asking for review. We only use small parts of the library so I’m not sure if the choices I made are always sensible.

@GeorgeElsham
Copy link
Contributor

I don't use all that much of the package. The main thing is no warnings in complete concurrency mode and no errors in Swift 6 mode. SwiftUI views are @MainActor by default so I suspect a lot of things will have that annotation. I'll have to check it out later (today, ideally) but this looks like a good start after skim read of the changes. I'm by no means a Swift 6 concurrency expert but I'll see what I can do.

@Alex293
Copy link
Contributor Author

Alex293 commented Jun 21, 2024

Thing is most of the api is indeed made to be used inside a SwiftUI view but I kept wondering if some should instead be Sendable to enable code outside to use parts of it. I’m not sure it makes total sense though.

@GeorgeElsham
Copy link
Contributor

This builds in my project perfectly fine and removed the warnings. I'll also try to refactor from the main branch too and see if I have less reliance on @MainActor, otherwise if I can't simplify it this looks good to merge.

@GeorgeElsham
Copy link
Contributor

I've made a new pull request which makes far less use of @MainActor. Let me know what you think, continuing the conversation on #424.

@davdroman
Copy link
Collaborator

This builds in my project perfectly fine and removed the warnings.

If this branch fixes all the Swift 6 concurrency warnings, let's get it merged.

I kept wondering if some should instead be Sendable to enable code outside to use parts of it. I’m not sure it makes total sense though.

We can worry about this in a separate PR.

@Alex293 mark as ready for review if you're happy with your changes.

@Alex293 Alex293 marked this pull request as ready for review July 12, 2024 08:06
@Alex293
Copy link
Contributor Author

Alex293 commented Jul 12, 2024

I'll rename the commits, do you prefer a signe one ? or maybe one for the Package.swift edits ?

@davdroman
Copy link
Collaborator

I'll rename the commits, do you prefer a signe one ? or maybe one for the Package.swift edits ?

Doesn't matter, I'm going to squash and merge anyway.

@davdroman davdroman changed the title Feature/swift 6 Support Swift 6 Concurrency Jul 16, 2024
@davdroman
Copy link
Collaborator

davdroman commented Jul 16, 2024

Sorry I didn't see the thumbs up til now, I'm going to get CI in order and then merge.

@davdroman davdroman changed the title Support Swift 6 Concurrency [Misc] Fix more concurrency warnings + CI for beta platforms Jul 17, 2024
@davdroman davdroman merged commit 1e33227 into siteline:main Jul 17, 2024
18 of 19 checks passed
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.

Swift 6 complete concurrency warnings
3 participants