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

Fix _CMSG_ALIGN on DragonFly #2610

Merged
merged 1 commit into from
Jan 4, 2022
Merged

Conversation

rtzoeller
Copy link
Contributor

@rtzoeller rtzoeller commented Dec 31, 2021

@rust-highfive
Copy link

r? @Amanieu

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

Copy link
Contributor

@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.

Note that this is not a libc bug. Rather, it's an ABI change in Dragonfly. libc's code was correct at the time it was written. Presumably this PR will break Nix's SCM tests for older Dragonfly versions. I don't see any CHANGELOG file for libc. Is there a different way to announce a change of the minimum supported OS version ?

DragonFlyBSD/DragonFlyBSD@b3aa44a

src/unix/bsd/freebsdlike/dragonfly/mod.rs Outdated Show resolved Hide resolved
@rtzoeller
Copy link
Contributor Author

Is there a different way to announce a change of the minimum supported OS version ?

In the past I've been told to mark items impacted by ABI changes as deprecated, and then waiting a few libc releases before submitting the change. In theory this notifies everyone uptaking new libc versions at a periodic cadence.

My reluctance to do so here is motivated by my experience doing so for a previous DragonFly ABI change:

  • We needed to adjust the build process for the nix crate twice in order to accommodate the change due to the deprecation warnings being converted to errors. Given the small DragonFly user-base and its tier 3 status, IMHO handling this deprecation was disproportionately painful to the actual impact of the change.
  • DragonFlyBSD/DragonFlyBSD@b3aa44a was submitted two years and multiple DragonFly releases ago, first appearing in DragonFly 5.8. I don't believe any versions of DragonFly older than that are still supported by the DragonFly team.
  • libc does not list a supported version for DragonFly, and the current state of libc likely straddles multiple ABI versions making it an imperfect match for any version of DragonFly.

IMHO, given current interest/investment levels towards rust on DragonFly, the best strategy is for libc to always target the latest in-development version of DragonFly (perhaps stabilizing briefly around DragonFly release windows). This helps guarantee that each DragonFly release is reasonably supported by at least one version of libc, date-aligned. If needed, users could force other crates to use this version of libc and cherry-pick additional changes on top of it.

I'm not sure there's sufficient interest to maintain support for multiple versions like FreeBSD, and having a "live at head" policy seems more internally consistent than the ABI-version-mystery-meat that exists today. It's certainly imperfect, but I'd wager most people uptaking new versions of rust on DragonFly are also uptaking new versions of DragonFly, and vice-versa.

@asomers
Copy link
Contributor

asomers commented Jan 2, 2022

I agree that #[deprecated] is too much trouble for this case. It's not really appropriate anyway, for APIs that will be changed but not removed, without replacement.

@Amanieu
Copy link
Member

Amanieu commented Jan 3, 2022

I'm not sure there's sufficient interest to maintain support for multiple versions like FreeBSD, and having a "live at head" policy seems more internally consistent than the ABI-version-mystery-meat that exists today. It's certainly imperfect, but I'd wager most people uptaking new versions of rust on DragonFly are also uptaking new versions of DragonFly, and vice-versa.

I'm happy to go with this for now (unless someone comes along and complains).

@bors r+

@bors
Copy link
Contributor

bors commented Jan 3, 2022

📌 Commit 67464ff has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Jan 4, 2022

⌛ Testing commit 67464ff with merge f51fd85...

@bors
Copy link
Contributor

bors commented Jan 4, 2022

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Amanieu
Pushing f51fd85 to master...

@bors bors merged commit f51fd85 into rust-lang:master Jan 4, 2022
bors added a commit that referenced this pull request Jan 24, 2022
Fix _CMSG_ALIGN on DragonFly

The attempted fix in #2610 originally had `7` hard coded, but it was suggested I replace this with a size_of call. Unfortunately the suggestion omitted a subtraction from the size_of call, and I didn't catch it.

Tested by running the failing `nix` tests on DragonFly (and didn't change the code again after running the tests).
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.

5 participants