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

Initial conversion to objc2 #30

Merged
merged 19 commits into from
Sep 11, 2023
Merged

Initial conversion to objc2 #30

merged 19 commits into from
Sep 11, 2023

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Jul 10, 2022

Blocked on #29. Part of #28.

An initial pass of just getting things to compile. Upgrade instructions for most of the breaking changes can be found in objc2's CHANGELOG, I've essentially just done as instructed by the examples there.

@ryanmcgrath ryanmcgrath deleted the branch ryanmcgrath:trunk July 12, 2022 05:22
@ryanmcgrath ryanmcgrath reopened this Jul 12, 2022
@ryanmcgrath
Copy link
Owner

Alright, #29 is merged and done... albeit through some extra steps because I am of the idiot clan tonight.

You should be able to rebase this onto trunk cleanly (ish) now. Thanks again for all your work!

@madsmtm
Copy link
Contributor Author

madsmtm commented Jul 18, 2022

Wonderful, no worries about the merge mistake, happens to the best of us ;)

This PR is fairly close to serviceable now (just need to do a new release of some things in objc2, and give things an extra review myself), I'll press the button when I've done so!

Note that we're currently getting a lot of deprecation warnings, will fix those in a subsequent PR.

@ryanmcgrath
Copy link
Owner

Cool, excited!

@madsmtm madsmtm marked this pull request as ready for review July 20, 2022 04:29
Copy link
Contributor Author

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Alright should be ready to give a test drive (warnings will be fixed by #34).

I'd recommend reviewing each commit separately, in particular it's very likely that I've made a mistake somewhere in the last commit 437c539 so extra scrutiny of that one would be nice.

Cargo.toml Outdated
Comment on lines 24 to 27
# Temporary: Patched versions that implement `Encode` for common types
# Branch: `objc2`
core-foundation = { git = "https://github.com/madsmtm/core-foundation-rs.git", rev = "c506e7d70da010c0070048e9471ff0b441506e65", features = ["with-chrono"] }
core-graphics = { git = "https://github.com/madsmtm/core-foundation-rs.git", rev = "c506e7d70da010c0070048e9471ff0b441506e65" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A possible fix for these is #35

Cargo.toml Outdated Show resolved Hide resolved
@ryanmcgrath
Copy link
Owner

I'll find time later this week (or this weekend) to review - I've been at a racing thing for most of the beginning of this week and am exhausted by the time I sit down to my laptop at night. :(

Really excited for this though! Your work is super appreciated.

@madsmtm
Copy link
Contributor Author

madsmtm commented Jul 20, 2022

No problem, take all the time you need - I'm just enjoying my summer holidays, have lots of other things to distract me with ;)

@ryanmcgrath
Copy link
Owner

I am comfortable merging this one whenever makes sense - I want to merge #48 soon, but I'm thinking it might be smarter to get the objc2 stuff out of the way first.

I'd be fine with using the 0.3.x beta series of objc2, but figured I'd ask if there's any bigger concerns you can think of before I merge this?

(Aside: msg_send_id is ergonomically so much better than what was going on with pure msg_send before, so thank you so much for your work!)

@ryanmcgrath
Copy link
Owner

(slash, could you rebase this onto trunk and we can let the updated CI pieces run?)

@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 22, 2022

Just do #48 first, I'm gonna be away for a few days so I won't have time for this before around Friday.

Re beta series, I'm fairly certain I know where I want objc2 to end up for a stable 0.3, the milestone should be fairly accurate and in order now, it's just a matter of actually finishing implementing it. I expect it to take maybe two months in total, and I think this is a fairly accurate assessment, but yeah, anything can happen.

@madsmtm madsmtm changed the base branch from airyx-appkit-uikit-features to trunk August 28, 2022 13:17
@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 28, 2022

Okay, I've rebased this now, thanks for your patience!

@madsmtm madsmtm force-pushed the objc2 branch 2 times, most recently from 3fdc19f to 87fe0e3 Compare August 29, 2022 02:30
@madsmtm madsmtm closed this Aug 29, 2022
@madsmtm madsmtm reopened this Aug 29, 2022
@ryanmcgrath
Copy link
Owner

I haven't forgotten about this - last I checked in on it, I had a few examples crash and I need to look into why. I get the sense it's probably small stuff but I've just been too busy to sit down and poke at it.

(Hoping I have time this weekend tho since I'll be mostly chilling at home)

@ryanmcgrath
Copy link
Owner

ryanmcgrath commented Oct 11, 2022 via email

@ryanmcgrath
Copy link
Owner

ryanmcgrath commented Aug 1, 2023 via email

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 5, 2023

Okay, so I went over the examples, and fixed the memory safety issues I could find by running those. That said, there are probably still more that I haven't found yet, but I feel like it's at a pretty good stage!

Note to self: The following is really useful for debugging double-frees and such:

DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib MallocStackLogging=YES NSZombieEnabled=YES MallocGuardEdges=YES MallocScribble=YES ./target/debug/examples/my_example

@ryanmcgrath ryanmcgrath merged commit 094ed59 into ryanmcgrath:trunk Sep 11, 2023
@ryanmcgrath
Copy link
Owner

Merged. :)

Thank you again for the sheer amount of work here, as well as for the wider ObjC work you're doing in the Rust community. It's really great that someone's picked up the torch and focused on making it all safer and more reliable overall.

I'll let this sit in trunk for a bit to see how people find it - whether we need to fix any bugs or anything - but then if all seems fine I'll eye pushing out the next (-beta, maybe) release that uses this.

@madsmtm madsmtm deleted the objc2 branch September 11, 2023 20:13
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 11, 2023

Perfect!

I think before you can push a beta, we'll need to stop relying on the git dependencies that I had to add temporarily to make things work - I think I have that somewhere in a PR or a stash or something, will try to get it sorted out in the next few days.

@ryanmcgrath
Copy link
Owner

Yup, no rush!

@abhillman
Copy link

FYI: #122

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