-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Re-enable _LARGEFILE64_SOURCE when _GNU_SOURCE is defined #20123
Conversation
318a3aa
to
22e7dcb
Compare
According to https://musl.libc.org/releases.html this was done on purpose and codebases that depend on these legacy LFS64 glibc-ABI-compat symbols should be fixed instead:
However, I'm aware that Rust builds are currently broken due to this, I submitted a PR for this at rust-lang/libc#3308 last month. For the same reason, Alpine has not shipped this in the v3.18.x cycle, see commits: |
I think going with that glibc does and being a little more friendly to legacy software is good course of action for emscripten in this case. Its fine for upstream musl to be more strict than us |
OK to land this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm to land quickly if this is blocking something.
When we updated to the latest version of musl it broke some codebases that were using LFS functions (e.g. `stat64`) and assuming those functions would be defined when `_GNU_SOURCE` is defined. See bminor/musl@25e6fee This change effectively reverts the above one by defining _LARGEFILE64_SOURCE whenever _GNU_SOURCE is defined. This is what glibc does: https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/include/features.h#L206
22e7dcb
to
8045c65
Compare
Doing this in the short term seems fine to unblock users, but I'm not convinced this will work in the longer term. As mentioned, LARGEFILE_64_SOURCE is going away upstream in the next release, and I'm not sure we want to maintain downstream patches for this (especially not integrated into various headers the way these are, as that would be an invasive change to maintain). We should probably try to figure out whether we can keep this compatibility in a more easily-maintainable way, or maybe we can help our users fix their code. |
I think punting for now is fine. "The latter will also be removed in a future version" isn't necessarily the next release.. and musl releases a few and far between. I think we can pick the best option for dealing with that event once it happens. |
When we updated to the latest version of musl it broke some codebases that were using LFS functions (e.g.
stat64
) and assuming those functions would be defined when_GNU_SOURCE
is defined.See bminor/musl@25e6fee
This change effectively reverts the above one by defining _LARGEFILE64_SOURCE whenever _GNU_SOURCE is defined.
This is what glibc does:
https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/include/features.h#L206