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

(refactor) refactored MXLRUCache to swift #1544

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mredig
Copy link
Contributor

@mredig mredig commented Aug 6, 2022

Per #1497, the larger issue is that this project is just not SPM compatible in the first place, so I'm hoping to, as time allows, contribute what I can converting code over to Swift.

I started with a smaller, simpler file to help get hello worlded into the project. I hope to have more in the near future! :D

Signed-off-by: Michael Redig hybrids-07-baggage at icloud.com

Pull Request Checklist

@mredig mredig marked this pull request as ready for review August 6, 2022 07:43
@pixlwave
Copy link
Member

pixlwave commented Aug 9, 2022

Thanks for this PR @mredig. I don't suppose you could share your plan of attack in regards to #1497. I'm not sure how you chose to start with MXLRUCache, and we're slightly nervous of simply re-writing files (especially ones that aren't well tested) with the potential to introduce regressions into the project.

Whilst it is still very early days, our focus for the future of a MatrixSDK for Swift (with SwiftPM support out of the box) is going to be https://github.com/matrix-org/matrix-rust-components-swift, which may not be helpful for you right now, but might help provide some context if you haven't yet seen this :)

@mredig
Copy link
Contributor Author

mredig commented Aug 10, 2022

I don't suppose you could share your plan of attack in regards to #1497.
Plan?! ha!

To be honest, I was just approaching it with the mindset that the ultimate issue is the mix of ObjC and Swift in the project, preventing the mixed target SPM build. I'd have to gain some familiarity with the project to really know whether it'd be a matter of separating some components out to make two targets, one with Swift and the other with ObjC with one relying on the other, or if it'd just make more sense to convert everything over to Swift.

...going to be https://github.com/matrix-org/matrix-rust-components-swift, which may not be helpful for you right now, but might help provide some context if you haven't yet seen this :)

I hadn't seen that! However, without looking at it yet, I'd agree it's probably too early. I'll check it out, though. (I'm honestly not sure if it's possible to link Rust into Swift, but maybe you know something I don't)

@mredig
Copy link
Contributor Author

mredig commented Aug 10, 2022

I'm not sure how you chose to start with MXLRUCache, and we're slightly nervous of simply re-writing files (especially ones that aren't well tested) with the potential to introduce regressions into the project.

I started there because it was a file I randomly landed on and seemed relatively self contained. It was just an easy item to start with.

I had hoped that tests would have coverage to assert these things would provide some more confidence.

However, while I may be the only one (tho I doubt it), I feel that Cocoapods could really hinder the usage and adoption of this project. The more I learn about Matrix, the more I want it to become the new standard for communication. My opinion is such that something needs to be done sooner than later to modernize this SDK for usage in the modern iOS ecosystem. If the Rust project is still very early and will take a while to get full SPM support, especially since SPM isn't exactly new anymore. Additionally, there isn't even Carthage support. These lack of options is quite frustrating, tbh.

I also don't wanna be one of those people demanding free tech support from open source projects, so I wanna help however I can! This is also the way I see most likely to get SPM asap going forward.

@mredig
Copy link
Contributor Author

mredig commented Aug 10, 2022

(I'm honestly not sure if it's possible to link Rust into Swift, but maybe you know something I don't)

I should look at resources before I speak! /facepalm I see you've already got some code over there presumably working.

As you said it's very early in the project, what needs to be done over there? I'm not familiar with Rust, nor linking it to Swift (obviously given my previous dumbass assumptions), but maybe I'd have something to contribute over there instead.

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022

Codecov Report

Attention: Patch coverage is 0% with 45 lines in your changes missing coverage. Please review.

Project coverage is 42.12%. Comparing base (56a23ee) to head (e097cec).
Report is 1011 commits behind head on develop.

Files with missing lines Patch % Lines
MatrixSDK/Utils/MXLRUCache.swift 0.00% 45 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1544      +/-   ##
===========================================
+ Coverage    42.10%   42.12%   +0.01%     
===========================================
  Files          515      515              
  Lines        84113    84087      -26     
  Branches     37064    37065       +1     
===========================================
+ Hits         35418    35419       +1     
+ Misses       47663    47636      -27     
  Partials      1032     1032              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pixlwave
Copy link
Member

pixlwave commented Aug 12, 2022

I was just approaching it with the mindset that the ultimate issue is the mix of ObjC and Swift in the project, preventing the mixed target SPM build. I'd have to gain some familiarity with the project to really know whether it'd be a matter of separating some components out to make two targets, one with Swift and the other with ObjC with one relying on the other, or if it'd just make more sense to convert everything over to Swift.

Given the size of the project, and how mixed together some of the Swift/Objective-C code is, I have a feeling the most sensible solution here would be to continue to build an xcframework and provide a binary package ala https://github.com/niochat/MatrixSDK. It would be a case of looking to the the blocking issues which as I see it are:

  • There's an issue with SwiftyBeaver module in the interface. A hack for this that I found (but never had the time to evaluate if it is sane or not) was to run the following after building the framework.
    find MatrixSDK.xcframework/**/MatrixSDK.swiftmodule/*.swiftinterface -exec sed -i '' '/import SwiftyBeaver/d' {} +
  • Figure out how to handle the change to statically link pods during build .

However, while I may be the only one (tho I doubt it), I feel that Cocoapods could really hinder the usage and adoption of this project… These lack of options is quite frustrating, tbh.

Totally understand this (and it was the reason the Nio package was created in the first place) 🙂

I also don't wanna be one of those people demanding free tech support from open source projects, so I wanna help however I can! This is also the way I see most likely to get SPM asap going forward.

Thank you, this is a very appreciated mindset 🙏

I should look at resources before I speak! /facepalm I see you've already got some code over there presumably working.

As you said it's very early in the project, what needs to be done over there? I'm not familiar with Rust, nor linking it to Swift (obviously given my previous dumbass assumptions), but maybe I'd have something to contribute over there instead.

So this is the more exciting part! ElementX is already running on top of that package and providing a basic experience. The Rust team are currently working on implementing robust APIs for Sliding Sync and the Timeline. Outside of that, its a case of figuring out which parts of the SDK are useful and exposing them through the matrix-sdk-ffi crate. We're aiming to have a fairly high-level API on the bindings side and abstracting away most of the complexities into Rust itself.

@pixlwave
Copy link
Member

pixlwave commented Aug 12, 2022

Note: The Nio package is building the xcframework in the GitHub actions workflow, but you can run the following in the project which was based off the same steps:

./build.sh xcframework {CFBundleShortVersionString} {CFBundleVersion}

AlexandreHauber added a commit to AlexandreHauber/MatrixSDK that referenced this pull request Aug 12, 2022
@mredig
Copy link
Contributor Author

mredig commented Aug 12, 2022

  • There's an issue with SwiftyBeaver module in the interface. A hack for this that I found (but never had the time to evaluate if it is sane or not) was to run the following after building the framework.
    find MatrixSDK.xcframework/**/MatrixSDK.swiftmodule/*.swiftinterface -exec sed -i '' '/import SwiftyBeaver/d' {} +

Should this work on the Realm import issue as well? (It appears to be the same thing, but I can get around SwiftyBeaver by including it via an SPM package in my final project, but Realm refuses to work with the same fix, despite reporting the identical error)

  • Figure out how to handle the change to statically link pods during build .

Are we sure that would fix it? One approach I tried on my fork of the project was to remove both SwiftyBeaver and Realm from the podfile and add them via SPM, which I believe would have linked them statically at that point (at least, that was my intention). It resulted in the exact same errors.

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