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 compiling with libfmt>=11.0 #13262

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

Ferdi265
Copy link
Contributor

@Ferdi265 Ferdi265 commented Jan 4, 2025

This PR fixes compilation with libfmt 11.0 and later. Without this fix, the build fails on Arch Linux with fmt 11.1.1.

The main changes are that fmt::detail::is_compile_string no longer exists and that fmt::string_view is no longer directly constructible from compile time format strings (not sure exactly why), but manually constructing a fmt::format_string before converting to fmt::string_view works.

The change to fmt v11 also brought up that BBA/HLE BuiltIn.cpp seems to contain an invalid format string that hasn't been caught previously, fixed that as well.

Tested compilation and basic functionality with the bundled fmt 10.2.1 and with fmt 11.1.1.

I'm not sure if the implementation is perfect, I'll try to find a cleaner way to do this that requires fewer #ifdefs and I'm also open to suggestions.

@Ferdi265
Copy link
Contributor Author

Ferdi265 commented Jan 4, 2025

Something still seems to be wrong here. I'll doublecheck this and mark this as draft in the meantime.

@Ferdi265 Ferdi265 marked this pull request as draft January 4, 2025 18:08
@Ferdi265 Ferdi265 changed the title Fix compiling with libfmt>=11.0 WIP: Fix compiling with libfmt>=11.0 Jan 4, 2025
@Ferdi265 Ferdi265 marked this pull request as ready for review January 4, 2025 19:29
@Ferdi265 Ferdi265 changed the title WIP: Fix compiling with libfmt>=11.0 Fix compiling with libfmt>=11.0 Jan 4, 2025
@Ferdi265
Copy link
Contributor Author

Ferdi265 commented Jan 4, 2025

Builds pass, local testing checks out, I think this is ready.

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. Thanks for doing this. Untested.

Copy link
Member

@dreamsyntax dreamsyntax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on CachyOS (Arch) which currently can't build master.
Worked for me as expected.

@dreamsyntax
Copy link
Member

Completed building and running this branch on both
Debian 12 and Linux Mint 22.
Builds and runs successfully.

I think we're good to merge.

@AdmiralCurtiss AdmiralCurtiss merged commit 22dc21c into dolphin-emu:master Jan 8, 2025
13 checks passed
@Nystrata
Copy link
Contributor

Nystrata commented Jan 8, 2025

Can confirm that it builds and runs on my endeavorOS Arch system where master (two days ago) would fail to build before

@v-fox
Copy link

v-fox commented Jan 9, 2025

I'm still getting a fmt-related error on openSUSE Tumbleweed:

[  728s] ld.lld: error: undefined symbol: fmt::v11::detail::locale_ref::locale_ref<std::locale, 0>(std::locale const&)
[  728s] >>> referenced by State.cpp
[  728s] >>>               ../Core/dolphin-emu.lto.libcore.a(State.cpp.o at 1002102).o:(State::GetInfoStringOfSlot[abi:cxx11](int, bool))
[  728s] >>> referenced by Movie.cpp
[  728s] >>>               ../Core/dolphin-emu.lto.libcore.a(Movie.cpp.o at 999822).o:(Movie::MovieManager::GetRTCDisplay[abi:cxx11]() const)

Even though /usr/lib64/libfmt.so is being passed for linking.

@Ferdi265
Copy link
Contributor Author

Ferdi265 commented Jan 9, 2025

I'm still getting a fmt-related error on openSUSE Tumbleweed:

[  728s] ld.lld: error: undefined symbol: fmt::v11::detail::locale_ref::locale_ref<std::locale, 0>(std::locale const&)
[  728s] >>> referenced by State.cpp
[  728s] >>>               ../Core/dolphin-emu.lto.libcore.a(State.cpp.o at 1002102).o:(State::GetInfoStringOfSlot[abi:cxx11](int, bool))
[  728s] >>> referenced by Movie.cpp
[  728s] >>>               ../Core/dolphin-emu.lto.libcore.a(Movie.cpp.o at 999822).o:(Movie::MovieManager::GetRTCDisplay[abi:cxx11]() const)

