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

Freeze MXResponse enum #1002

Closed
wants to merge 2 commits into from
Closed

Freeze MXResponse enum #1002

wants to merge 2 commits into from

Conversation

pixlwave
Copy link
Contributor

@pixlwave pixlwave commented Jan 27, 2021

When using niochat/MatrixSDK (or MatrixSDK.xcframework) any switch against MXResponse produces a warning that unknown cases are unhandled.

Hoping this change is welcome as I can't foresee any other cases that could be added to MXResponse?

Please let me know if there's any changes you would like.

Signed-off-by: Doug Earnshaw pixlwave@users.noreply.github.com

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request updates CHANGES.rst
  • Pull request includes a sign off

@SBiOSoftWhare
Copy link
Contributor

Hi @pixlwave thanks to point the issue. But by default user defined Swift enums are frozen: All enums written in Swift outside of the standard library and overlays will implicitly be considered frozen in Swift 4.2. Enums imported from C will be non-frozen by default, with a new C-side annotation to treat them as frozen. (from https://github.com/apple/swift-evolution/blob/master/proposals/0192-non-exhaustive-enums.md).
If you don't use CocoaPods can you check the build options you are using when you compile the SDK? There are also other consideration on build settings option regarding frozen attribute you can find here https://docs.swift.org/swift-book/ReferenceManual/Attributes.html

@pixlwave
Copy link
Contributor Author

pixlwave commented Feb 17, 2021

Hey, thanks for looking into this 😄. We're building it with:

xcodebuild archive -workspace MatrixSDK.xcworkspace -scheme MatrixSDK-iOS -destination "generic/platform=iOS" -archivePath build/MatrixSDK-iOS SKIP_INSTALL=NO BUILD_LIBRARY_FOR_DISTRIBUTION=YES IPHONEOS_DEPLOYMENT_TARGET=11.0 GCC_WARN_ABOUT_DEPRECATED_FUNCTIONS=NO

(See #982)

I guess it'll be BUILD_LIBRARY_FOR_DISTRIBUTION=YES which is causing issues as this is enabling library evolution (I think with -enable-library-evolution). There's more here: https://swift.org/blog/library-evolution/ but sadly it seems that building XCFrameworks results in differences to the compiled code 😕

@pixlwave
Copy link
Contributor Author

Interestingly that post notes:

Swift Package Manager packages or binary frameworks that are internal to your app, should not be built with library evolution support.

I'll check and see if it's possible to build an XCFramework without BUILD_LIBRARY_FOR_DISTRIBUTION=YES as that would definitely be preferable for our use case (I had thought it was a requirement 🤔 after watching the WWDC session)

@pixlwave
Copy link
Contributor Author

pixlwave commented Feb 17, 2021

Hmmmm, it seems not 😣 The XCFramework gets created but none of the slices are added. I guess the post means if those packages/frameworks are built as part of your app ☹️

@SBiOSoftWhare
Copy link
Contributor

Indeed from Swift documentation

When the compiler isn’t in library evolution mode, all structures and enumerations are implicitly frozen, and this attribute is ignored.

@pixlwave pixlwave changed the base branch from develop to frozen-mxresponse December 17, 2021 17:06
@pixlwave pixlwave changed the base branch from frozen-mxresponse to develop December 17, 2021 17:07
@pixlwave pixlwave mentioned this pull request Dec 17, 2021
@pixlwave
Copy link
Contributor Author

Closed in favour of #1330 which includes an updated changelog. (The remote repository this was based on was deleted and so no further changes can be pushed to this PR is it seems).

@pixlwave pixlwave closed this Dec 17, 2021
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.

None yet

2 participants