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

Jump to bottom

Bump MSRV to 1.63 for I/O safety #1862

Merged
merged 4 commits into from
Dec 4, 2022
Merged

Bump MSRV to 1.63 for I/O safety #1862

merged 4 commits into from
Dec 4, 2022

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented Nov 13, 2022 •
edited
Loading

Prep for #1750

@SUPERCILEX SUPERCILEX mentioned this pull request Nov 19, 2022
Open
29 tasks
@asomers asomers added this to the 0.27.0 milestone Nov 28, 2022
@asomers
Copy link
Member

asomers commented Dec 3, 2022

Needs a rebase.

@notgull
Copy link
Contributor

notgull commented Dec 3, 2022

Is there any way we can use io-lifetimes or a similar crate to avoid an MSRV bump?

@SUPERCILEX
Copy link
Contributor Author

@asomers rebased.

@notgull The rationale was that moving to I/O safety is already a massive breaking change, so a MSRV bump shouldn't add extra pain.

@SUPERCILEX
Copy link
Contributor Author

Some clippy lints I applied were false positives that needed the 2021 edition, so I bumped us up to that.

@SUPERCILEX
Copy link
Contributor Author

#1861 should be merged first because clippy changed stuff in the code we're deleting.

@SUPERCILEX
Copy link
Contributor Author

Unrelated CI failure

asomers
asomers reviewed Dec 3, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

It looks like the OSX build failed because it tried to download cargo-hack exactly as that project uploaded a new release.

.cirrus.yml Outdated
env:
TARGET: x86_64-unknown-linux-musl
setup_script:
- rustup target add $TARGET
- rustup component add clippy
- rustup toolchain install $TOOLCHAIN --profile minimal --target $TARGET
Copy link
Member

Choose a reason for hiding this comment

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

Why is this step necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the other setup_script blocks. If you remove this I'm pretty sure the build fails with a wrong rust version error.

Copy link
Member

Choose a reason for hiding this comment

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

And yet the old version didn't fail. I think this section is masking some other problem.

asomers
asomers reviewed Dec 4, 2022
View reviewed changes
.cirrus.yml Outdated
@@ -9,7 +9,7 @@ env:
RUSTDOCFLAGS: -D warnings
TOOL: cargo
# The MSRV
TOOLCHAIN: 1.56.1
TOOLCHAIN: 1.63
Copy link
Member

Choose a reason for hiding this comment

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

Make this change and I don't think you'll need to reinstall the toolchain on line 159 below.

Suggested change
TOOLCHAIN: 1.63
TOOLCHAIN: 1.63.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, fixed.

@asomers asomers mentioned this pull request Dec 4, 2022
Merged
asomers
asomers reviewed Dec 4, 2022
View reviewed changes
src/sys/mman.rs Outdated
);

let ret = libc::mmap(ptr, length.into(), prot.bits(), flags.bits(), fd, offset);
let ptr =
Copy link
Member

Choose a reason for hiding this comment

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

There's more formatting changes like this mixed in with content changes. Can you please move them into separate commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was due to the edition bump, but it looks like this is because I'm on nightly? When I run a cargo fmt on master this stuff changes. Created #1909

@SUPERCILEX SUPERCILEX mentioned this pull request Dec 4, 2022
Merged
bors bot added a commit that referenced this pull request Dec 4, 2022
@bors @SUPERCILEX
Merge #1909
3286c4d 
1909: More annoying formatting changes r=asomers a=SUPERCILEX

Extracted from #1862

Co-authored-by: Alex Saveau <saveau.alexandre@gmail.com>
asomers
asomers requested changes Dec 4, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -73,6 +73,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
([#1870](https://github.com/nix-rust/nix/pull/1870))
- The `length` argument of `sys::mman::mmap` is now of type `NonZeroUsize`.
([#1873](https://github.com/nix-rust/nix/pull/1873))
- The MSRV is now 1.63
Copy link
Member

Choose a reason for hiding this comment

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

Arg, you need to move this part to the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shucks, fixed.

@SUPERCILEX
Bump MSRV to 1.63 for I/O safety
1f68cf6 
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Bump the edition since why not
e2ff9d5 
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Fix clippy lints
fb80246 
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Tweak cirrus rust versions
bc2b023 
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
asomers
asomers approved these changes Dec 4, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@asomers asomers mentioned this pull request Dec 4, 2022
Merged
@bors
Copy link
Contributor

bors bot commented Dec 4, 2022

Build succeeded:

@bors bors bot merged commit e756c96 into nix-rust:master Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.27.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SUPERCILEX @asomers @notgull