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

linux fixes recvmmsg flags type for musl/emscripten. #2963

Merged
merged 1 commit into from
Oct 23, 2022

Conversation

devnexen
Copy link
Contributor

closes #2945

@rust-highfive
Copy link

r? @JohnTitor

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

@devnexen devnexen marked this pull request as ready for review October 14, 2022 19:39
@JohnTitor
Copy link
Member

Changing their types would break existing code, I'd prefer to follow https://github.com/rust-lang/libc/blob/master/CONTRIBUTING.md#breaking-change-policy first.

 a type change to fit Musl and Emscripten.

close rust-lang#2945.
@JohnTitor
Copy link
Member

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Oct 23, 2022

📌 Commit 00204b0 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 23, 2022

⌛ Testing commit 00204b0 with merge a59c842...

@bors
Copy link
Contributor

bors commented Oct 23, 2022

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing a59c842 to master...

@bors bors merged commit a59c842 into rust-lang:master Oct 23, 2022
@asomers
Copy link
Contributor

asomers commented Oct 23, 2022

This change is causing deprecation warnings downstream, and there's nothing that downstream crates can do about it except to suppress the warning. I don't think the policy laid out in https://github.com/rust-lang/libc/blob/master/CONTRIBUTING.md#breaking-change-policy is appropriate in this case. That policy would make sense if upstream really were changing an API, and especially if upstream were removing it. But in this case musl/emscripten didn't change anything. Instead, libc was simply wrong. I think libc should just go ahead and fix the definition for musl/emscripten.
A deprecation notice, IMHO, should only be used if there's some way for a downstream crate to be compatible with both the deprecated libc definitions and the fixed ones.

asomers added a commit to asomers/nix that referenced this pull request Oct 23, 2022
asomers added a commit to asomers/nix that referenced this pull request Oct 23, 2022
@JohnTitor
Copy link
Member

A deprecation notice, IMHO, should only be used if there's some way for a downstream crate to be compatible with both the deprecated libc definitions and the fixed ones.

Users can prepare the upcoming type changes if they depend on it.

@niluxv
Copy link
Contributor

niluxv commented Oct 24, 2022

This change is breaking CI for rustix (and probably other crates), which has deny(warnings) in the CI. And as @asomers already said, there is nothing (except allow(deprecated)) the user can do about this. Deprecating items is a mechanism for soft removal, not for announcing modifications to existing APIs.

devnexen added a commit to devnexen/libc that referenced this pull request Oct 24, 2022
@thomcc
Copy link
Member

thomcc commented Oct 25, 2022

Yes, as I mentioned here, these deprecations don't feel like the right way to handle API changes -- it makes sense to handle removals that way, but it's unclear what downstream users are supposed to do for changes other than #[allow(...)] them.

We should find an alternative approach though. I'm guessing an 0.3 is considered off the table?

devnexen added a commit to devnexen/libc that referenced this pull request Oct 25, 2022
devnexen added a commit to devnexen/libc that referenced this pull request Oct 25, 2022
bors added a commit that referenced this pull request Oct 25, 2022
…itor

follow-up on #2963, changing MSG* constant types for musl/emscripten.
@davidv1992
Copy link

I came here from the deprecation warnings spitted out, but I am deeply unclear given the above discussion what the expected alternative going forward is. These constants still seem to exist in glibc, so under what name can we expect them in the future?

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.

Mismatch between flag type and function argument for recvmmsg
8 participants