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

Add Basic Windows & Linux Support #184

Merged
merged 41 commits into from
Jan 11, 2024

Conversation

brianmichel
Copy link
Contributor

What kind of change does this PR introduce?

This adds the ability to use this SDK on Windows and Linux environments.

What is the current behavior?

Currently, the SDK doesn't compile on Linux or Windows.

What is the new behavior?

This PR adds support to compile, test, and run the SDK on Windows and Linux.

Additional context

As with all things, Swift on non-Darwin platforms comes with some caveats:

  • The keychain doesn't exist on non-Darwin platforms. I've made a stub to get the SDK working, and left comments on what will probably need to happen in a different PR (tl;dr it will probably be easier to add the Windows support here compared to Linux since there are stable APIs on Windows to use)
  • All of the test files had to be broken out on different platforms for two reasons:
    1. Windows uses crlf line endings instead of Darwin and Linux which use lf line endings, this is quite annoying, but that's life
    2. Foundation on non-Darwin behaves differently and does some auto-canonicalization of http headers (i.e. apikey becomes Apikey), except anything starting with an X- which creates ordering issues when using the textual format.

I was unable to get the examples to run on my mac since it looks like they required a different team to get setup, so I'm hoping that running them on this repo's CI will work, but I'm seeing some success on my fork https://github.com/brianmichel/supabase-swift/actions/runs/7160617987/job/19495202392?pr=1

Looking forward to the discussion :D

@brianmichel brianmichel requested a review from grdsdev as a code owner December 10, 2023 22:40
Copy link
Collaborator

@grdsdev grdsdev left a comment

Choose a reason for hiding this comment

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

Thank you for the hard work on PR this, it looks awesome.

I left a few comments for discussion.

Thanks again!

Sources/Realtime/PhoenixTransport.swift Show resolved Hide resolved
Sources/Realtime/PhoenixTransport.swift Outdated Show resolved Hide resolved
Sources/_Helpers/URLSession+AsyncAwait.swift Outdated Show resolved Hide resolved
Sources/_Helpers/URLSession+AsyncAwait.swift Outdated Show resolved Hide resolved
Tests/GoTrueTests/GoTrueClientTests.swift Outdated Show resolved Hide resolved
Tests/GoTrueTests/RequestsTests.swift Outdated Show resolved Hide resolved
Sources/GoTrue/GoTrueLocalStorage.swift Outdated Show resolved Hide resolved
Sources/GoTrue/Internal/PKCE.swift Outdated Show resolved Hide resolved
Sources/GoTrue/Internal/FixedWidthInteger+Random.swift Outdated Show resolved Hide resolved
Sources/_Helpers/AssertSnapshot+Platform.swift Outdated Show resolved Hide resolved
@brianmichel
Copy link
Contributor Author

Hey @grdsdev sorry for the delay, I'm going to pick this back up today and see if I can address some of the changes you've requested.

@brianmichel
Copy link
Contributor Author

Hey @grdsdev I'm seeing errors regarding swift evolution support, which trace back to snapshot testing and I see an open issue on their repo pointfreeco/swift-snapshot-testing#536 has this CI job worked before?

@brianmichel
Copy link
Contributor Author

Hey @grdsdev I'm pretty close to responding to all of your feedback, let me know what you think of the approach I've uploaded, looks like all of CI that was passing is passing again https://github.com/brianmichel/supabase-swift/actions/runs/7240544944/job/19723650563?pr=1. I'll open up convos with the Pointfree folks in a little bit.

I might have to rebase this another day as I imagine it's going to be quite messy.

@brianmichel brianmichel force-pushed the brian/compile-on-windows branch from 2d8c882 to e28cb4e Compare January 7, 2024 16:52
@brianmichel
Copy link
Contributor Author

Hey @grdsdev I believe I've fixed all of the issues and my latest CI run was all green brianmichel@2d8c882 there are likely some lingering questions that should be addressed, but I'd say this is 100% ready for a re-review and potentially ready for merging.

Examples/Examples/ExamplesApp.swift Outdated Show resolved Hide resolved
Sources/Auth/AuthClient.swift Show resolved Hide resolved
Tests/AuthTests/GoTrueClientTests.swift Show resolved Hide resolved
Tests/AuthTests/RequestsTests.swift Show resolved Hide resolved
@brianmichel brianmichel requested a review from grdsdev January 7, 2024 16:58
Examples/Examples/ExamplesApp.swift Outdated Show resolved Hide resolved
Sources/_Helpers/URLSession+AsyncAwait.swift Outdated Show resolved Hide resolved
Sources/_Helpers/URLSession+AsyncAwait.swift Show resolved Hide resolved

helper.register(task)

task.resume()
Copy link

Choose a reason for hiding this comment

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

i think this has no guaranteed ordering relative to the effective helper calls. i.e. the task will start running ~here even if the on-actor actuallyCancel/actuallyRegister calls haven't yet executed. does that matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had this same question when looking at the reference code. I'm not sure if it's totally avoidable, and we might end up with whatever quirks the reference has as well.

One complication is that the onCancel closure is required to be synchronous so I'm not sure how to improve upon what the async http client folks have already cooked up. Any ideas?

Copy link

Choose a reason for hiding this comment

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

i think one possibility would be to merge resumption of the task into the register invocation (registerAndResume(...)?). i'm not sure there would be any issues in practice with the code as-is (cancellation now seems like it will eventually be propagated), but since swift concurrency gives no assurances about the ordering of submitted tasks, or how long it might take before they're run, it seems like there's a theoretical opening for weird behavior where the data task runs for a non-trivial amount of time before the cancel/register calls are made (or could it even run to completion before registering/canceling in some edge case?). the main benefit of moving resumption into the registration logic might be to just eliminate having to think through this sort of scenario.

again, i don't think a change is necessarily needed here... mostly this is just a question out of curiosity.

@brianmichel brianmichel requested review from grdsdev and jamieQ January 9, 2024 03:27
@brianmichel brianmichel requested a review from jamieQ January 9, 2024 14:01
Copy link
Collaborator

@grdsdev grdsdev left a comment

Choose a reason for hiding this comment

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

Awesome work, I'll have it merged soon, thank you!

@grdsdev grdsdev merged commit 37b68d2 into supabase:main Jan 11, 2024
5 checks passed
@Jeehut Jeehut mentioned this pull request Jan 18, 2024
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.

3 participants