Skip to content

Use objc2-core-foundation and objc2-core-services#726

Merged
JohnTitor merged 3 commits intonotify-rs:mainfrom
madsmtm:objc2
Jan 12, 2026
Merged

Use objc2-core-foundation and objc2-core-services#726
JohnTitor merged 3 commits intonotify-rs:mainfrom
madsmtm:objc2

Conversation

@madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Oct 28, 2025

objc2-core-foundation provides safer and auto-generated wrappers around CoreFoundation than the ones provided by fsevent-sys. In particular, you no longer have to worry about when to call CFRelease, objc2-core-foundation will mostly take care of that for you.

objc2-core-services does the same, but for CoreServices, in particular the FSEvents API that notify uses.

I maintain both crates as part of the objc2 project.

I have tried to keep the functionality as close to the existing as possible, to make this PR easier to review and bisect, though I suspect the code in append_path and remove_path could be simplified.

This is an alternative to updating to fsevent-sys v5.1.0. I have filed a similar PR to fsevent in octplane/fsevent-rust#48.

@riberk
Copy link
Contributor

riberk commented Oct 28, 2025

I'm not a member, but I noticed your suggestion to replace servo core foundation with these crates (the team has accepted the changes, but they haven't been merged yet), and also the wgpu team did the same.

I think it’s worth mentioning, since they have a much larger and more active community, and they might have already investigated these crates.

IMO, it’s a good idea, but personally I’d prefer us to have more tests — especially for corner cases — before applying the update.

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 30, 2025

IMO, it’s a good idea, but personally I’d prefer us to have more tests — especially for corner cases — before applying the update.

Fair enough, how do you propose we test things like this though? It's kinda hard, since most of it touches the file system and involves multiple processes to test their interaction.

@dfaust
Copy link
Member

dfaust commented Oct 30, 2025

I like it. But I don't have a mac to test it.

@riberk
Copy link
Contributor

riberk commented Oct 31, 2025

Fair enough, how do you propose we test things like this though? It's kinda hard, since most of it touches the file system and involves multiple processes to test their interaction.

OK, I'm going to add tests from the old PR and others, if something else comes to my mind

@dfaust
Copy link
Member

dfaust commented Oct 31, 2025

@riberk You are very welcome to do so. But be careful, these tests used to be extremely flaky. That's why we removed them at some point. In order to be useful we need automatic retries.

@riberk
Copy link
Contributor

riberk commented Oct 31, 2025

@riberk You are very welcome to do so. But be careful, these tests used to be extremely flaky. That's why we removed them at some point. In order to be useful we need automatic retries.

Yeah, I got it. I think I’ll be able to adapt them so they won’t be flaky — I’ll use channels instead of timeouts, ignore the order of events, and so on.

@JohnTitor
Copy link
Member

The tests added in main now, could you rebase and ensure if something isn't broken? @madsmtm

@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 6, 2025

cargo test passes on macOS 15.6.1 if that's all you wanted me to test?

I did also try out the examples, they seemed to act the same before and after.

@JohnTitor
Copy link
Member

Yeah, thank you!
I think the tests added by @riberk would be a great safeguard here.

Basically I'd like to accept this replacement, given the community leans to accept transitions and the benefits from it.
I'm going to take a deeper look in a week or so.

sapphi-red added a commit to rolldown/notify that referenced this pull request Dec 19, 2025
Pulling in notify-rs#726 before
introducing more changes.

Co-authored-by: Mads Marquart <mads@marquart.dk>
This is an alternative to updating to fsevent-sys v5.1.
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Sorry for the big delay, I was too busy with my day job...
So, this LGTM with minor tweaks I've added. I think the tests ensure most of things but let's ship it as a new major version just in case (the release date is TBD but I'm going to clean up the PR and issue queues then check-in).

@JohnTitor JohnTitor merged commit fe7a961 into notify-rs:main Jan 12, 2026
17 checks passed
@madsmtm madsmtm deleted the objc2 branch January 13, 2026 03:05
JohnTitor added a commit that referenced this pull request Jan 16, 2026
Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>
JohnTitor added a commit to JohnTitor/notify that referenced this pull request Jan 16, 2026
JohnTitor added a commit to JohnTitor/notify that referenced this pull request Jan 16, 2026
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.

4 participants