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

Fixes for nightly features, bump MSRV #21

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rmsyn
Copy link

@rmsyn rmsyn commented Mar 17, 2023

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

`maybe_uninit_ref` has stabilized since 1.55, and `read_initializer` is
no longer a nightly feature.

Remove both features now that stable versions on supported (run in CI)
packages include 1.55+ compilers.

Consider bumping MSRV to 1.55+
All latest stable on supported OSes is at least ten versions ahead of
the MSRV.

Fixes MSRV build on MacOS
@rmsyn
Copy link
Author

rmsyn commented Apr 6, 2023

Ping @bbqsrc

@rmsyn
Copy link
Author

rmsyn commented Jun 10, 2023

Ping @bbqsrc, is this repository still maintained?

@TheBlueMatt
Copy link
Contributor

Not sure why this bumps the MSRV?

@rmsyn
Copy link
Author

rmsyn commented Aug 29, 2023

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
Copy link
Contributor

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
Copy link
Author

rmsyn commented Aug 30, 2023

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
Copy link
Contributor

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
Copy link
Author

rmsyn commented Aug 30, 2023

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
Copy link
Author

rmsyn commented Aug 30, 2023

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

@TheBlueMatt
Copy link
Contributor

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
Copy link
Author

rmsyn commented Aug 30, 2023

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 699986b, 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 #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.

@rmsyn rmsyn mentioned this pull request Aug 30, 2023
@TheBlueMatt
Copy link
Contributor

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 :)

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.

2 participants