Even though /usr/lib64/libfmt.so is being passed for linking.

That looks like it's related to these two locale-related fmt calls:

State.cpp:292 (likely inlined from this call) and
Movie.cpp:174.

I'm not sure why that fails to link. Does it work prior to this PR being merged?

@Ferdi265
Copy link
Contributor Author

Ferdi265 commented Jan 9, 2025

I'm still getting a fmt-related error on openSUSE Tumbleweed:

[  728s] ld.lld: error: undefined symbol: fmt::v11::detail::locale_ref::locale_ref<std::locale, 0>(std::locale const&)
[  728s] >>> referenced by State.cpp
[  728s] >>>               ../Core/dolphin-emu.lto.libcore.a(State.cpp.o at 1002102).o:(State::GetInfoStringOfSlot[abi:cxx11](int, bool))
[  728s] >>> referenced by Movie.cpp
[  728s] >>>               ../Core/dolphin-emu.lto.libcore.a(Movie.cpp.o at 999822).o:(Movie::MovieManager::GetRTCDisplay[abi:cxx11]() const)

Even though /usr/lib64/libfmt.so is being passed for linking.

This appears to be an unintentional ABI break in fmt 11.0.0 -> 11.1.1.

@v-fox
Copy link

v-fox commented Jan 9, 2025

I'm not sure why that fails to link. Does it work prior to this PR being merged?

Haven't updated dolphin in a while, so not sure exactly when it stopped building.

This appears to be an fmtlib/fmt#4292 in fmt 11.0.0 -> 11.1.1.

fmt is quickly becoming my most hated dependency ever. Every single time I'll update my emulator collection to latest versions/snapshots and half of them fails with most incomprehensible errors due to fmt.

This particular break was affecting easyeffects. But it did not break for me because fmt package in the main repo was already rebuilt and EE was rebuilding against it. Not sure why dolphin would be affected now.

@v-fox
Copy link

v-fox commented Jan 9, 2025

I've managed to make the build and my worst expectations about the culprit seems to have been confirmed: it's libOpenAL.

The current main branch have started statically compiling-in the bundled version of fmt which conflicts with system one when an app links to both libopenal and libfmt. And most notable emulators do both, so when it makes an official release they will all start failing to build or launch on mismatch.

Someone needs to force the OpenAL author to stop doing this inane crap and start linking to the system fmt. But it's hardcoded into cmake scripts so much, it seems like very intentional broken-by-design chose that he's proud of, so he may refuse.

@OpenSauce04
Copy link

OpenSauce04 commented Mar 3, 2025

Azahar developer here, was experiencing some odd issues with fmt >=11.1.3 after upgrading submodules, and I also discovered that OpenAL was the culprit.

With OpenAL at version 1.24.2, fmt was not working how it should at all, but with OpenAL 1.24.1 it works fine.

@Ferdi265
Copy link
Contributor Author

Ferdi265 commented Mar 3, 2025

Azahar developer here, was experiencing some odd issues with fmt >=11.1.3 after upgrading submodules, and I also discovered that OpenAL was the culprit.

With OpenAL at version 1.24.2, fmt was not working how it should at all, but with OpenAL 1.24.1 it works fine.

It could be that what you are seeing is openal#1105, since openal links a vendored static copy of libfmt, which can cause issues if there are ABI incompatibilities between the versions used. This was fixed (by renaming the inline namespace in openal to fmt::v11_openal) on January 26th, but not released yet.

EDIT: Completely forgot that @v-fox already talked about this.

@Ferdi265 Ferdi265 deleted the feature/fix-fmt11 branch March 3, 2025 01:28
@iwubcode iwubcode mentioned this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants