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

music_mpg123.c: disable _FILE_OFFSET_BITS suffixes on libmpg123 calls. #677

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

sezero
Copy link
Contributor

@sezero sezero commented Mar 13, 2025

Avoids libmpg123's terrifying mess. Useful if mpg123_shared is disabled
and if SDL_mixer is built with _FILE_OFFSET_BITS=64.

If accepted, will cherry pick to SDL2 branch too (also to release-2.8.x)

( CC @smcv )

@sezero sezero requested review from slouken and madebr March 13, 2025 01:09
Avoids libmpg123's terrifying mess. Useful if mpg123_shared is disabled
and if SDL_mixer is built with _FILE_OFFSET_BITS=64.
@sezero sezero force-pushed the MPG123_NO_LARGENAME branch from 6816066 to 5e932e1 Compare March 13, 2025 17:40
@sezero sezero merged commit 72a7333 into libsdl-org:main Mar 14, 2025
5 checks passed
@sezero sezero deleted the MPG123_NO_LARGENAME branch March 14, 2025 17:14
@smcv
Copy link
Contributor

smcv commented Mar 17, 2025

Useful if mpg123_shared is disabled

We do this in Debian to get "normal" shared library dependencies in an environment where it's easy for the package manager to enforce them. I assume this is why I was pinged?

and if SDL_mixer is built with _FILE_OFFSET_BITS=64

We avoid doing this on Debian i386 for legacy binary compatibility (to avoid breaking the ABI of old libraries), and it's a no-op on x86_64 (offsets are always 64-bit on any reasonable 64-bit architecture), so the Steam Runtime is unaffected.

Debian armel and armhf (the surviving 32-bit non-i386 architectures), together with some unofficial ports like m68k, might conceivably be affected by whatever issue this is solving - but I don't see any compiler warnings in the armel or armhf build logs, so perhaps we dodged that particular bullet.

@smcv
Copy link
Contributor

smcv commented Mar 17, 2025

It might be more future-proof to explicitly call mpg123_seek64() and so on (without the underscore), which always use int64_t for offsets, and do not vary with the size of off_t.

@sezero
Copy link
Contributor Author

sezero commented Mar 17, 2025

Useful if mpg123_shared is disabled

We do this in Debian to get "normal" shared library dependencies in an environment where it's easy for the package manager to enforce them. I assume this is why I was pinged?

Yup

and if SDL_mixer is built with _FILE_OFFSET_BITS=64

We avoid doing this on Debian i386 for legacy binary compatibility (to avoid breaking the ABI of old libraries), and it's a no-op on x86_64 (offsets are always 64-bit on any reasonable 64-bit architecture), so the Steam Runtime is unaffected.

Debian armel and armhf (the surviving 32-bit non-i386 architectures), together with some unofficial ports like m68k, might conceivably be affected by whatever issue this is solving - but I don't see any compiler warnings in the armel or armhf build logs, so perhaps we dodged that particular bullet.

OK, that's great

It might be more future-proof to explicitly call mpg123_seek64() and so on (without the underscore), which always use int64_t for offsets, and do not vary with the size of off_t.

That would forcibly require MPG123_API_VERSION >= 48 (mpg123 >= 1.32.0), but something to think about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants