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

Remove libstd from objc-sys and block-sys #305

Closed
wants to merge 5 commits into from
Closed

Remove libstd from objc-sys and block-sys #305

wants to merge 5 commits into from

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Dec 17, 2022

This removes libstd as a dependency for objc-sys and block-sys, by replacing std::os::raw with core::ffi, which contains the necessary types.

This bumps to MSRV to at least 1.64, but it looks like this package doesn't have an MSRV policy yet (#203), so I hope that's not an issue.

@madsmtm
Copy link
Owner

madsmtm commented Dec 21, 2022

Yeah, this crate doesn't have a policy yet so this wouldn't be a breaking change; however, I would still like it to work with winit, which is currently at 1.60.0 with a rough policy of "every Rust version released in the last 6 months".

@madsmtm madsmtm added the enhancement New feature or request label Dec 21, 2022
@notgull
Copy link
Contributor Author

notgull commented Dec 21, 2022

We could use libc or cty to circumvent the MSRV bump, if you're fine with the extra dependency.

@madsmtm
Copy link
Owner

madsmtm commented Dec 21, 2022

I think I've also sort-of tried to keep the amount of dependencies down.

For now, I've kinda deemed that the downsides of "bump Rust version" or "added dependency" outweigh the benefit "no_std support", since these are basically only useful on Apple platforms, where the standard library is already present.

But in a few months, when 1.64 is in winit's MSRV, I'll probably do this change.

@silvanshade
Copy link
Contributor

But in a few months, when 1.64 is in winit's MSRV, I'll probably do this change.

One thing to note is that the Tauri project seems to want to keep an MSRV of 1.61. I'm not sure if I would let that be a deciding factor, just something to be aware of considering #174.

@notgull notgull closed this by deleting the head repository Mar 28, 2023
@notgull
Copy link
Contributor Author

notgull commented Apr 4, 2023

Whoops, I was deleting forks and I accidentally deleted the fork with this change.

Now that winit has an MSRV of 1.64, would this change be accepted now?

@notgull notgull reopened this Apr 4, 2023
Copy link
Owner

@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.

Yeah, I'd be fine with a MSRV bump, mostly because some things are much easier to express with GATs (from Rust 1.65).

Another thing that would be nice is more capabilities for const fn, esp. the fn pointer handling for eventually allowing safe method overriding in declare_class!. I'd also really like to start using ptr::cast_mut!


All in all we don't really gain that much from the bump yet though, since objc2 still needs std::sync::Once for declare_class!.
I think perhaps we could avoid that completely with a helper like objc2::__macro_helpers::CachedClass + (ab-)using the fact that the objc runtime is locked while creating classes? Or maybe we'd need to re-implement parts of std::sync::Once? Or perhaps do so using functionality from libdispatch?

Do you know what the recommended approach is for std::sync::Once-like functionality in no_std?

Comment on lines +27 to -31
#[cfg(feature = "std")]
extern crate std;

#[cfg(not(feature = "std"))]
compile_error!("The `std` feature currently must be enabled.");

Copy link
Owner

Choose a reason for hiding this comment

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

Please do roughly the same setup as I did in this PR to the url project (for both alloc and std, since neither are required).

@notgull
Copy link
Contributor Author

notgull commented Apr 27, 2023

Not as enthusiastic about this anymore.

@notgull notgull closed this Apr 27, 2023
@madsmtm
Copy link
Owner

madsmtm commented May 2, 2023

No problem, I'll probably get around to doing it myself at some point, then I'll use the code here

@madsmtm madsmtm mentioned this pull request Sep 3, 2023
80 tasks
madsmtm added a commit that referenced this pull request Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants