Skip to content

Conversation

@ArmstrongCN
Copy link

This PR updates our codebase to comply with the new formatting rules and API changes introduced in fmtlib v10+ while keeping the compatity of fmt v8.

Key changes include:

  1. add default enum formatter that without stringfy impl.
  2. fix some non-activated stringfy struct.

relate issue: #173

@ArmstrongCN ArmstrongCN changed the title Support new formatter rules introduced in fmt v10 Support new formatter rules introduced in fmt v10 and v11 Mar 12, 2025
@ArmstrongCN
Copy link
Author

I have formatted the commit, please review again.

the commit is tested with both fmt v8.0.1(the original) and fmt v11.1.4(the newest). Updating the fmt also requires a compatible version of spdlog.

Changes:

  1. make custom formatter const
  2. make default enum formatter for non-stringfy type
  3. fmt::join is moved to <fmt/ranges.h>
  4. some source files are not properly include the enum formatter

@ArmstrongCN ArmstrongCN requested a review from JackLau1222 March 13, 2025 05:13
@hulibruce
Copy link
Collaborator

Thank you for your contribution. We are excited to announce an open-source rewards event—every valid merge request qualifies for a reward! For more details, please join the Feishu and WeChat groups listed in our documentation or contact us directly. All prizes will be distributed collectively at the end of the event.
https://bytedance.larkoffice.com/docx/YJSVdaSaIo6pmgxf9WgcZcvwnge?from=from_copylink

Copy link
Collaborator

@JackLau1222 JackLau1222 left a comment

Choose a reason for hiding this comment

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

First of all, thanks very much for your great work.
However, this multi-version compatibility issue involves API changes. It may be more appropriate to get the version number in the cmake build and then implement it according to the version in the code.

@ArmstrongCN
Copy link
Author

The patch is compatible with fmt versions ranging from v8 to the latest v11. Therefore, I believe there is no need to modify the CMake build script.

@ArmstrongCN
Copy link
Author

UPDATE: support both char and wchar_t

Copy link
Collaborator

@JackLau1222 JackLau1222 left a comment

Choose a reason for hiding this comment

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

there're some conflicts with latest commit, may you rebase this patch?

1. make custom formatter const
2. make default enum formatter for non-stringfy type
3. fmt::join is moved to <fmt/ranges.h>
4. some source files are not properly include the enum formatter
@ArmstrongCN
Copy link
Author

rebase to main with no conflicts

@ArmstrongCN ArmstrongCN requested a review from JackLau1222 April 8, 2025 03:21
@hulibruce
Copy link
Collaborator

In the GPU environment, compilation fails with fmt versions 10.0.0 and 11.0.2. It's likely that some fmt functions used exclusively in the GPU context require version-specific adaptations.

@ArmstrongCN
Copy link
Author

include the header <hmp/format.h> resolves the cudaError_enum issue.

@hulibruce
Copy link
Collaborator

still have compilation errors when using fmt 10.0.0 (tag a0b8a92e3d1532361c2f7feb63babc5c18d00ef2) in the GPU environment

@ArmstrongCN
Copy link
Author

still have compilation errors when using fmt 10.0.0 (tag a0b8a92e3d1532361c2f7feb63babc5c18d00ef2) in the GPU environment

Add the line set(SPDLOG_FMT_EXTERNAL ON CACHE BOOL "SPDLOG_FMT_EXTERNAL" FORCE) to ignore the fmt bundled with spdlog which cause the conflict.

By the way, this may probably reduce the final package size. I GUESS.

@h-vetinari
Copy link

Hi! 👋

This patch would be very helpful for conda-forge, where we generally only build for one fmt version across the entire ecosystem, and we've been on fmt v11 for a long time already. The fact that bmf is not yet compatible with this makes it difficult to keep the package up-to-date and usable.

I've tried backporting this patch in conda-forge/bmf-feedstock#36 (on top of v0.1.0), and it still fails.

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.

4 participants