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

macos: Migrate to objc2-core-foundation #96

Closed
wants to merge 1 commit into from

Conversation

MarijnS95
Copy link
Contributor

All tests succeed. Opening as draft because there are no helpers yet an all functions have to be called on a low level with no safety abstractions. Following up on #95 because I upgraded objc2 dependencies for iOS but noticed we could just as well utilize the new objc2 CF crate for MacOS now.

The objc2-core-servcies crate is empty as of yet so all these external functions remain defined in-line.

CC @madsmtm

@MarijnS95 MarijnS95 force-pushed the objc2-core-foundation branch 2 times, most recently from 8954442 to 0c63319 Compare March 1, 2025 00:47
@madsmtm
Copy link
Contributor

madsmtm commented Mar 1, 2025

Yeah, objc2-core-foundation isn't really ready for widespread usage yet, see e.g. madsmtm/objc2#692.

(I'll also link to discussion in #87 regarding supply-chain attacks, the concerns there are more pronounced when targetting macOS).

@amodm
Copy link
Owner

amodm commented Mar 1, 2025

@MarijnS95 unlike #87, it's less clear why this PR is useful/important to webbrowser. My comment #95 (comment) on your other PR is applicable here too.

@MarijnS95
Copy link
Contributor Author

Sure, it's less obvious because unlike the iOS code core-foundation has no dependency on ancient objc bindings. Regardless, it seems @madsmtm is more proactive about creating a high-quality, well maintained and coherent ecosystem around Apple bindings, which are above all autogenerated.

For the time being this might not look like a win as no high(er)-level bindings are available and certain helpers had to be copied/inlined from core-foundation.

It might be better to hold off merging this for a little bit, but it's hopefully useful for @madsmtm to inventorize how certain ecosystem crates might be using these Apple/objc bindings and from what improvements they could benefit the most.

@madsmtm
Copy link
Contributor

madsmtm commented Mar 1, 2025

it's hopefully useful for @madsmtm to inventorize how certain ecosystem crates might be using these Apple/objc bindings and from what improvements they could benefit the most.

It is! Thanks! And I do think webbrowser should use the crates on macOS too some day!

@amodm
Copy link
Owner

amodm commented Mar 1, 2025

I would certainly like to move to objc2 on macos at some point in time. However:

  1. I'd like to give it some more time for reasons that @madsmtm has pointed to already.
  2. When implemented, the code cannot have all the unwraps that it currently does.

Given that I plan to revisit this in a few months time, by which time the nature of changes would likely expected to be different, I'm closing this PR. If you don't expect the code to be much different in a few months (except for the clean up of all the unwraps), you can reopen this PR and I'll let it remain open until I revisit this.

@amodm amodm closed this Mar 1, 2025
@MarijnS95
Copy link
Contributor Author

Pushing to this PR (to improve the unwraps and experiment with other changes) would mean it can no longer be reopened. And I cannot reopen it myself after it was closed by a maintainer for that purpose.

@amodm amodm reopened this Mar 2, 2025
@amodm
Copy link
Owner

amodm commented Mar 2, 2025

I'm leaving it open in that case.

@MarijnS95 MarijnS95 force-pushed the objc2-core-foundation branch from 0c63319 to 1690c6d Compare March 8, 2025 10:12
@MarijnS95 MarijnS95 closed this Mar 9, 2025
@MarijnS95 MarijnS95 deleted the objc2-core-foundation branch March 9, 2025 15:44
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