technocreatives / core2

The bare essentials of std::io for use in no_std. Alloc support is optional.
https://docs.rs/core2
Apache License 2.0
71 stars 23 forks source link

Fixes for nightly features, bump MSRV #21

Open rmsyn opened 1 year ago

rmsyn commented 1 year ago

Some fixes for features that have become stable, and an MSRV bump to fix MacOS CI builds.

rmsyn commented 1 year ago

Ping @bbqsrc

rmsyn commented 1 year ago

Ping @bbqsrc, is this repository still maintained?

TheBlueMatt commented 1 year ago

Not sure why this bumps the MSRV?

rmsyn commented 1 year ago

Not sure why this bumps the MSRV?

MSRV bump to fix MacOS CI builds

The MacOS CI builds fail without it.

You can confirm the build failure by reverting the MSRV in the CI builder script, and running the CI build against the MacOS targets.

You can also confirm by checking the error messages for the currently failing build on main, if you have access to those.

TheBlueMatt commented 1 year ago

Sure, but what was the issue with macOS, why does it fail? Also you can just bump the macOS job - just because macOS is broken in some way or another doesn't imply that should require an MSRV bump for all machines.

rmsyn commented 1 year ago

just because macOS is broken in some way or another doesn't imply that should require an MSRV bump for all machines.

OK, if macOS is not a supported platform, then it should be removed from the CI.

If it is a supported platform, the MSRV needs to be bumped so the crate can actually build on macOS.

A third way could be to have a split MSRV where you say "Alright so we have MSRV 1.x.y on this that and the other platform, and MSRV 1.z.p on macOS".

The last option looks decidedly wonky to me.

TheBlueMatt commented 1 year ago

Given the crate seems unmaintained I'm not sure it matters, but that's not that wonky - Afaiu older rustc doesn't support modern macOS, which is fine, MSRV can still be checked on older macOS - "if your system can run rustc X, we support it" seems like a great policy to me.

rmsyn commented 1 year ago

Given the crate seems unmaintained I'm not sure it matters

Right, I had to fork it for another project I was working on, and the stuff in this crate really belong upstream. Last I checked there is still ongoing work for that.

Though, the RFC looks a little dormant, too.

"if your system can run rustc X, we support it" seems like a great policy to me

I think we are talking past each other. You basically just said the Minimum Supported Rust Version in a different way :)

rmsyn commented 1 year ago

FWIW Rust 1.55 is also nearly two years old itself, much less the current MSRV.

TheBlueMatt commented 1 year ago

I think we are talking past each other. You basically just said the Minimum Supported Rust Version in a different way :)

I think you missed my note that newer rustc doesn't work on older macOS - so you can't support new macOS on older rustc, hence if you limit to "if your system can run..." then the MSRV can absolutely be older ;)

FWIW Rust 1.55 is also nearly two years old itself, much less the current MSRV.

I'm aware. Projects I work on sadly have to support 1.48 still both for validation and to build for old targets.

rmsyn commented 1 year ago

hence if you limit to "if your system can run..." then the MSRV can absolutely be older

Fair enough, but you see the contradiction here? The MSRV to get the build to work under the macOS in the CI is higher than what is available on the older macOS (making older macOS a non-supported platform).

With CI breaking, it limits the usability for all the other targets not just the older macOS targets (which are technically unsupported until added to the CI).

It truly doesn't matter to me, there is a fork of this code, the fork does what I need it to, and I'll replace it as soon as upstream implements these traits in core.

I submitted this patch in an attempt to help the users/maintainers of this crate have something that builds in the CI.

If there is a similar fix that doesn't require the MSRV bump, and achieves the same as the changes in https://github.com/technocreatives/core2/pull/21/commits/699986bfc52716b1eef0fb430911379d14613e16, great.

Maybe something like https://crates.io/crates/rustversion would work, but that comes with an additional dependency.

Edit: other errors also come up using rustversion:


Run actions-rs/cargo@v1
/Users/runner/.cargo/bin/cargo test --tests --no-default-features --features nightly
    Updating crates.io index
 Downloading crates ...
  Downloaded rustversion v1.0.14
  Downloaded memchr v2.6.1
   Compiling rustversion v1.0.14
   Compiling memchr v2.6.1
   Compiling core2 v0.4.0 (/Users/runner/work/core2/core2)
error[E0658]: non-inline modules in proc macro input are unstable
Error:  --> src/lib.rs:8:1
  |
8 | pub mod error;
  | ^^^^^^^^^^^^^^
  |
  = note: see issue #54727 <https://github.com/rust-lang/rust/issues/54727> for more information
  = help: add `#![feature(proc_macro_hygiene)]` to the crate attributes to enable

For more information about this error, try `rustc --explain E0658`.
error: could not compile `core2` (lib) due to previous error
Error: warning: build failed, waiting for other jobs to finish...
error: could not compile `core2` (lib test) due to previous error
Error: The process '/Users/runner/.cargo/bin/cargo' failed with exit code 101

From changes in https://github.com/technocreatives/core2/pull/24

If the maintainer(s) have abandoned the crate, then this whole discussion is pointless.

Your patch also fails CI checks, so no further additions will pass CI until the changes in this PR are addressed.

TheBlueMatt commented 1 year ago

Fair enough, but you see the contradiction here? The MSRV to get the build to work under the macOS in the CI is higher than what is available on the older macOS (making older macOS a non-supported platform).

That's not what I'm suggesting at all, you can also run older macOS in CI :)