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

Raise libc's FreeBSD ABI to 12 #2406

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Sep 17, 2021

FreeBSD 11 will be EoL on 30-Sept-2021. Update libc's ABI to the
minimum supported version of FreeBSD.

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@Amanieu
Copy link
Member

Amanieu commented Sep 18, 2021

LGTM, but cc @JohnTitor just in case.

@JohnTitor
Copy link
Member

If rustc drops it, I think it's fine. Rustc's CI currently uses FreeBSD 11 (https://github.com/rust-lang/rust/blob/35c8f2612ffea42cc09350accdba934e85a19f35/src/ci/docker/scripts/freebsd-toolchain.sh#L8), could you ask infra folks if it's okay, and make a PR? When it's approved, I'll r+ this one.

@asomers
Copy link
Contributor Author

asomers commented Sep 18, 2021

rust PR: rust-lang/rust#89083 .

@bors
Copy link
Contributor

bors commented Nov 18, 2021

☔ The latest upstream changes (presumably #2545) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnTitor
Copy link
Member

Triage: I guess we should wait for rust-lang/rust#89058, and I wonder if we could release this along with the musl change.

@asomers
Copy link
Contributor Author

asomers commented Feb 22, 2023

@JohnTitor there is a possibility that this change could cause breakage downstream, for example if a downstream crate assumes something about the size of ino_t. Is there any way to use crater to check whether downstream crates will still compile?

@workingjubilee
Copy link
Member

Tagging this as breakage-candidate on the same "should be included in the upcoming review" rationale.

@workingjubilee
Copy link
Member

workingjubilee commented Mar 2, 2023

Also I hit up grep.app and searched for libc::ino_t, and I didn't find many users of it at all. Most of them were perfectly "by value" uses. The closest I can find is this thingie, "Reverie", which assumes that libc::ino_t will fit inside u32... and in that case, it's done via a direct cast rather than unsafe code.

Also, Reverie is Linux-only to begin with.

@jrtc27
Copy link

jrtc27 commented Jul 26, 2023

Is any progress happening on this? Given Rust's continued inflexibility regarding OS ABIs, this is blocking FreeBSD/riscv from having a working Rust toolchain.

@workingjubilee
Copy link
Member

@asomers Please resolve the merge conflict.

@asomers
Copy link
Contributor Author

asomers commented Jul 26, 2023

Merge conflict resolved, @workingjubilee .

@workingjubilee
Copy link
Member

workingjubilee commented Jul 28, 2023

My inclination is also to Just Merge This Already and have it go out in libc 0.2.{next}, with disregard to users (incorrectly) assuming things about the type of ino_t (hopefully they used the type alias fully knowing its value can differ from platform to platform?), but given the concern initially raised by @asomers, I started a conversation on Zulip to feel out if there's consensus or additional concerns.

Also after further reading the Reverie thing is a weird gdb thing that has its own definition and any resemblance to stat is in truth incidental, apparently?

I feel like #3139 is a better example of something we might want to wait for 0.3.0 before accepting. Might.

@JohnTitor
Copy link
Member

Given rust-lang/rust#114521 has been merged and rust-lang/rust#89058 is now accepted, we can go ahead with this change. Removing FreeBSD 11 (and older) support should be announced somewhere (we may deprecate the modules first).
@bors r+

@bors
Copy link
Contributor

bors commented Nov 6, 2023

📌 Commit d3f5a15 has been approved by JohnTitor

It is now in the queue for this repository.

@JohnTitor
Copy link
Member

Maybe making a Rust inside blog post before releasing would be an alternative way to announce, I guess.

@bors
Copy link
Contributor

bors commented Nov 6, 2023

☀️ Try build successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Build commit: 17cc521 (17cc5219ae6bb41ef2ff7d076c423a864e18de66)

@BlackHoleFox
Copy link

@JohnTitor A +1 from me at least on the blog post idea. It seemed to work fine for the Apple minimum bumps and I have yet to see anyone calling for my head for lack of notice.

@tgross35
Copy link
Contributor

tgross35 commented Nov 19, 2024

We can probably drop this without an announcement at this point since FreeBSD 11 has been EOL for over three years, long enough that I don't think anyone would be reasonably expecting support. FreeBSD 12 is EOL but we won't drop that here until Rust moves off of it and the platform docs merge (rust-lang/rust#129220).

@rustbot author for a rebase (ah, looks like my labels raced)

@asomers
Copy link
Contributor Author

asomers commented Nov 19, 2024

@tgross35 since this PR was opened main and 0.2 have become separate branches. Since breaking changes are allowed in main, the barrier to merging this PR should be even lower, right?

@tgross35
Copy link
Contributor

That is correct. I think we can probably drop 11 from both branches, then maybe drop 12 from main? If there is a policy that makes sense then we can put it in the readme.

@asomers
Copy link
Contributor Author

asomers commented Nov 19, 2024

That is correct. I think we can probably drop 11 from both branches, then maybe drop 12 from main? If there is a policy that makes sense then we can put it in the readme.

I would love to do that. The alternative suggestion that I would make would be to drop 11 from libc-0.2, for riscv64 only. The reason being that FreeBSD 11 didn't even support riscv64, so there are no backwards-compatibility concerns. Then after some period of time if there are no complaints from riscv users we could drop it for all architectures.

@tgross35
Copy link
Contributor

That is correct. I think we can probably drop 11 from both branches, then maybe drop 12 from main? If there is a policy that makes sense then we can put it in the readme.

I would love to do that. The alternative suggestion that I would make would be to drop 11 from libc-0.2, for riscv64 only. The reason being that FreeBSD 11 didn't even support riscv64, so there are no backwards-compatibility concerns. Then after some period of time if there are no complaints from riscv users we could drop it for all architectures.

I think we may as well drop 11 entirely if we intend to drop it at all, but if there are concerns that the version is still relevant then maybe the blog post idea is worth it? Feel free to author something short, just submit a PR adding to this directory.

In any case you are the target maintainer so I'm fine with whatever you decide here :) just update this PR with as is reasonable.

@asomers
Copy link
Contributor Author

asomers commented Nov 21, 2024

@tgross35 I've rebased the PR. I think we can merge this to main now. I tested it by running the test suites of several crates with a patched libc 0.2.164 that used a FreeBSD 12 ABI. All passed. Interestingly, one (cap-std) failed with the unmodified libc, but passed with the modified one. That's a direct result of the 64-bit inode change.

[✓] bfffs
[✓] bsd-pidfile-rs
[✓] cap-ftpd
[✓] cap-std ( fails with vanilla libc; works with patched )
[✓] capsicum-net
[✓] capsicum-rs
[✓] fsx-rs
[✓] fuse-ufs
[✓] gstat-rs
[✓] libnv-rs
[✓] mdconfig
[✓] mio
[✓] mio-aio
[✓] nix
[✓] rctl
[✓] rustix
[✓] sysctl-rs
[✓] tokio
[✓] xattr
[✓] xfuse
[✓] ztop

FreeBSD 11 was EoL on 30-Sept-2021.  Update libc's ABI to 12.  That
version includes significant changes, such as 64-bit inodes.
@tgross35 tgross35 added this pull request to the merge queue Nov 21, 2024
Merged via the queue into rust-lang:main with commit 63cf6a1 Nov 21, 2024
45 checks passed
@asomers asomers deleted the fbsd12-default branch November 24, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants