fmt fixes by KrisThielemans · Pull Request #1605 · UCL/STIR

KrisThielemans · 2025-07-16T15:07:32Z

When compiling with C++-20 and using `fmt`, some problems transpired. This commit fixes those.

This avoids lilnking problems with external "users" of STIR.

This was breaking due to fmt being used in with C++-20, but we didn't test it.

- a debug case, which I forgot - in KeyParser, for whatever reason there was ambiguity between std::format and stir::format.

When using format_string, I get error: no matching function for call to ‘vformat(std::format_string<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>&, ...' with g++-13 /14 and C++20

Doesn't create dependencies, so original version should be fine.

Also fix test in format.h (we needed <version>)

KrisThielemans · 2025-08-30T23:20:19Z

@markus-jehl this works now. Possibly you could have a look, but I'm tempted to merge rather soon, such that we can move on.

The target_link_libraries(... PRIVATE fmt) still had link problems for external dependencies. Luckily, fmt also has a header-only target.

KrisThielemans · 2025-08-31T08:22:48Z

Using

target_link_libraries(... PRIVATE fmt)

still created problems on my Ubuntu 16.04 test system (wth gcc-8), although not in CI. Luckily, depending on fmt::fmt-header-only solved that for me.

KrisThielemans · 2025-08-31T09:01:08Z

Still no luck! Building with STIR now gives

The following imported targets are referenced, but are missing:
  fmt::fmt-header-only

will need to use the trick of

STIR/src/buildblock/CMakeLists.txt

Lines 123 to 135 in 2ecec35

    
           # Add the header-only nlohman_json header-only library 
        
           # Unfortunately, the simple line below exports the dependency while this is really not 
        
           # necessary. 
        
           # 
        
           # target_link_libraries(buildblock PRIVATE "$<BUILD_INTERFACE:nlohmann_json::nlohmann_json>") 
        
           # So, we currently use an ugly work-around from 
        
           # https://gitlab.kitware.com/cmake/cmake/-/issues/15415#note_334852 
        
           # Warning: this will fail once nlohman_json stops being header-only! 
        
           # In that case, we will need to add it in STIRConfig.cmake.in 
        
           # 
        
           get_target_property(TMP nlohmann_json::nlohmann_json INTERFACE_INCLUDE_DIRECTORIES) 
        
           target_include_directories(buildblock PUBLIC "${TMP}")

KrisThielemans · 2025-08-31T09:04:25Z

Actually, we then will need to #define FMT_HEADER_ONLY by hand (this is normally done by the target)

markus-jehl · 2025-09-01T06:46:35Z

@markus-jehl this works now. Possibly you could have a look, but I'm tempted to merge rather soon, such that we can move on.

Ooof, sorry to have put you through this ordeal! Went through your changes and they look good (I haven't had time to test it myself yet, but feel free to merge anyways). Probably I just haven't run into the same issue because we use fmt ourselves as well...

Otherwise we'd need to add the /utf-8 switch.

KrisThielemans · 2025-09-01T07:27:06Z

With the latest, AppVeyor and most of GHA is fine, except the gcc-12 job, which seems to hang in ctest (no output). See e.g. https://github.com/UCL/STIR/actions/runs/17370039888/job/49303726707
This is a case where {fmt} is used. Sigh...

@markus-jehl you don' thappen to have a gcc-12 ubuntu 22.04 system lying around?

markus-jehl · 2025-09-01T07:34:21Z

With the latest, AppVeyor and most of GHA is fine, except the gcc-12 job, which seems to hang in ctest (no output). See e.g. https://github.com/UCL/STIR/actions/runs/17370039888/job/49303726707 This is a case where {fmt} is used. Sigh...

@markus-jehl you don' thappen to have a gcc-12 ubuntu 22.04 system lying around?

Unfortunately not, but I can try to look into it. Earliest by Thursday, though :-(

KrisThielemans · 2025-09-01T09:39:34Z

Everything works fine on the SIRF 3.8.1 VM (building a new STIR with gcc-12), so I'm not sure what's going on.

It's also not clear if this is actually ctest, as it seems the install command hasn't completed yet.
another job https://github.com/UCL/STIR/actions/runs/17370039888/job/49303726705#step:11:1932 has

Installing: /home/runner/work/STIR/STIR/install/bin/stir_image_to_simset_object.sh
-- Installing: /home/runner/work/STIR/STIR/install/bin/fwdtest
-- Set non-toolchain portion of runtime path of "/home/runner/work/STIR/STIR/install/bin/fwdtest" to ""
-- Installing: /home/runner/work/STIR/STIR/install/bin/bcktest
-- Set non-toolchain portion of runtime path of "/home/runner/work/STIR/STIR/install/bin/bcktest" to ""
-- Installing: /home/runner/work/STIR/STIR/install/share/STIR-6.2/config
-- Installing: /home/runner/work/STIR/STIR/install/share/STIR-6.2/config/radionuclide_info.json
-- Installing: /home/runner/work/STIR/STIR/install/share/STIR-6.2/config/radionuclide_names.json
-- Installing: /home/runner/work/STIR/STIR/install/share/STIR-6.2/config/ct_slopes.json
-- Installing: /home/runner/work/STIR/STIR/install/python/_stir.so

while the problematic one stops with

Installing: /home/runner/work/STIR/STIR/install/bin/fwdtest
-- Set non-toolchain portion of runtime path of "/home/runner/work/STIR/STIR/install/bin/fwdtest" to ""
-- Installing: /home/runner/work/STIR/STIR/install/bin/bcktest

Of course, the fact that it stops there could just be due to caching of the output.

No idea what's going on.

I guess I'll have to enable tmate to ssh into the runner, when I find some time.

KrisThielemans · 2025-09-02T07:36:50Z

I guess I'll have to enable tmate to ssh into the runner

I did: went in via ssh, built, ran ctest. All fine. (it timed out before I could do more).

I have no idea what's going on therefore. I've now updated the doc, and this should be good to go. Hopefully, the gcc12 job problem auto-disappears...

KrisThielemans · 2025-09-02T10:08:37Z

No luck with that job. As I don't know how to diagnose/resolve, I'm afraid I'm going to disable that job from the Actions and squash-merge.

This job seems to hang since using header-only {fmt}, although not when using tmate to ssh into the runner. Commenting it out therefore...

KrisThielemans added this to the v6.3 milestone Jul 16, 2025

KrisThielemans self-assigned this Jul 16, 2025

KrisThielemans force-pushed the fmt_format_fixes branch 3 times, most recently from 183b727 to 8f8bd58 Compare July 17, 2025 00:33

KrisThielemans added 7 commits July 17, 2025 01:37

re-add stir/format.h

a8cdd30

fixes for format

c7514e2

When compiling with C++-20 and using `fmt`, some problems transpired. This commit fixes those.

Make fmt a private dependency

ca4e46a

This avoids lilnking problems with external "users" of STIR.

[GHA] add gcc-12 c++-20 test case

36947ed

This was breaking due to fmt being used in with C++-20, but we didn't test it.

fix a few more cases of format()

d35e90d

- a debug case, which I forgot - in KeyParser, for whatever reason there was ambiguity between std::format and stir::format.

use string_view as argument for format()

63e828c

When using format_string, I get error: no matching function for call to ‘vformat(std::format_string<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>&, ...' with g++-13 /14 and C++20

attempt using std::format if it's available

8ad6203

KrisThielemans force-pushed the fmt_format_fixes branch from 8f8bd58 to 8ad6203 Compare July 17, 2025 00:37

KrisThielemans added 3 commits August 30, 2025 21:08

revert format() changes in .txx

5bbe464

Doesn't create dependencies, so original version should be fine.

remove one more format() from .h

b3d13b6

[AppVeyor] make sure CMake finds the conda python

0b63a2b

KrisThielemans force-pushed the fmt_format_fixes branch 2 times, most recently from c49fb36 to 3a73667 Compare August 30, 2025 22:58

If std::format works, no longer depend on fmt

e913b80

Also fix test in format.h (we needed <version>)

KrisThielemans force-pushed the fmt_format_fixes branch from 3a73667 to e913b80 Compare August 30, 2025 23:05

KrisThielemans requested a review from markusjehl August 30, 2025 23:20

KrisThielemans added 2 commits August 31, 2025 09:18

use header-only version of fmt

01402ee

The target_link_libraries(... PRIVATE fmt) still had link problems for external dependencies. Luckily, fmt also has a header-only target.

remove fmt dependency from STIRConfig.cmake

c24bb57

correct documentation on FMT_DEPENDENCY

0a23578

force fmt to be header-only by using #define

78cfb59

KrisThielemans force-pushed the fmt_format_fixes branch from 25533c1 to 468c6f0 Compare August 31, 2025 20:34

KrisThielemans force-pushed the fmt_format_fixes branch from 468c6f0 to f550483 Compare August 31, 2025 20:49

remove FMT_DEPENDENCY but add FMT_INCLUDE_DIRS

6754bea

KrisThielemans force-pushed the fmt_format_fixes branch from f550483 to 6754bea Compare August 31, 2025 21:11

disable fmt's Unicode support for MSVC

c4e7e8e

Otherwise we'd need to add the /utf-8 switch.

update doc to latest fmt work-arounds

fde71c1

[GHA] commented out gcc-12 job

3c7bd4b

This job seems to hang since using header-only {fmt}, although not when using tmate to ssh into the runner. Commenting it out therefore...

KrisThielemans merged commit 24f18bc into UCL:master Sep 3, 2025
10 checks passed

KrisThielemans deleted the fmt_format_fixes branch September 3, 2025 06:58

KrisThielemans mentioned this pull request Sep 22, 2025

GitHub Actions jobs out of disk-space #1618

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fmt fixes#1605

fmt fixes#1605
KrisThielemans merged 19 commits intoUCL:masterfrom
KrisThielemans:fmt_format_fixes

KrisThielemans commented Jul 16, 2025

Uh oh!

KrisThielemans commented Aug 30, 2025

Uh oh!

KrisThielemans commented Aug 31, 2025

Uh oh!

KrisThielemans commented Aug 31, 2025

Uh oh!

KrisThielemans commented Aug 31, 2025

Uh oh!

markus-jehl commented Sep 1, 2025

Uh oh!

KrisThielemans commented Sep 1, 2025

Uh oh!

markus-jehl commented Sep 1, 2025

Uh oh!

KrisThielemans commented Sep 1, 2025

Uh oh!

KrisThielemans commented Sep 2, 2025

Uh oh!

KrisThielemans commented Sep 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

KrisThielemans commented Jul 16, 2025

Uh oh!

KrisThielemans commented Aug 30, 2025

Uh oh!

KrisThielemans commented Aug 31, 2025

Uh oh!

KrisThielemans commented Aug 31, 2025

Uh oh!

KrisThielemans commented Aug 31, 2025

Uh oh!

markus-jehl commented Sep 1, 2025

Uh oh!

KrisThielemans commented Sep 1, 2025

Uh oh!

markus-jehl commented Sep 1, 2025

Uh oh!

KrisThielemans commented Sep 1, 2025

Uh oh!

KrisThielemans commented Sep 2, 2025

Uh oh!

KrisThielemans commented Sep 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants