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

Build system: default to fmt shared library when available #8198

Merged
merged 1 commit into from Mar 15, 2020
Merged

Build system: default to fmt shared library when available #8198

merged 1 commit into from Mar 15, 2020

Conversation

mazes-80
Copy link

Detect system libfmt, with minimal version 5.3.
I don't know if minimal version is required, but as it is the one found in externals ...

CMakeLists.txt Outdated Show resolved Hide resolved
leoetlino
leoetlino previously approved these changes Nov 9, 2019
@leoetlino leoetlino dismissed their stale review November 9, 2019 23:13

Oops, just noticed there's a new version of fmtlib (6.0.0)...

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Since fmtlib 6.0.0 -- which contains backward-compatibility breaking changes -- has been released, and distros will be moving to 6.0.0 in the future, could you bump the requirement to 6.0.0 and update the copy in Externals please? Otherwise Dolphin will stop building at some point with this change...

@orbea
Copy link
Contributor

orbea commented Nov 10, 2019

A lot of distros already provide 6.0.0.

https://repology.org/project/fmt/versions

@mazes-80
Copy link
Author

I updated fmt from External, everything seems compiles fine, no breaking changes detected yet !
The switch works well as my system fmt is 5.3, build system used 6.0.0 from "static"

@leoetlino
Copy link
Member

Hmm, it's weird that everything still builds fine because fmt/time.h was removed in 6.0, and Core (IIRC) includes it.

@BhaaLseN
Copy link
Member

The functionality is still there, but I can't find where we use it (nor in which PR I mentioned this before).
I mean, as long as it works...

@leoetlino
Copy link
Member

Looks like the PR is based on an old-ish commit. Please rebase and fix the build issues that will most likely appear after doing so. (I think the only change you have to make is replace fmt/time.h with fmt/chrono.h.)

@leoetlino leoetlino dismissed their stale review November 24, 2019 21:56

comments addressed

@leoetlino
Copy link
Member

Looks like fmt 6.0.0 introduces a lot of new warnings (fmtlib/fmt#1278). Maybe we should consider disabling warnings for fmt (or even Externals in general)?

@orbea
Copy link
Contributor

orbea commented Jan 3, 2020

FWIW fmt upstream is now at 6.1.2, this PR still being worked on?

@mazes-80
Copy link
Author

mazes-80 commented Jan 7, 2020

FWIW fmt upstream is now at 6.1.2, this PR still being worked on?

For now using fmt 6.1.2 from system fails to compile:
Source/Core/VideoCommon/BPStructs.cpp:992:55: error: cannot bind bitfield 'left_top.X10Y10::.X10Y10::::x' to 'unsigned int&'
Source/Core/VideoCommon/BPStructs.cpp:1186:77: error: cannot bind bitfield 'teximg.TexImage0::.TexImage0::::format' to 'unsigned int&'
Source/Core/VideoCommon/BPStructs.cpp:1209:40: error: cannot bind bitfield 'teximg.TexImage1::.TexImage1::::tmem_even' to 'unsigned int&'
Source/Core/VideoCommon/BPStructs.cpp:1232:40: error: cannot bind bitfield 'teximg.TexImage2::.TexImage2::::tmem_odd' to 'unsigned int&'

Locally I just add a '+ 0' to all those complaining bitfields, it works, but I'm sure it is not the proper way to cast. What would be the good way to cast ?

@leoetlino
Copy link
Member

leoetlino commented Jan 7, 2020

Apparently this is a known issue with 6.0+. The recommended workaround is to use the unary plus operator:

fmtlib/fmt#1284 (comment)

@mazes-80
Copy link
Author

mazes-80 commented Jan 9, 2020

Apparently this is a known issue with 6.0+. The recommended workaround is to use the unary plus operator.

Pushed fmt 6.1.2, added unary plus were they were missing.

@orbea
Copy link
Contributor

orbea commented Jan 9, 2020

@mazes-80 I don't see the new commits?

@orbea
Copy link
Contributor

orbea commented Jan 9, 2020

Fwiw this builds here.

$  ldd /usr/games/dolphin-emu | grep -i fmt
	libfmt.so.6 => /usr/lib64/libfmt.so.6 (0x00007feed2d98000)

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay... I've rebased your branch and removed some (now) unnecessary commits (considering #8602 was merged meanwhile). Thanks for your contribution!

@leoetlino leoetlino merged commit 97aaee1 into dolphin-emu:master Mar 15, 2020
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Mar 20, 2020
Follow the update pntroduced in
dolphin-emu/dolphin#8198

Reported-by: Samuel BAUER
Package-Manager: Portage-2.3.94, Repoman-2.3.21
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
@mazes-80 mazes-80 deleted the system-fmt branch October 23, 2020 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants