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

lib/fmt: New library #601

Closed
wants to merge 1 commit into from
Closed

lib/fmt: New library #601

wants to merge 1 commit into from

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Jul 2, 2022

libfmt/fmt requires some patching to make it work on NXDK

I've done the patching in my fork https://github.com/glebm/fmt/tree/nxdk

It'd be better to have the fork be in the XboxDev org but I don't have permissions to set it up (please feel free to do so)

Fixes #594

@AJenbo
Copy link

AJenbo commented Jul 6, 2022

This is also part of C++20 so devs would start to expect it afaik?

@dracc
Copy link
Contributor

dracc commented Jul 6, 2022

This is also part of C++20 so devs would start to expect it afaik?

libcxx already supports C++20, doesn't it? In that case, this library would be extraneous. :)

@glebm
Copy link
Contributor Author

glebm commented Jul 6, 2022

fmt implements a lot more than what's in C++20.

Both DevilutionX and SDL_audiolib use it and we do not plan to migrate to std::format anytime soon as we need to support a language level somewhere around C++14 (gcc 6.3).

@glebm
Copy link
Contributor Author

glebm commented Jul 7, 2022

Additionally, std::format is not widely implemented yet. Not even clang implements it fully (and it's hidden behind a flag).
People basically use libfmt so that they can use all the features without worrying about compiler support.
libfmt also implements print which is still at a proposal stage for C++23.

@JayFoxRox
Copy link
Member

Thanks for the work; however, I think this port should not be a part of the nxdk repository.

See:

This PR, for example, has to add a generic rule for *.cc, which probably should not be part of the nxdk build-system.
We have been abusing the old nxdk Makefile for way too long.


We should go forward with adding better package support or submodule / install dependencies from projects until nxdk has a package manager.
DevolutionX (for which this libfmt is meant) already has a brewfile and install instructions to install deps for most platforms - why not include libfmt install instructions for nxdk?

Upstreaming of packages is still a good goal of course, so the community benefits from any ports.
Hence, it makes sense to list new dependencies on the wiki, until we have a better solution (~package-repository + package-manager)

Adding more packages to nxdk only complicates maintenance of nxdk and makes it harder to create a clean package manager (especially with old projects depending on more and more default packages or even hardcoded paths within nxdk).


Packages which should continue to live in nxdk (for now) are drivers (such as pbkit) and low level standard APIs which mimic the OS / userspace APIs (win32 / POSIX / ...).
Inclusion of SDL2 is an edge-case already and we mostly include it to standardize APIs until we settle on underlying drivers (as we had no stable API for audio and input, it was easier to recommend SDL2 as stable API).

Any API which is purely optional or just implementing logic should be avoided. I consider libfmt such a library.
Other libraries which should be removed from the core set of nxdk libraries are zlib, libjpeg and libpng.

@glebm
Copy link
Contributor Author

glebm commented Jul 10, 2022

Makes sense.

NXDK does not implement enough Windows APIs to compile libfmt, so the NXDK-compatible version of libfmt requires some patching.

We can switch over to the patched version of libfmt in DevilutionX.

Can you fork https://github.com/fmtlib/fmt to XboxDev/fmt and give me access?
I'll push our patch there to the nxdk branch so that everyone can benefit.

@glebm glebm closed this Jul 10, 2022
@JayFoxRox
Copy link
Member

JayFoxRox commented Jul 10, 2022

Can you fork https://github.com/fmtlib/fmt to XboxDev/fmt and give me access?

I personally think that it's also fine to keep the libfmt port on a personal account for now.
I'm concerned with diffusion of responsibility while also actively promoting libfmt as an official XboxDev port which should imply a maintained port.
However, I don't think we have enough people in XboxDev to keep a libfmt port up-to-date (we already struggle to keep SDL2 and libcxx updated, despite being essential in almost every project - they are outdated by years).
Clearly we need a better solution to keep track of all these libraries (and appoint responsible people or encourage updates to libraries).

However, I'm mostly inactive, so I'll let other XboxDev maintainers decide.

@AJenbo
Copy link

AJenbo commented Jul 10, 2022

We build the NXDK version of DevilutionX multiple times each day, we would be happy to maintain the libfmt port in NXDK and update it as we bump our requirements.

@glebm
Copy link
Contributor Author

glebm commented Sep 19, 2023

libfmt no longer requires patching as of fmtlib/fmt#3636 🎉

Only a couple of options need to be set.

E.g. when including libfmt as a CMake subproject:

set(FMT_OS OFF) # required for nxdk
include(FetchContent)
FetchContent_Declare(libfmt
  URL https://github.com/fmtlib/fmt/archive/a8a73da7e44e26d7c18d752976522eff7a21e0bf.tar.gz
  URL_HASH MD5=4ddd617a0921e919f1c2e6ab7c36c349
)
FetchContent_MakeAvailable(libfmt)
target_compile_definitions(fmt PUBLIC FMT_WINDOWS_NO_WCHAR) # required for nxdk

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.

Port libfmt
4 participants