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.

Sign up for GitHub

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

Fix Redox MSRV CI #228

Merged
merged 1 commit into from
Jul 21, 2024
Merged

Fix Redox MSRV CI #228

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ jobs:
run: |
cargo update -p ahash --precise 0.8.7
cargo update -p bumpalo --precise 3.14.0
cargo update -p redox_syscall@0.5.3 --precise 0.5.2
Copy link
Member Author

Choose a reason for hiding this comment

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

This is all very unfortunate, newer versions of Cargo support redox_syscall@0.5.

Copy link
Member

Choose a reason for hiding this comment

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

Truth be told this is somewhat appalling to me. Why would such a foundational crate be bumped to such a relatively recent version of Rust?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK they don't have a MSRV policy at all.

Copy link
Member

Choose a reason for hiding this comment

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

Poke @jackpot51, would you consider bringing a more conservative MSRV policy into the foundational Redox crates?

Copy link
Member

Choose a reason for hiding this comment

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

What is the desired MSRV?

Copy link
Member Author

@daxpedda daxpedda Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Softbuffer currently requires v1.70.
But the problem is rather that we need some sort of MSRV policy.

E.g. don't increase the MSRV without a minor version update or something (the policy most crates I know have).

@notgull on that note, I know that all crates in rust-windowing have the same MSRV policy as the one described in Winit, but we should still add that to the README here.

EDIT: Following the MSRV policy of Winit, the next version could be updated to v1.77. But we aren't there yet.

Copy link
Member

Choose a reason for hiding this comment

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

Worth noting that redox_syscall is only useful for compiling for Redox, and Redox is only supporting a specific nightly toolchain at this point in time, nightly-2024-05-11, which identifies itself as rustc 1.80.0-nightly (6e1d94708 2024-05-10).

It is strange to call it "appalling", for a crate that is only designed for use on an operating system that only supports a toolchain newer than Rust 1.77 to use features available in that toolchain.

Can I get some more background on why this was a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is strange to call it "appalling", for a crate that is only designed for use on an operating system that only supports a toolchain newer than Rust 1.77 to use features available in that toolchain.

I believe that @notgull is terribly passionate about this specific problem. The word "appalling" is a bit strong worded imo.

Can I get some more background on why this was a problem?

The problem is that it breaks our CI and also breaks any promises we make to the user. I entirely forgot that Redox requires nightly, so I agree adding an MSRV policy to redox_syscall really doesn't make any sense.

I think what we need to do is:

  1. Don't check MSRV for Redox in our CI.
  2. Document en exception for Redox in our MSRV policy, exactly like we do for Android as well.

Thanks @jackpot51.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do not check MSRV on Redox, at least until we are able to use a stable compiler.

If redox_syscall is causing MSRV checks to fail on other Operating Systems, that is not desired behavior so please let me know!


- name: Build crate
shell: bash
Expand Down